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: <b6707190-d5f6-32cc-1c83-095dc1c5c118@gmail.com>
Date:   Tue, 2 Oct 2018 14:54:05 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Eric Dumazet <edumazet@...gle.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 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?

> 
> (dev_kfree_skb_any() calls were added way before the drop monitor stuff)
> 


-- 
Florian