Message-ID: <CACYkzJ6-csGkdMQiGYYr5_DgShPWrUFfs92sUOhwzQt=T13+SA@mail.gmail.com>
Date:   Fri, 1 Apr 2022 00:32:10 +0200
From:   KP Singh <kpsingh@...nel.org>
To:     Hao Luo <haoluo@...gle.com>, Jann Horn <jannh@...gle.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Yonghong Song <yhs@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, bpf <bpf@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC bpf-next 0/2] Mmapable task local storage.

On Wed, Mar 30, 2022 at 8:26 PM Hao Luo <haoluo@...gle.com> wrote:
>
> On Wed, Mar 30, 2022 at 11:16 AM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Wed, Mar 30, 2022 at 11:06 AM Hao Luo <haoluo@...gle.com> wrote:
> > >
> > > On Tue, Mar 29, 2022 at 4:30 PM Alexei Starovoitov
> > > <alexei.starovoitov@...il.com> wrote:
> > > >
> > > > On Tue, Mar 29, 2022 at 10:43:42AM -0700, Hao Luo wrote:
> > > > > On Tue, Mar 29, 2022 at 2:37 AM Kumar Kartikeya Dwivedi
> > > > > <memxor@...il.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 28, 2022 at 11:16:15PM IST, Hao Luo wrote:
> > > > > > > On Mon, Mar 28, 2022 at 10:39 AM Hao Luo <haoluo@...gle.com> wrote:
> > > > > > > >
> > > > > > > > Hi Yonghong,
> > > > > > > >
> > > > > > > > On Fri, Mar 25, 2022 at 12:16 PM Yonghong Song <yhs@...com> wrote:
> > > > > > > > >
> > > > > > > > > On 3/24/22 4:41 PM, Hao Luo wrote:
> > > > > > > > > > Some map types support mmap operation, which allows userspace to
> > > > > > > > > > communicate with BPF programs directly. Currently only arraymap
> > > > > > > > > > and ringbuf have mmap implemented.
> > > > > > > > > >
> > > > > > > > > > However, in some use cases, when multiple program instances can
> > > > > > > > > > run concurrently, global mmapable memory can cause race. In that
> > > > > > > > > > case, userspace needs to provide necessary synchronizations to
> > > > > > > > > > coordinate the usage of mapped global data. This can be a source
> > > > > > > > > > of bottleneck.
> > > > > > > > >
> > > > > > > > > I can see your use case here. Each calling process can get the
> > > > > > > > > corresponding bpf program task local storage data through
> > > > > > > > > mmap interface. As you mentioned, there is a tradeoff
> > > > > > > > > between more memory vs. non-global synchronization.
> > > > > > > > >
> > > > > > > > > I am thinking that another bpf_iter approach can retrieve
> > > > > > > > > the similar result. We could implement a bpf_iter
> > > > > > > > > for task local storage map, optionally it can provide
> > > > > > > > > a tid to retrieve the data for that particular tid.
> > > > > > > > > This way, user space needs an explicit syscall, but
> > > > > > > > > does not need to allocate more memory than necessary.
> > > > > > > > >
> > > > > > > > > WDYT?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Thanks for the suggestion. I have two thoughts about bpf_iter + tid and mmap:
> > > > > > > >
> > > > > > > > - mmap prevents the calling task from reading other task's value.
> > > > > > > > Using bpf_iter, one can pass other task's tid to get their values. I
> > > > > > > > assume there are two potential ways of passing tid to bpf_iter: one is
> > > > > > > > to use global data in bpf prog, the other is adding tid parameterized
> > > > > > > > iter_link. For the first, it's not easy for unpriv tasks to use. For
> > > > > > > > the second, we need to create one iter_link object for each interested
> > > > > > > > tid. It may not be easy to use either.
> > > > > > > >
> > > > > > > > - Regarding adding an explicit syscall. I thought about adding
> > > > > > > > write/read syscalls for task local storage maps, just like reading
> > > > > > > > values from iter_link. Writing or reading task local storage map
> > > > > > > > updates/reads the current task's value. I think this could achieve the
> > > > > > > > same effect as mmap.
> > > > > > > >
> > > > > > >
> > > > > > > Actually, my use case of using mmap on task local storage is to allow
> > > > > > > userspace to pass FDs into bpf prog. Some of the helpers I want to add
> > > > > > > need to take an FD as parameter and the bpf progs can run
> > > > > > > concurrently, thus using global data is racy. Mmapable task local
> > > > > > > storage is the best solution I can find for this purpose.
> > > > > > >
> > > > > > > Song also mentioned to me offline, that mmapable task local storage
> > > > > > > may be useful for his use case.
> > > > > > >
> > > > > > > I am actually open to other proposals.
> > > > > > >
> > > > > >
> > > > > > You could also use a syscall prog, and use bpf_prog_test_run to update local
> > > > > > storage for current. Data can be passed for that specific prog invocation using
> > > > > > ctx. You might have to enable bpf_task_storage helpers in it though, since they
> > > > > > are not allowed to be called right now.
> > > > > >
> > > > >
> > > > > The loading process needs CAP_BPF to load bpf_prog_test_run. I'm
> > > > > thinking of allowing any thread including unpriv ones to be able to
> > > > > pass data to the prog and update their own storage.
> > > >
> > > > If I understand the use case correctly all of this mmap-ing is only to
> > > > allow unpriv userspace to access a priv map via unpriv mmap() syscall.
> > > > But the map can be accessed as unpriv already.
> > > > Pin it with the world read creds and do map_lookup sys_bpf cmd on it.
> > >
> > > Right, but, if I understand correctly, with
> > > sysctl_unprivileged_bpf_disabled, unpriv tasks are not able to make
> > > use of __sys_bpf(). Is there anything I missed?
> >
> > That sysctl is a heavy hammer. Let's fix it instead.
> > map lookup/update/delete can be allowed for unpriv for certain map types.
> > There are permissions checks in corresponding lookup/update calls already.
>

(Adding Jann)

I wonder if we can tag a map as BPF_F_UNPRIVILEGED and allow the writes to
only maps that are explicitly marked as writable by unprivileged processes.

We will have task local storage in LSM programs that we
won't like unprivileged processes to write to as well.

struct {
        __uint(type, BPF_MAP_TYPE_TASK_STORAGE);
        __uint(map_flags, BPF_F_NO_PREALLOC | BPF_F_UNPRIVILEGED);
        __type(key, int);
        __type(value, struct fd_storage);
} task_fd_storage_map SEC(".maps");

- KP

> This sounds great. If we can allow basic map operations for some map
> types, it will change many use cases I'm looking at. Let me take a
> look and report back.

Powered by blists - more mailing lists