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 for Android: free password hash cracker in your pocket

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

Message-ID: <66d9b23322f2f_18ac212944a@willemb.c.googlers.com.notmuch>
Date: Thu, 05 Sep 2024 09:29:23 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Vadim Fedorenko <vadfed@...a.com>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 David Ahern <dsahern@...nel.org>, 
 Jason Xing <kerneljasonxing@...il.com>, 
 Simon Horman <horms@...nel.org>
Cc: 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

Vadim Fedorenko wrote:
> On 04/09/2024 21:54, Willem de Bruijn wrote:
> > Vadim Fedorenko wrote:
> >> SOF_TIMESTAMPING_OPT_ID socket option flag gives a way to correlate TX
> >> timestamps and packets sent via socket. Unfortunately, there is no way
> >> to reliably predict socket timestamp ID value in case of error returned
> >> by sendmsg. For UDP sockets it's impossible because of lockless
> >> nature of UDP transmit, several threads may send packets in parallel. In
> >> case of RAW sockets MSG_MORE option makes things complicated. More
> >> details are in the conversation [1].
> >> This patch adds new control message type to give user-space
> >> software an opportunity to control the mapping between packets and
> >> values by providing ID with each sendmsg for UDP sockets.
> >> The documentation is also added in this patch.
> >>
> >> [1] https://lore.kernel.org/netdev/CALCETrU0jB+kg0mhV6A8mrHfTE1D1pr1SD_B9Eaa9aDPfgHdtA@mail.gmail.com/
> >>
> >> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>

> >> @@ -1325,8 +1330,11 @@ static int ip_setup_cork(struct sock *sk, struct inet_cork *cork,
> >>   	cork->mark = ipc->sockc.mark;
> >>   	cork->priority = ipc->priority;
> >>   	cork->transmit_time = ipc->sockc.transmit_time;
> >> +	cork->ts_opt_id = ipc->sockc.ts_opt_id;
> >>   	cork->tx_flags = 0;
> >>   	sock_tx_timestamp(sk, ipc->sockc.tsflags, &cork->tx_flags);
> >> +	if (ipc->sockc.tsflags & SOCKCM_FLAG_TS_OPT_ID)
> >> +		cork->flags |= IPCORK_TS_OPT_ID;
> > 
> > We can move initialization of ts_opt_id into the branch.
> > 
> > Or conversely avoid the branch with some convoluted shift operations
> > to have the rval be either 1 << 1 or 0 << 1. But let's do the simpler
> > thing.
> 
> What is the reason to move initialization behind the flag? We are not
> doing this for transmit_time even though it's also used with flag only.
> 
> It's not a big deal to change, I just wonder what are the benefits?

Just avoid one assignment in the hot path that does not use this
feature.

cork->ts_opt_id is only valuid if cork->flags & IPCORK_TS_OPT_ID.