On Wed, 31 Jul 2024 at 15:29, Juntong Deng <juntong.deng@xxxxxxxxxxx> wrote: > > On 7/23/24 01:20, Kumar Kartikeya Dwivedi wrote: > > On Thu, 11 Jul 2024 at 13:26, Juntong Deng <juntong.deng@xxxxxxxxxxx> wrote: > >> > >> [...] > >> > >> 2. sk_write_queue in struct sock > >> sk_write_queue is a struct member in struct sock, not a pointer > >> member, so we cannot use the guaranteed valid nested pointer method > >> to get a valid pointer to sk_write_queue. > > > > I think Matt recently had a patch addressing this issue: > > https://lore.kernel.org/bpf/20240709210939.1544011-1-mattbobrowski@xxxxxxxxxx/ > > I believe that should resolve this one (as far as passing them into > > KF_TRUSTED_ARGS kfuncs is concerned atleast). > > > > Thanks for letting me know. > > I tested it and it works well in most cases, but there are a few cases > that are not fully resolved. > > Yes, the verifier has relaxed the constraint on non-zero offset > pointers, but the type of the pointer does not change. > > This means that passing non-zero offset pointers as arguments to kfuncs > with KF_ACQUIRE will be rejected by the verifier because KF_ACQUIRE > requires strict type match and casting cannot be used. > > An example, bpf_skb_peek_tail: > > # ; struct sk_buff *skb = bpf_skb_peek_tail(head); > @ test_restore_udp_socket.bpf.c:209 > > # 75: (bf) r1 = r2 ; > frame2: R1_w=ptr_sock(ref_obj_id=6,off=168) > R2=ptr_sock(ref_obj_id=6,off=168) refs=4,6 > > # 76: (85) call bpf_skb_peek_tail#101113 > # kernel function bpf_skb_peek_tail args#0 expected pointer to > STRUCT sk_buff_head but R1 has a pointer to STRUCT sock > > Should we relax the strict type-matching constraint on non-zero offset > pointers when used as arguments to kfuncs with KF_ACQUIRE? > Yes, I think it makes sense (on surface, though it requires some deliberation, can do it over the exact code). As long as we prevent special cases through existing btf_type_ids_nocast_alias, it should be ok. Please send a separate patch for this, and include selftests exercising corner cases. > > In addition, this method is not portable (such as &task->cpus_mask), > and the offset of the member will change once a new structure member > is added, or an old structure member is removed. This should be handled by BPF CO-RE. A lot of selftests would fail if this didn't work. As long as the struct is annotated as __attribute__((preserve_access_index)). > > [...]
