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;
MIME-Version: 1.0
References: <20201027170317.2011119-1-kpsingh@chromium.org> <20201027170317.2011119-2-kpsingh@chromium.org>
In-Reply-To: <20201027170317.2011119-2-kpsingh@chromium.org>
From:   Song Liu <song@kernel.org>
Date:   Thu, 29 Oct 2020 16:27:58 -0700
Message-ID: <CAPhsuW6yFbWLGZwpCE4whUm_ncJG4Fr7kf75XeqYLRWG8PvnWQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next 1/5] bpf: Implement task local storage
To:     KP Singh <kpsingh@chromium.org>
Cc:     open list <linux-kernel@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Paul Turner <pjt@google.com>, Jann Horn <jannh@google.com>,
        Hao Luo <haoluo@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Oct 28, 2020 at 9:17 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> Similar to bpf_local_storage for sockets and inodes add local storage
> for task_struct.
>
> The life-cycle of storage is managed with the life-cycle of the
> task_struct.  i.e. the storage is destroyed along with the owning task
> with a callback to the bpf_task_storage_free from the task_free LSM
> hook.

It looks like task local storage is tightly coupled to LSM. As we discussed,
it will be great to use task local storage in tracing programs. Would you
like to enable it from the beginning? Alternatively, I guess we can also do
follow-up patches.

>
> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in
> the security blob which are now stackable and can co-exist with other
> LSMs.
>
> The userspace map operations can be done by using a pid fd as a key
> passed to the lookup, update and delete operations.

While testing task local storage, I noticed a limitation of pid fd:

/* Currently, the process identified by
 * @pid must be a thread-group leader. This restriction currently exists
 * for all aspects of pidfds including pidfd creation (CLONE_PIDFD cannot
 * be used with CLONE_THREAD) and pidfd polling (only supports thread group
 * leaders).
 */

This could be a problem for some use cases. How about we try to remove
this restriction (maybe with a new flag to pidfd_open) as part of this set?

Thanks,
Song

[...]