Re: [PATCH net-next v8 6/8] hsr: Assign a socket for cloned skbs

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

On 2026-10-06 14:31:20 [+0000], netdev-bot+sashiko@kernel.org wrote:
> [Severity: Low]
> Could the commit message say that this only covers port-directed PTP
> frames?
> 
> hsr_create_tagged_frame() only assigns the socket when
> frame->req_tx_port != HSR_PT_NONE. hsr_dev_xmit() only moves tx_port away
> from HSR_PT_NONE for ETH_P_1588 frames that carry the inline HSR header.
> The has_foreign_header path in hsr_forward_do() also needs a port.
> 
> Other traffic that asks for TX timestamps still reaches the slaves as
> clones with skb->sk == NULL. One example is a UDP socket using
> SO_TIMESTAMPING over hsr0. For those clones, __skb_tstamp_tx() returns
> early:
…
UDP timestamping is not a thing here (in HSR).

> The restriction looks deliberate, since it avoids one timestamp per slave
> and double charging. The message, though, reads as if it applies to all
> cloned skbs.
> 
> The message also leaves out the side effects of skb_set_owner_w(). The
> clone is now charged to the sender's sk_wmem_alloc, and its skb->hash and
> ooo_okay now come from the socket.

Right. None of this should have an impact that should be mentioned
explicitly. The primary motivation is the timestamp information which
requires the socket.
The accounting for sk_wmem_alloc happens, too. The original skb is freed
early.

> > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > index 562802ddff003..46f9bd5039821 100644
> > --- a/net/hsr/hsr_forward.c
> > +++ b/net/hsr/hsr_forward.c
> 
> [ ... ]
> 
> > @@ -348,6 +349,9 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
> >  	if (!skb)
> >  		return NULL;
> >  
> > +	if (frame->req_tx_port != HSR_PT_NONE && frame->skb_std->sk)
> > +		skb_set_owner_w(skb, frame->skb_std->sk);
> > +
> >  	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
> >  		return skb;
> >  
> 
> [Severity: High]
> Does the timestamp still get lost on PRP devices?
…

Not really. PTP+PRP is a bit different compared to PTP+HSR. According to
the specification, there is no PRP header attached to PTP message (where
we would need Kernel's sequence number for the packet and so on).
Therefore for the PRP case the prp0 device isn't used but the interface
of the underlying slave device.

> [ ... ]
> 
> > @@ -581,6 +585,8 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> >  		 */
> >  		if (frame->has_foreign_header && frame->skb_std) {
> >  			skb = skb_clone(frame->skb_std, GFP_ATOMIC);
> > +			if (skb && frame->skb_std->sk)
> > +				skb_set_owner_w(skb, frame->skb_std->sk);
> >  			goto inject_into_stack;
> >  		}
> 
> [Severity: Medium]
> Is it safe to call skb_set_owner_w() here, and in
> hsr_create_tagged_frame(), without checking how the original skb holds
> skb_std->sk?
> 
> skb_set_owner_w() adds the clone's truesize to sk->sk_wmem_alloc. It does
> not take an sk_refcnt reference. That is fine when the original skb is
> owned through sock_wfree, which is the usual AF_PACKET case, because that
> charge keeps the socket alive. For comparison, skb_clone_sk() calls
> refcount_inc_not_zero() on sk_refcnt.
> 
> An egress skb can carry a socket that nothing pins, though.
> bpf_sk_assign() at TC ingress does:
> 
> 	skb->sk = sk;
> 	skb->destructor = sock_pfree;
> 
> It takes no reference for SOCK_RCU_FREE sockets. The tcx ingress redirect
> path and __bpf_tx_skb() do not orphan the skb before dev_queue_xmit().
> hsr_dev_xmit() accepts any ETH_P_1588 skb with the inline magic,
> whatever its destructor.
> 
> Suppose that socket is being closed at the same time and sk_wmem_alloc
> has already reached zero. Could skb_set_owner_w() then add to a zero
> refcount? After the clone leaves the RCU section through the slave qdisc
> or driver, could its sock_wfree() touch the freed socket?
> 
> Triggering this needs CAP_BPF or CAP_NET_ADMIN and a narrow close race,
> and the full race has not been shown in practice.
> 
> Would it make sense to transfer ownership only when
> frame->skb_std->destructor == sock_wfree, like the copy_dtor check in
> UDP GSO? ip_frag_next() calls skb_set_owner_w(skb2, skb->sk)
> unconditionally, so similar exposure exists elsewhere in the kernel.

That skb arrives from the user on that socket so this skb holds a
reference on that socket. That original skb is returned to the stack
after the skb_set_owner_w() so none of this should happen.

Sebastian