netdev - Re: [PATCH net-next v5 03/15] bpf: introduce timestamp_used to allow UDP socket fetched in bpf prog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoChGB3vA7LMm0VHb9OjmXHUw0--f6v4Crz5R7U+EPo+cg@mail.gmail.com>
Date: Thu, 16 Jan 2025 09:12:30 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, willemdebruijn.kernel@...il.com, 
	willemb@...gle.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, 
	eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev, 
	john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me, 
	haoluo@...gle.com, jolsa@...nel.org, horms@...nel.org, bpf@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5 03/15] bpf: introduce timestamp_used to allow
 UDP socket fetched in bpf prog

On Thu, Jan 16, 2025 at 8:51 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 1/14/25 6:54 PM, Jason Xing wrote:
> > I construct my thoughts here according to our previous discussion:
> > 1. not limiting the use of is_fullsock, so in patch 2, I will use the
> > follow codes:
> > +void bpf_skops_tx_timestamping(struct sock *sk, struct sk_buff *skb, int op)
> > +{
> > +       struct bpf_sock_ops_kern sock_ops;
> > +
> > +       memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp));
> > +       sock_ops.op = op;
> > +       sock_ops.is_fullsock = 1;
> > +       sock_ops.sk = sk;
>
> lgtm.
>
> > +       BPF_CGROUP_RUN_PROG_SOCK_OPS(sk, &sock_ops, CGROUP_SOCK_OPS);
>
> After looking through the set and looking again at how sk is used in
> __skb_tstamp_tx(), I think the sk must be fullsock here, so using
> __cgroup_bpf_run_filter_sock_ops() as in patch 2 is good. It will be useful to
> have a comment here to explain it must be a fullsock.

Got it, will add more comments on it.

>
> > +}
> >
> > 2. introduce the allow_direct_access flag which is used to test if the
> > socket is allowed to access tcp socket or not.
>
> yeah, right now is only tcp_sock, but future will have UDP TS support.
>
> May be the "allow_direct_access" naming is not obvious to mean the existing
> tcp_sock support. May be "allow_tcp_access"?

I like this name :)

>
> I was thinking to set the allow_direct_access for the "existing" sockops
> callback which must be tcp_sock and must have the sk locked.
>
> > On the basis of the above bpf_skops_tx_timestamping() function, I
> > would add one check there:
> > + if (sk_is_tcp(sk))
> > +       sock_ops. allow_direct_access = 1;
>
> so don't set this in the new TS callback from bpf_skops_tx_timestamping
> regardless it is tcp or not.
>
> >
> > Also, I need to set allow_direct_access to one as long as there is
> > "sock_ops.is_fullsock = 1;" in the existing callbacks.
>
> Only set allow_direct_access when the sk is fullsock in the "existing" sockops
> callback.

Only "existing"? Then how can the bpf program access those members of
the tcp socket structure in the current/new timestamping callbacks?

>
> After thinking a bit more today, I think this should work. Please give it a try
> and check if some cases may be missed in sock_ops_convert_ctx_access().

I will give it a shot this week.

>
> >
> > 3. I will replace is_fullsock with allow_direct_access in
> > SOCK_OPS_GET/SET_FIELD() instead of SOCK_OPS_GET_SK().
>
> Yep.
>
> >
> > Then the udp socket can freely access the socket with the helper
> > SOCK_OPS_GET_SK() because it is a fullsock. And udp socket cannot
> > access struct tcp_sock because in the timestamping callback, there is
> > no place where setting allow_direct_access for udp use.
>
> __sk_buff->sk? yes.