netdev - Re: [PATCH v3 net 7/9] ipvlan: remove skb_share

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 11 Feb 2020 10:50:46 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Mahesh Bandewar <maheshb@...gle.com>,
        Florian Westphal <fw@...len.de>
Subject: Re: [PATCH v3 net 7/9] ipvlan: remove skb_share_check from xmit path



On 2/11/20 9:44 AM, Jason A. Donenfeld wrote:
> On Tue, Feb 11, 2020 at 6:39 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
>> Yes, maybe, but can you elaborate in this changelog ?
>>
>> AFAIK net/core/pktgen.c can definitely provide shared skbs.
>>
>>      refcount_inc(&pkt_dev->skb->users);
>>      ret = dev_queue_xmit(pkt_dev->skb);
>>
>> We might have to change pktgen to make sure we do not make skb shared
>> just because it was convenient.
>>
>> Please do not give a link to some web page that might disappear in the future.
>>
>> Having to follow an old thread to understand the reasoning is not appealing
>> for us having to fix bugs in the following years.
> 
> Well, I don't know really.
> 
> Florian said I should remove skb_share_check() from a function I was
> adding, because according to him, the ndo_start_xmit path cannot
> contain shared skbs. (See the 0/9 cover letter.) If this claim is
> true, then this series is correct. If this claim is not true, then the
> series needs to be adjusted.

The claim might be true for a particular driver, but not others.

ipvlan has a way to forward packets from TX to RX, and RX to TX,
I would rather not touch it unless you really can make good arguments,
and possibly some tests :)

I am worried about a missing skb_share_check() if for some
reason pskb_expand_head() has to be called later

      BUG_ON(skb_shared(skb));

> 
> I tried to trace these and couldn't totally make up my mind, hence the
> ALL CAPS mention in the 0/9.
> 
> Do you know if this is a safe presumption to make? It sounds like your
> opinion is "no" in light of pktgen.c? Should that simply be adjusted
> instead?

The key here is IFF_TX_SKB_SHARING, but really this is the intent.
I am not sure if all paths have been correctly audited/tested.

I am not saying your patch is wrong, I am unsure about it.