Received: by 2002:a05:6a10:2785:0:0:0:0 with SMTP id ia5csp2582507pxb; Mon, 11 Jan 2021 13:37:52 -0800 (PST) X-Google-Smtp-Source: ABdhPJx2LqYE2wc/NssjTDEd97sEmRvm4aFwyLcNHwlOeMWkKHpcpdl04Aaae1P4BPzctZc4jeE7 X-Received: by 2002:a17:906:34ca:: with SMTP id h10mr951462ejb.417.1610401072028; Mon, 11 Jan 2021 13:37:52 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1610401072; cv=none; d=google.com; s=arc-20160816; b=PW6HOBBbR+eks5/GGrenC/k4xZ/OGP0s5y7riPG0euzB1/dKeUD6YWXdMrEnsjbhpW wIqBXiKmxWc14gsLOIcFC+iBrnw+mUfpOUC1IwiT5xEDbvbn/xCll5vkvq5iDbT4LU9S /IDkXLB+vjqvOQ8OxBSGtRB33uTU8CAyJ+L9RbanIF5cXzk2Eg/GWiyfHVnfw+ZPwdMH ctPRHBbovaKUp7UsszqYs50/g2GyHQkpkAcGB8hGINN2dGLz1omfR8J6jbUsHwDqPhYp l6QxXhKqdr4FiFU6h+N6M8Ngte+sW1FREVjn7SJ9YHar1FEi2Ab9NPiuWhkjJuTPS91B Lo5Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=yjeqm/CWqpTTjyYDHxs2ABAQO1r3rsqMNKank7ho//g=; b=wxkIQ6JBNNBXB0KXggHkOoKM0gD0MSBJ/GWywHwdoN+Q4mcNWhDVEo+NpO+VVCk/da jBl1A/FR8I+gJAs9rDxP49kLuFAdKouqjL2gsh4Rghtv3dhYoQ3efbzWI3hFllfaU5el rcwQ1stpLpTdF0cK2UTbxA8nriX8ovpG/no0wRuoCozIz0L/esAKX1YKuCRLfZMeIj/j 7TSqBlw70jC23RGr8xV0S5FwEH9eonbc6fep80/FuFBPiUJYdN8fGUzTNHxhxq7qgrQl FAnEc9GC3vFiaIWNF/KmAGQI+cters6m+0wQjLavM65akiWPOOepmQOi/bHSSV2ynZ/J WqvA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=RlXW49Na; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id bx8si212587ejb.664.2021.01.11.13.37.27; Mon, 11 Jan 2021 13:37:52 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=RlXW49Na; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727073AbhAKVgh (ORCPT + 99 others); Mon, 11 Jan 2021 16:36:37 -0500 Received: from mail.kernel.org ([198.145.29.99]:47326 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728324AbhAKVgh (ORCPT ); Mon, 11 Jan 2021 16:36:37 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id D2C7022D0A for ; Mon, 11 Jan 2021 21:35:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1610400956; bh=x27HDn56BBd7xtw8Y4u+duMMPb+kznDGfTPxdKkdPWc=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=RlXW49NaFQIkHt4dCWDn1UqS36e9nMdJcSf1Wlyi2c1m1St6bIH+It2McCPP4V74J vCJsj7Cbui021n20zTccENm0/dRXV3CR3bd1nxPIqG2zVJPgMQ8St3ezUlM9cd8dPD q1RHDfuD6FMset6PYR3FYDveKUJtD9z300eUepzN0ikrOkqakhHYmL7ZvedFgHwqVx b5rQbv2SucmbytcEte2xQvGjO2YaVDGGJCJPbwZN3j0McGU6/YkEN3Ad/YcHqUBtsR h/sj3KJ/uKKQTIvmaRjX2b6YPmmZbq/NGxo5p5VR0lO712R/Wb2MlJ/CVoM4t/wVYr 1krerSKlEr9ew== Received: by mail-lf1-f47.google.com with SMTP id s26so158074lfc.8 for ; Mon, 11 Jan 2021 13:35:55 -0800 (PST) X-Gm-Message-State: AOAM532RU/osHLOCQl0gbwXj83vZQdwcayi42HFGJ+beHIZ9qDTo7TP7 CunziRxbvwLBzKIilMIYPIQ78HuLElOj6p7DOpJD5Q== X-Received: by 2002:a05:6512:398e:: with SMTP id j14mr683024lfu.9.1610400953864; Mon, 11 Jan 2021 13:35:53 -0800 (PST) MIME-Version: 1.0 References: <20210108231950.3844417-1-songliubraving@fb.com> <20210108231950.3844417-2-songliubraving@fb.com> <20210111185650.hsvfpoqmqc2mj7ci@kafai-mbp.dhcp.thefacebook.com> In-Reply-To: <20210111185650.hsvfpoqmqc2mj7ci@kafai-mbp.dhcp.thefacebook.com> From: KP Singh Date: Mon, 11 Jan 2021 22:35:43 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH bpf-next 1/4] bpf: enable task local storage for tracing programs To: Martin KaFai Lau Cc: Song Liu , bpf , Networking , open list , mingo@redhat.com, Peter Zijlstra , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , John Fastabend , Kernel Team , Hao Luo , kernel test robot Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 11, 2021 at 7:57 PM Martin KaFai Lau wrote: > > On Fri, Jan 08, 2021 at 03:19:47PM -0800, Song Liu wrote: > > [ ... ] > > > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c > > index dd5aedee99e73..9bd47ad2b26f1 100644 > > --- a/kernel/bpf/bpf_local_storage.c > > +++ b/kernel/bpf/bpf_local_storage.c > > @@ -140,17 +140,18 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem) > > { > > struct bpf_local_storage *local_storage; > > bool free_local_storage = false; > > + unsigned long flags; > > > > if (unlikely(!selem_linked_to_storage(selem))) > > /* selem has already been unlinked from sk */ > > return; > > > > local_storage = rcu_dereference(selem->local_storage); > > - raw_spin_lock_bh(&local_storage->lock); > > + raw_spin_lock_irqsave(&local_storage->lock, flags); > It will be useful to have a few words in commit message on this change > for future reference purpose. > > Please also remove the in_irq() check from bpf_sk_storage.c > to avoid confusion in the future. It probably should > be in a separate patch. > > [ ... ] > > > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c > > index 4ef1959a78f27..f654b56907b69 100644 > > diff --git a/kernel/fork.c b/kernel/fork.c > > index 7425b3224891d..3d65c8ebfd594 100644 > [ ... ] > > > --- a/kernel/fork.c > > +++ b/kernel/fork.c > > @@ -96,6 +96,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -734,6 +735,7 @@ void __put_task_struct(struct task_struct *tsk) > > cgroup_free(tsk); > > task_numa_free(tsk, true); > > security_task_free(tsk); > > + bpf_task_storage_free(tsk); > > exit_creds(tsk); > If exit_creds() is traced by a bpf and this bpf is doing > bpf_task_storage_get(..., BPF_LOCAL_STORAGE_GET_F_CREATE), > new task storage will be created after bpf_task_storage_free(). > > I recalled there was an earlier discussion with KP and KP mentioned > BPF_LSM will not be called with a task that is going away. > It seems enabling bpf task storage in bpf tracing will break > this assumption and needs to be addressed? For tracing programs, I think we will need an allow list where task local storage can be used.