Re: [PATCH bpf-next 1/2] bpf: Allow bpf_local_storage to be used by sleepable programs

[KP Singh <kpsingh@kernel.org>]

On Thu, Sep 30, 2021 at 8:47 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Tue, Aug 31, 2021 at 11:32:17PM -0700, Martin KaFai Lau wrote:
> > > > > > > > > --- a/net/core/bpf_sk_storage.c
> > > > > > > > > +++ b/net/core/bpf_sk_storage.c
> > > > > > > > > @@ -13,6 +13,7 @@
> > > > > > > > >  #include <net/sock.h>
> > > > > > > > >  #include <uapi/linux/sock_diag.h>
> > > > > > > > >  #include <uapi/linux/btf.h>
> > > > > > > > > +#include <linux/rcupdate_trace.h>
> > > > > > > > >

[...]

> > > Or is there another way to update the verifier to recognize safe sk and inode
> > > pointers?
> > I was thinking specifically for this pointer walking case.
> > Take a look at btf_struct_access().  It walks the struct
> > in the verifier and figures out reading sock->sk will get
> > a "struct sock *".  It marks the reg to PTR_TO_BTF_ID.
> > This will allow the bpf prog to directly read from sk (e.g. sk->sk_num)
> > or pass the sk to helper that takes a "struct sock *" pointer.
> > Reading from any sk pointer is fine since it is protected by BPF_PROBE_MEM
> > read.  However, we only allow the sk from sock->sk to be passed to the
> > helper here because we only know this one is refcnt-ed.
> >
> > Take a look at check_ptr_to_btf_access().  An individual verifier_ops
> > can also have its own btf_struct_access.  One possibility is
> > to introduce a (new) PTR_TO_RDONLY_BTF_ID to mean it can only
> > do BPR_PROBE_MEM read but cannot be used in helper.
> KP,  not sure how far you are with the verifier change, if it is
> moving well, please ignore.  Otherwise,
> I looked at the sock a bit and I currently don't see
> potential concern on the following pointer case without the
> rcu_read_lock for those sock-related sleepable lsm hooks in bpf_lsm.c.
> If cases did come up later (e.g. other lsm hooks are added or something got
> overlooked), we could bring in a bigger hammer to make the above verifier
> change.  I think it should be fine to stop some exotic usage

+1 Makes sense.

> later that is proven to be not safe.  For the lsm sleepable hook case,
> another option is to lock the sock first before calling the bpf prog.
>
> If you agree a similar situation is also true for inode and task,
> do you want to respin the patches addressing other points
> discussed in this thread.

I will respin the series with the other changes discussed.