netdev - Re: __dev_kfree_skb_any() and use of dev_kfree

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]

Message-ID: <CANn89iJsZoVaMZGOXkcks04amXoSfL-_MQXxDE3_2iyA4cX8+w@mail.gmail.com>
Date:   Tue, 2 Oct 2018 15:05:23 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>,
        David Miller <davem@...emloft.net>
Subject: Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

On Tue, Oct 2, 2018 at 2:54 PM Florian Fainelli <f.fainelli@...il.com> wrote:
>
> On 10/02/2018 02:17 PM, Eric Dumazet wrote:
> > On Tue, Oct 2, 2018 at 1:07 PM Florian Fainelli <f.fainelli@...il.com> wrote:
> >>
> >> Hi Eric, Neil,
> >>
> >> Should not __dev_kfree_skb_any() call kfree_skb() instead of
> >> dev_kfree_skb() which is aliased to consumes_skb() and therefore does
> >> not flag the skb with SKB_REASON_DROPPED?
> >>
> >> If we take the in_irq() || irqs_disabled() branch, we will be calling
> >> __dev_kfree_skb_irq() which takes care of setting the skb_free_reason
> >> frmo the caller.
> >>
> >> Is there an implied semantic with dev_kfree_skb() that it means it was
> >> freed by the network device and therefore this equals to a consumption
> >> (not a drop)? The comment above dev_kfree_skb_any() seems to imply this
> >> should be a context unaware replacement for kfree_skb().
> >
> >
> > Really the problem here is that we have more than one thousand calls
> > to dev_kfree_skb_any()
> > (compared to ~ 90 calls to dev_consume_skb_any())
> >
> > So it will be a huge task cleaning all this.
>
> So you are kind of saying this is an established behavior, don't change
> it :)
>
> One could argue that if people were happily sprinkling
> dev_kfree_skb_any() in error or success paths, and all SKB freeing was
> accounted for as "consumed" instead of "dropped" in non-atomic context,
> this may not be such a big deal to reverse that and make it "dropped" in
> all contexts?
>

Most of these calls happening on typical hosts are from TX completion path,
so they really are consumed, not dropped.

So if you intend to pretend they are drops, this will not please
people using drop monitor.

Really the only way would to review all call sites and perform a
cleanup, then propagate the ' reason' properly
in the helper.