On Thu, Sep 30, 2021 at 8:47 PM Martin KaFai Lau <kafai@xxxxxx> 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.
