Re: [PATCH net] net: Drop the lock in skb_may_tx_timestamp()

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Vadim Fedorenko wrote:
> On 15/02/2026 18:46, Sebastian Andrzej Siewior wrote:
> > On 2026-02-14 23:05:27 [-0500], Willem de Bruijn wrote:
> >>> --- a/net/core/skbuff.c
> >>> +++ b/net/core/skbuff.c
> >>> @@ -5555,16 +5555,25 @@ static void __skb_complete_tx_timestamp(struct sk_buff *skb,
> >>>   
> >>>   static bool skb_may_tx_timestamp(struct sock *sk, bool tsonly)
> >>>   {
> >>> -	bool ret;
> >>> +	struct socket *sock;
> >>> +	struct file *file;
> >>>   
> >>>   	if (likely(tsonly || READ_ONCE(sock_net(sk)->core.sysctl_tstamp_allow_data)))
> >>>   		return true;
> >>>   
> >>> -	read_lock_bh(&sk->sk_callback_lock);
> >>> -	ret = sk->sk_socket && sk->sk_socket->file &&
> >>> -	      file_ns_capable(sk->sk_socket->file, &init_user_ns, CAP_NET_RAW);
> >>> -	read_unlock_bh(&sk->sk_callback_lock);
> >>> -	return ret;
> >>> +	/* The sk pointer remains valid as long as the skb is. The sk_socket and
> >>> +	 * file pointer may become NULL if the socket is closed. Both structures
> >>> +	 * (including file->cred) are RCU freed which means they can be accessed
> >>> +	 * within a RCU read section.
> >>
> >> The fields are not __rcu annotated. Is this common knowledge or is
> >> there clear documentation of this behavior? Not disputing it, just not
> >> familiar with the rules around these fields.
> > 
> > The pointer is not RCU annotated because normally, within a syscall, the
> > file descriptor does not vanish and so the pointer remains valid. The
> > interrupt callback is an exception because we don't hold a reference on
> > the fd. But since the underlying data structures have a RCU lifetime we
> > could use it.
> > 
> > sock_alloc() allocates a struct socket_alloc which contains the struct
> > socket and struct inode (sock_alloc_inode()). This the sk_socket in
> > struct sock. Once you close the socket via the close syscall and the
> > last reference is gone then you end up in __fput(). The invoked
> > ->release() callback is sock_close(). Here socket::file gets NULLed and
> > proto::release should sock_orphan() the sock where sock::sk_socket gets
> > NULLed (I don't see where else and packet_release() does so). The
> > "struct sock" remains around because the skb has a reference on.
> > Otherwise it would go, too.
> > 
> > Once that is done __fput() does dput() -> __dentry_kill() -> iput() ->
> > destroy_inode() which does RCU free the inode.
> > That means if we observe the pointer non-NULL we can use it under the
> > RCU rules because the inode is released via RCU.
> > 
> > ->file. At the end of __fput() there is file_free() which releases the
> > pointer. file goes to filp_cachep which is SLAB_TYPESAFE_BY_RCU. There
> > is a put_cred for file::f_cred before that and it does call_rcu() at the
> > end, too.
> > 
> > This is me reading the code on FRI/ SAT and double checking it with
> > ptp4l. I might have missed something but so far, it makes sense.
> > 
> >> It does seem that updates to sk_socket, in sock_orphan and
> >> sock_graft, are protected by sk_callback_lock.
> >>> +	 */
> >>> +	lockdep_assert_in_rcu_reader();

It will be great to have this additional context in the commit
message.

And a Link: to the original discussion thread.

Btw, the commit states that a few drivers already call this from their
hard interrupt handler. Can you point to one or two?

If this is only for new drivers, it would be vastly preferable if this
can go through net-next.

> >>
> >> This function can be called from process context outside an RCU read
> >> side section, I think.
> >>
> >> Such as a packet socket sending and hitting the __skb_tstamp_tx
> >> SCM_TSTAMP_SCHED at the top of __dev_queue_xmit.
> > 
> > If that is the case, then we could create a RCU read section here.
> 
> well, __dev_queue_xmit() grabs rcu_read_lock_bh() already, we just have
> to move it right before timestamping check:

True. That was just the first example I found though.

I did not check tcp_ack_tstamp, or whether any drivers defer the
operation to a process context kthread without RCU protection.