netdev - Re: [PATCH net-next v5 01/15] net-timestamp: add support for bpf

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+tcoDLRnsTXF-ChGh4uwM=5t3Gk8jOK_ZCARyCM+8kV9Wrag@mail.gmail.com>
Date: Wed, 15 Jan 2025 07:29:46 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, 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
Subject: Re: [PATCH net-next v5 01/15] net-timestamp: add support for bpf_setsockopt()

On Wed, Jan 15, 2025 at 7:20 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 1/12/25 3:37 AM, Jason Xing wrote:
> > Users can write the following code to enable the bpf extension:
> > bpf_setsockopt(skops, SOL_SOCKET, SK_BPF_CB_FLAGS, &flags, sizeof(flags));
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> > ---
> >   include/net/sock.h             |  7 +++++++
> >   include/uapi/linux/bpf.h       |  8 ++++++++
> >   net/core/filter.c              | 25 +++++++++++++++++++++++++
> >   tools/include/uapi/linux/bpf.h |  1 +
> >   4 files changed, 41 insertions(+)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index ccf86c8a7a8a..f5447b4b78fd 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -303,6 +303,7 @@ struct sk_filter;
> >     * @sk_stamp: time stamp of last packet received
> >     * @sk_stamp_seq: lock for accessing sk_stamp on 32 bit architectures only
> >     * @sk_tsflags: SO_TIMESTAMPING flags
> > +  *  @sk_bpf_cb_flags: used for bpf_setsockopt
> >     * @sk_use_task_frag: allow sk_page_frag() to use current->task_frag.
> >     *                    Sockets that can be used under memory reclaim should
> >     *                    set this to false.
> > @@ -445,6 +446,12 @@ struct sock {
> >       u32                     sk_reserved_mem;
> >       int                     sk_forward_alloc;
> >       u32                     sk_tsflags;
> > +#ifdef CONFIG_BPF_SYSCALL
>
> The CONFIG_BPF is used instead in the existing "u8 bpf_sock_ops_cb_flags;" in
> tcp_sock. afaik, CONFIG_BPF is selected by CONFIG_NET. It is why the test bot
> fails when CONFIG_BPF_SYSCALL is used here but not with the existing
> bpf_sock_ops_cb_flags. Considering CONFIG_BPF is also mostly useless here
> because of CONFIG_NET, I would remove this ifdef usage altogether. If there is
> really a need to distinguish CONFIG_BPF_SYSCALL is enabled or not, this can be
> improved together with the existing bpf_sock_ops_cb_flags.

Thank you, Martin. Then I will remove those ifdef related limitation.

>
> > +#define SK_BPF_CB_FLAG_TEST(SK, FLAG) ((SK)->sk_bpf_cb_flags & (FLAG))
> > +     u32                     sk_bpf_cb_flags;
> > +#else
> > +#define SK_BPF_CB_FLAG_TEST(SK, FLAG) 0
> > +#endif
> >       __cacheline_group_end(sock_write_rxtx);
> >
> >       __cacheline_group_begin(sock_write_tx);
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 4162afc6b5d0..e629e09b0b31 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -6903,6 +6903,13 @@ enum {
> >       BPF_SOCK_OPS_ALL_CB_FLAGS       = 0x7F,
> >   };
> >
> > +/* Definitions for bpf_sk_cb_flags */
> > +enum {
> > +     SK_BPF_CB_TX_TIMESTAMPING       = 1<<0,
> > +     SK_BPF_CB_MASK                  = (SK_BPF_CB_TX_TIMESTAMPING - 1) |
> > +                                        SK_BPF_CB_TX_TIMESTAMPING
> > +};
> > +
> >   /* List of known BPF sock_ops operators.
> >    * New entries can only be added at the end
> >    */
> > @@ -7081,6 +7088,7 @@ enum {
> >       TCP_BPF_SYN_IP          = 1006, /* Copy the IP[46] and TCP header */
> >       TCP_BPF_SYN_MAC         = 1007, /* Copy the MAC, IP[46], and TCP header */
> >       TCP_BPF_SOCK_OPS_CB_FLAGS = 1008, /* Get or Set TCP sock ops flags */
> > +     SK_BPF_CB_FLAGS         = 1009, /* Used to set socket bpf flags */
> >   };
> >
> >   enum {
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index b957cf57299e..c6dd2d2e44c8 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5222,6 +5222,23 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = {
> >       .arg1_type      = ARG_PTR_TO_CTX,
> >   };
> >
> > +static int sk_bpf_set_cb_flags(struct sock *sk, char *optval, bool getopt)
> > +{
> > +     u32 sk_bpf_cb_flags;
> > +
> > +     if (getopt)
>
> I may have this in my earlier sample code? This is probably because of my
> laziness for a quick example. getopt should also be supported, similar to the
> existing TCP_BPF_SOCK_OPS_CB_FLAGS.

Right, to be honest, I keep curious about this one, but I have no
clue. I will support getopt which should be easy :)

Thanks,
Jason