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]

Date:   Tue, 2 Oct 2018 15:20:48 -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 03:05 PM, Eric Dumazet wrote:
> 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.

I am not intending to pretend they are drops, just trying to make their
behavior consistent depending on the calling context, hence my question
whether this was intentional or not because __dev_kfree_skb_irq9() will
flag them as dropped correctly. Right now this is not consistent with
either the function name nor its comment in include/linux/netdevice.h.

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

Alright, thanks!
-- 
Florian