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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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();
> 
> 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.

> > +	sock = READ_ONCE(sk->sk_socket);
> > +	if (!sock)
> > +		return false;
> > +	file = READ_ONCE(sock->file);
> > +	if (!file)
> > +		return false;
> > +	return file_ns_capable(file, &init_user_ns, CAP_NET_RAW);
> >  }
> >  
> >  void skb_complete_tx_timestamp(struct sk_buff *skb,

Sebastian