netdev - Re: mpls_xmit() calls skb

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Thu, 22 Feb 2024 17:34:41 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Christoph Paasch <cpaasch@...le.com>, netdev <netdev@...r.kernel.org>, 
	Roopa Prabhu <roopa@...dia.com>
Cc: Craig Taylor <cmtaylor@...le.com>
Subject: Re: mpls_xmit() calls skb_orphan()

Reviving this old thread, as I stumbled upon it again...

On Fri, 2023-12-08 at 13:06 -0800, Christoph Paasch wrote:
> we observed an issue when running a TCP-connection with BBR on top 
> of an MPLS-tunnel in that we saw a lot of CPU-time spent coming 
> from tcp_pace_kick(), although sch_fq was configured on this host.
> 
> The reason for this seems to be because mpls_xmit() calls skb_orphan(), 
> thus settings skb->sk to NULL, preventing the qdisc to set 
> sk_pacing_status (which would allow to avoid the call to tcp_pace_kick()).
> 
> The question is: Why is this call to skb_orphan in mpls_xmit necessary ?

I guess the skb_orphan() call initially landed into mpls_xmit() to
provide isolation in case such xmit would correspond to netns crossing.

We had a similar situation for most IP tunnels before commit
9c4c325252c5 ("skbuff: preserve sock reference when scrubbing the
skb."). 

According to such commit changelog the skb socket reference is handled
carefully WRT netns boundaries elsewhere in the net stack, the
orphaning on TX is not required.

TL;DR: I believe removing the mentioned skb_orphan() is safe.

Cheers,

Paolo