Re: [PATCH RFC bpf-next 0/2] Mmapable task local storage.

[KP Singh]

On Fri, Apr 1, 2022 at 1:06 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Mar 31, 2022 at 3:32 PM KP Singh <kpsingh@xxxxxxxxxx> wrote:
> >
> > On Wed, Mar 30, 2022 at 8:26 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Mar 30, 2022 at 11:16 AM Alexei Starovoitov
> > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > >
> > > > On Wed, Mar 30, 2022 at 11:06 AM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, Mar 29, 2022 at 4:30 PM Alexei Starovoitov
> > > > > <alexei.starovoitov@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Yonghong,
> > > > > > > > > >
> > > > > > > > > > On Fri, Mar 25, 2022 at 12:16 PM Yonghong Song <yhs@xxxxxx> 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.
>
> I think it's overkill for existing unpriv maps like hash and array.
> These maps by themself don't pose a security threat.
> The sysctl was/is in the wrong place.
>
> > 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");
>
> local storage map was not exposed to unpriv before.
> This would be a different consideration.
> But even in such a case the extra flag looks unnecessary.

I took a look at the code and it makes sense to allow maps like hash and array
which are already mmapable. These should not really need the BPF_F_UNPRIVILEGED.

Hao, I think you can send a patch that removes these map operations
from the scope of
the sysctl. Regarding the local storages, let's do them separately
since it would be a newer
access surface.