On Tue, Aug 31, 2021 at 11:32:17PM -0700, Martin KaFai Lau wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

--- 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>

 DEFINE_BPF_STORAGE_CACHE(sk_cache);

@@ -22,7 +23,8 @@ bpf_sk_storage_lookup(struct sock *sk, struct bpf_map *map, bool cacheit_lockit)
      struct bpf_local_storage *sk_storage;
      struct bpf_local_storage_map *smap;

-     sk_storage = rcu_dereference(sk->sk_bpf_storage);
+     sk_storage = rcu_dereference_protected(sk->sk_bpf_storage,
+                                            bpf_local_storage_rcu_lock_held());
      if (!sk_storage)
              return NULL;

@@ -258,6 +260,7 @@ BPF_CALL_4(bpf_sk_storage_get, struct bpf_map *, map, struct sock *, sk,
 {
      struct bpf_local_storage_data *sdata;

+     WARN_ON_ONCE(!bpf_local_storage_rcu_lock_held());
      if (!sk || !sk_fullsock(sk) || flags > BPF_SK_STORAGE_GET_F_CREATE)

sk is protected by rcu_read_lock here.
Is it always safe to access it with the rcu_read_lock_trace alone ?

We don't dereference sk with an rcu_dereference though, is it still the case for
tracing and LSM programs? Or is it somehow implicity protected even
though we don't use rcu_dereference since that's just a READ_ONCE + some checks?

e.g. the bpf_prog (currently run under rcu_read_lock()) may read the sk from
req_sk->sk which I don't think the verifier will optimize it out, so as good
as READ_ONCE(), iiuc.

The sk here is obtained from the bpf_lsm_socket_* hooks?  Those sk should have
a refcnt, right?  If that is the case, it should be good enough for now.

The one passed in the arguments yes, but if you notice the discussion in

https://lore.kernel.org/bpf/20210826133913.627361-1-memxor@gmail.com/T/#me254212a125516a6c5d2fbf349b97c199e66dce0 (local)

one may also get an sk in LSM and tracing progs by pointer walking.

Right.  There is pointer walking case.
e.g. "struct request_sock __rcu *fastopen_rsk" in tcp_sock.
I don't think it is possible for lsm to get a hold on tcp_sock
but agree that other similar cases could happen.

May be for now, in sleepable program, only allow safe sk ptr
to be used in helpers that take sk PTR_TO_BTF_ID argument.
e.g. sock->sk is safe in the test in patch 2.  The same should go for other
storages like inode.  This needs verifier change.

Sorry, I may be missing some context. Do you mean wait for Yonghong's work?

I don't think we have to wait.  Just saying Yonghong's work could fit
well in this use case in the future.

quoted

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
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.