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

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

On 2026-02-17 15:48:11 [+0800], Jason Xing wrote:
> Hi Sebastian,
Hi,

> > 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.
> 
> If skb still exists, there is no way to do the real destruction of its
> socket, at least I know it works for TCP. That means IIUC TCP doesn't
> have such an issue?

There is struct sock and struct socket. socket will be removed once the
fd goes. sock remains until the last skb is gone.

> I roughly know what you meant here, but still wondering if you can
> provide a specific call trace or case where the socket using socket
> timestamping feature leads to this NULL-pointer problem?

So assuming we delay the timestamp delivery and as soon as ptp4l
complains, we kill it. Then we see this on ptp4l's side:
| ptp4l[89.480]: port 1 (enp4s0): delay timeout
| ptp4l[89.490]: timed out while polling for tx timestamp
| ptp4l[89.490]: increasing tx_timestamp_timeout or increasing kworker priority may correct this issue, but a driver bug likely causes it
|CTRL-C

and delay delivery in igc:

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 89a321a344d2..5bfd4a442b16 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1546,6 +1546,7 @@ static bool igc_request_tx_tstamp(struct igc_adapter *adapter, struct sk_buff *s
 		if (tstamp->skb)
 			continue;
 
+		trace_printk("skb %px dest %pS\n", skb, skb->destructor);
 		tstamp->skb = skb_get(skb);
 		tstamp->start = jiffies;
 		*flags = tstamp->flags;
@@ -5563,6 +5564,21 @@ igc_features_check(struct sk_buff *skb, struct net_device *dev,
 	return features;
 }
 
+struct delayed_ptp {
+	struct delayed_work dw;
+	struct igc_adapter *adapter;
+};
+
+static void delayed_ts(struct work_struct *work)
+{
+	struct delayed_ptp *dtp = container_of(work, struct delayed_ptp, dw.work);
+
+	/* retrieve hardware timestamp */
+	trace_printk("\n");
+	igc_ptp_tx_tstamp_event(dtp->adapter);
+	kfree(dtp);
+}
+
 static void igc_tsync_interrupt(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
@@ -5579,8 +5595,15 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
 	}
 
 	if (tsicr & IGC_TSICR_TXTS) {
-		/* retrieve hardware timestamp */
-		igc_ptp_tx_tstamp_event(adapter);
+		struct delayed_ptp *dtp;
+
+		dtp = kmalloc(sizeof(*dtp), GFP_ATOMIC);
+		if (dtp) {
+			INIT_DELAYED_WORK(&dtp->dw, delayed_ts);
+			dtp->adapter = adapter;
+			trace_printk("\n");
+			schedule_delayed_work(&dtp->dw, HZ * 10);
+		}
 	}
 
 	if (tsicr & IGC_TSICR_TT0) {

and few more in net/, I end up with:

|          ptp4l-745     [005] .....    88.609323: packet_create: sock ffff8ba689e28680 sk ffff8ba6822f8800
|          ptp4l-745     [005] .....    88.609325: sock_alloc_file: sock ffff8ba689e28680 sk ffff8ba6822f8800 file ffff8ba689575d80
socket allocation

|         <idle>-0       [004] dNh1.    89.645405: igc_tsync_interrupt:
TX TS delayed

|          ptp4l-745     [005] .....    89.655483: __sock_release: packet_release+0x0/0x490 sock ffff8ba689e28680, file is NULL
|          ptp4l-745     [005] .....    89.655484: packet_release: sock ffff8ba689e28680 sk ffff8ba6822f8800
|          ptp4l-745     [005] .....    89.669111: packet_release: gone ffff8ba689e28680

ptp4l closed socket, sock->sk = sk->sk_socket = NULL.

|    kworker/0:1-11      [000] .....    99.769061: delayed_ts:
The worked kicked in

|    kworker/0:1-11      [000] d..1.    99.769068: __skb_tstamp_tx: sk ffff8ba6822f8800 socket 0000000000000000
the NULL socket.

|    kworker/0:1-11      [000] d..1.    99.769070: __skb_complete_tx_timestamp: added to ffff8ba6822f8800
|    ksoftirqd/0-14      [000] ..s..    99.769081: __sk_destruct: sk ffff8ba6822f8800
|    ksoftirqd/0-14      [000] ..s..    99.769082: packet_sock_destruct: sk ffff8ba6822f8800

This is the RCU free of sock after the skb was released.

Does this make sense?

> Thanks,
> Jason

Sebastian