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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHmME9p_mz0oxwa6DuWom6Ffmes-JK1b9f9WXSeKn_-PFTY0WA@mail.gmail.com>
Date:   Tue, 11 Feb 2020 19:54:01 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Eric Dumazet <eric.dumazet@...il.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 Tue, Feb 11, 2020 at 7:50 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
>
>
>
> 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.

Thanks for the analysis here. It seems like removal of skb_share_check
in a blanket manner is a project in its own. I'll rework v4 of this
patch set to take the conservative set of choices I had originally, of
assuming this can happen and so the helper function needs to be robust
for it. Later we can revisit skb sharing on the tx path as a whole
situation probably best suited for a net-next series.

Jason