netdev - Re: [PATCH bpf-next v12 01/12] bpf: add networking timestamping support to 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: <67b697697bdf8_20efb0294b4@willemb.c.googlers.com.notmuch>
Date: Wed, 19 Feb 2025 21:46:01 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Martin KaFai Lau <martin.lau@...ux.dev>, 
 davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 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, 
 shuah@...nel.org, 
 ykolal@...com, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v12 01/12] bpf: add networking timestamping
 support to bpf_get/setsockopt()

> > Can you find a hole further down to place this in, or at least a spot
> > that does not result in 7b of wasted space (in the hotpath cacheline
> > groups of all places).
> 
> There is one place where I can simply insert the flag.
> 
> The diff patch on top of this series is:
> diff --git a/include/net/sock.h b/include/net/sock.h
> index e85d6fb3a2ba..9fa27693fb02 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -446,8 +446,6 @@ struct sock {
>         u32                     sk_reserved_mem;
>         int                     sk_forward_alloc;
>         u32                     sk_tsflags;
> -#define SK_BPF_CB_FLAG_TEST(SK, FLAG) ((SK)->sk_bpf_cb_flags & (FLAG))
> -       u8                      sk_bpf_cb_flags;
>         __cacheline_group_end(sock_write_rxtx);
> 
>         __cacheline_group_begin(sock_write_tx);
> @@ -528,6 +526,8 @@ struct sock {
>         u8                      sk_txtime_deadline_mode : 1,
>                                 sk_txtime_report_errors : 1,
>                                 sk_txtime_unused : 6;
> +#define SK_BPF_CB_FLAG_TEST(SK, FLAG) ((SK)->sk_bpf_cb_flags & (FLAG))
> +       u8                      sk_bpf_cb_flags;
> 
>         void                    *sk_user_data;
>  #ifdef CONFIG_SECURITY
> 
> 
> 1) before applying the whole series:
> ...
>         /* --- cacheline 10 boundary (640 bytes) --- */
>         ktime_t                    sk_stamp;             /* 0x280   0x8 */
>         int                        sk_disconnects;       /* 0x288   0x4 */
>         u8                         sk_txrehash;          /* 0x28c   0x1 */
>         u8                         sk_clockid;           /* 0x28d   0x1 */
>         u8                         sk_txtime_deadline_mode:1; /* 0x28e: 0 0x1 */
>         u8                         sk_txtime_report_errors:1; /*
> 0x28e:0x1 0x1 */
>         u8                         sk_txtime_unused:6;   /* 0x28e:0x2 0x1 */
> 
>         /* XXX 1 byte hole, try to pack */
> 
>         void *                     sk_user_data;         /* 0x290   0x8 */
>         void *                     sk_security;          /* 0x298   0x8 */
>         struct sock_cgroup_data    sk_cgrp_data;         /* 0x2a0  0x10 */
> ...
> /* sum members: 773, holes: 1, sum holes: 1 */
> 
> 
> 2) after applying the series with the above diff patch:
> ...
>         /* --- cacheline 10 boundary (640 bytes) --- */
>         ktime_t                    sk_stamp;             /* 0x280   0x8 */
>         int                        sk_disconnects;       /* 0x288   0x4 */
>         u8                         sk_txrehash;          /* 0x28c   0x1 */
>         u8                         sk_clockid;           /* 0x28d   0x1 */
>         u8                         sk_txtime_deadline_mode:1; /* 0x28e: 0 0x1 */
>         u8                         sk_txtime_report_errors:1; /*
> 0x28e:0x1 0x1 */
>         u8                         sk_txtime_unused:6;   /* 0x28e:0x2 0x1 */
>         u8                         sk_bpf_cb_flags;      /* 0x28f   0x1 */
>         void *                     sk_user_data;         /* 0x290
> 0x8 */
>         void *                     sk_security;          /* 0x298   0x8 */
>         struct sock_cgroup_data    sk_cgrp_data;         /* 0x2a0  0x10 */
> ...
> /* sum members: 774 */
> 
> It turns out that the new sk_bpf_cb_flags fills the hole exactly. The
> new field and some of its nearby fields are quite similar because they
> are only/nearly written during the creation or setsockopt phase.
> 
> I think now it's a good place to insert the new flag?

Thanks. This seems fine to me.