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: <ac14161a-33e9-4fa0-8e8c-1d7ea42afc56@linux.dev>
Date: Thu, 5 Sep 2024 09:34:42 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jason Xing <kerneljasonxing@...il.com>, Vadim Fedorenko <vadfed@...a.com>
Cc: 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

On 05/09/2024 09:24, Jason Xing wrote:
> 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.
> 
> [...]

That's a bit contradictory to Willem's comment about not exposing
implementation details to UAPI headers, which I think makes sense.

I will move the comment to the definition area of SOCKCM_FLAG_TS_OPT_ID
and will try to add meaningful message to BUILD_BUG_ON() to make it
easier for developers to understand the problem.

>> 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 :)

Yeah, I've fixed this one already, but thanks!

> 
>> +               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
>>