netdev - Re: [PATCH net-next v3 1/4] net_tstamp: add SCM_TS_OPT_ID to provide OPT

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+tcoAO=0g0mkmgODzNWLJZgRxNvJiXM7=DgoCgdbFsJ0cJEg@mail.gmail.com>
Date: Thu, 5 Sep 2024 16:24:01 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Vadim Fedorenko <vadfed@...a.com>
Cc: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Willem de Bruijn <willemb@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>, 
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 1/4] net_tstamp: add SCM_TS_OPT_ID to provide
 OPT_ID in control message

Hello Vadim,

On Wed, Sep 4, 2024 at 7:32 PM Vadim Fedorenko <vadfed@...a.com> wrote:
[...]
> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index a2c66b3d7f0f..1c38536350e7 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -38,6 +38,13 @@ enum {
>                                  SOF_TIMESTAMPING_LAST
>  };
>
> +/*
> + * The highest bit of sk_tsflags is reserved for kernel-internal
> + * SOCKCM_FLAG_TS_OPT_ID. This check is to control that SOF_TIMESTAMPING*
> + * values do not reach this reserved area

I wonder if we can add the above description which is quite useful in
enum{} like this:

diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index a2c66b3d7f0f..2314fccaf51d 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -13,7 +13,12 @@
 #include <linux/types.h>
 #include <linux/socket.h>   /* for SO_TIMESTAMPING */

-/* SO_TIMESTAMPING flags */
+/* SO_TIMESTAMPING flags
+ *
+ * The highest bit of sk_tsflags is reserved for kernel-internal
+ * SOCKCM_FLAG_TS_OPT_ID.
+ * SOCKCM_FLAG_TS_OPT_ID = (1 << 31),
+ */
 enum {
        SOF_TIMESTAMPING_TX_HARDWARE = (1<<0),
        SOF_TIMESTAMPING_TX_SOFTWARE = (1<<1),

to explicitly remind the developers not to touch 1<<31 field. Or else,
it can be very hard to trace who occupied the highest field in the
future at the first glance, I think.

[...]
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index f26841f1490f..9b87d23314e8 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1401,7 +1401,10 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
>         cork->base.gso_size = ipc6->gso_size;
>         cork->base.tx_flags = 0;
>         cork->base.mark = ipc6->sockc.mark;
> +       cork->base.ts_opt_id = ipc6->sockc.ts_opt_id;
>         sock_tx_timestamp(sk, ipc6->sockc.tsflags, &cork->base.tx_flags);
> +       if (ipc6->sockc.tsflags & SOCKCM_FLAG_TS_OPT_ID)
> +               cork->base.flags |= IPCORK_TS_OPT_ID;
>
>         cork->base.length = 0;
>         cork->base.transmit_time = ipc6->sockc.transmit_time;
> @@ -1433,7 +1436,7 @@ static int __ip6_append_data(struct sock *sk,
>         bool zc = false;
>         u32 tskey = 0;
>         struct rt6_info *rt = dst_rt6_info(cork->dst);
> -       bool paged, hold_tskey, extra_uref = false;
> +       bool paged, hold_tskey = false, extra_uref = false;
>         struct ipv6_txoptions *opt = v6_cork->opt;
>         int csummode = CHECKSUM_NONE;
>         unsigned int maxnonfragsize, headersize;
> @@ -1543,10 +1546,15 @@ static int __ip6_append_data(struct sock *sk,
>                         flags &= ~MSG_SPLICE_PAGES;
>         }
>
> -       hold_tskey = cork->tx_flags & SKBTX_ANY_TSTAMP &&
> -                    READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_OPT_ID;
> -       if (hold_tskey)
> -               tskey = atomic_inc_return(&sk->sk_tskey) - 1;
> +       if (cork->tx_flags & SKBTX_ANY_TSTAMP &&
> +           READ_ONCE(sk->sk_tsflags) & SOCKCM_FLAG_TS_OPT_ID) {

s/SOCKCM_FLAG_TS_OPT_ID/SOF_TIMESTAMPING_OPT_ID/
In case you forget to change here :)

> +               if (cork->flags & IPCORK_TS_OPT_ID) {
> +                       tskey = cork->ts_opt_id;
> +               } else {
> +                       tskey = atomic_inc_return(&sk->sk_tskey) - 1;
> +                       hold_tskey = true;
> +               }
> +       }
>
>         /*
>          * Let's try using as much space as possible.
> --
> 2.43.5
>