Re: [PATCH net 1/1] ip6_tunnel: use skb_cow_head() in ip6_tnl_xmit()

On 8/6/26 8:19 PM, Ido Schimmel wrote:
> On Thu, Aug 06, 2026 at 06:28:00PM +0200, Paolo Abeni wrote:
>> On 8/6/26 11:06 AM, zhilin zou wrote:
>>> On Thu, Aug 6, 2026 at 4:52 PM Ido Schimmel <[email protected]> wrote:
>>>>
>>>> On Wed, Aug 05, 2026 at 05:29:02PM -0700, Jakub Kicinski wrote:
>>>>> On Wed, 5 Aug 2026 12:07:21 +0300 Ido Schimmel wrote:
>>>>>> __gre6_xmit() and ip6erspan_tunnel_xmit() already call skb_cow_head()
>>>>>> before calling ip6_tnl_xmit().
>>>>>
>>>>> Probably just me but can't they all be buggy?
>>>>> PKTGEN likes to send shared skbs around in funny ways.
>>>>> Can we get a good explanation in the commit msg or maybe let's
>>>>> keep the check?
>>>>
>>>> ip6_tnl_xmit() is accessible via two Ethernet devices (pktgen doesn't
>>>> support other types) and they both clear IFF_TX_SKB_SHARING, so if
>>>> pktgen sends them shared skbs, I would say that it's a pktgen bug and
>>>> not a reason to block this patch. Note that pktgen is not available to
>>>> unprivileged users, so it's a less severe bug.
>>>>
>>>> The patch also makes ip6_tnl_xmit() consistent with its IPv4 counterpart
>>>> (ip_tunnel_xmit()) which is already using skb_cow_head().
>>>>
>>>> Zhiling, please add a note in the commit message that ip6gretap and
>>>> ip6erspan do not expect to be handed shared skbs given that they clear
>>>> IFF_TX_SKB_SHARING.
>>>
>>> Thanks, Ido. I'll add a note to the commit message explaining that
>>> ip6gretap and ip6erspan clear IFF_TX_SKB_SHARING and therefore do not
>>> expect shared skbs, and send a v2 with your Reviewed-by tag retained.
>>
>> I'm not sure if it's too late, but since a v2 is required, I think it
>> would be better to keep the skb_shared check: that will avoid a later
>> patch for the pktgen/shared issue that inevitably will land, possibly
>> via the security channel.
> 
> What do you mean by "keep the skb_shared check"? Return an error if the
> skb is shared before calling skb_cow_head()? How would that avoid "a
> later patch for the pktgen/shared issue" when other tunnels are already
> calling skb_cow_head() without an skb_shared() check (they clear
> IFF_TX_SKB_SHARING)?

My my concern is against possible regressions. At this late stage of the
release cycle we want to avoid them, even if there are already similar
pre-existing bugs.

AFAICS pktgen sets the per pkt_gen device F_SHARED flag unconditionally
and push shared skbs when F_SHARED is set regardless the NIC priv_flags.

What about addressing both issues in the same series? Something like
the following (completely untested) would do:
---
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index ee64f3012321..a7126d639586 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1385,6 +1385,9 @@ static ssize_t pktgen_if_write(struct file *file,
 					return -EINVAL;
 				pkt_dev->flags &= ~flag;
 			} else {
+				if (!(pkt_dev->odev->priv_flags &
+				      IFF_TX_SKB_SHARING))
+					return -EINVAL;
 				pkt_dev->flags |= flag;
 			}
 
@@ -3868,13 +3871,15 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->svlan_id = 0xffff;
 	pkt_dev->burst = 1;
 	pkt_dev->node = NUMA_NO_NODE;
-	pkt_dev->flags = F_SHARED;	/* SKB shared by default */
+	pkt_dev->flags = 0;
 
 	err = pktgen_setup_dev(t->net, pkt_dev, ifname);
 	if (err)
 		goto out1;
-	if (pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)
+	if (pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING) {
 		pkt_dev->clone_skb = pg_clone_skb_d;
+		pkt_dev->flags |= F_SHARED;
+	}
 
 	pkt_dev->entry = proc_create_data(ifname, 0600, t->net->proc_dir,
 					  &pktgen_if_proc_ops, pkt_dev);
---

/P