netdev - Re: [PATCH 07/17] net: convert sock.sk_wmem_alloc from atomic_t to refcount

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:   Fri, 15 Jun 2018 06:39:15 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Elena Reshetova <elena.reshetova@...el.com>,
        netdev@...r.kernel.org
Cc:     Krzysztof Mazur <krzysiek@...lesie.net>,
        Kevin Darbyshire-Bryant <kevin@...byshire-bryant.me.uk>,
        chas@....nrl.navy.mil, Mathias Kresin <dev@...sin.me>
Subject: Re: [PATCH 07/17] net: convert sock.sk_wmem_alloc from atomic_t to
 refcount_t



On 06/15/2018 06:27 AM, Eric Dumazet wrote:
> 
> 
> On 06/15/2018 05:29 AM, David Woodhouse wrote:
>> On Fri, 2017-06-30 at 13:08 +0300, Elena Reshetova wrote:
>>> diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
>>> index c1da539..4d97a89 100644
>>> --- a/include/linux/atmdev.h
>>> +++ b/include/linux/atmdev.h
>>> @@ -254,7 +254,7 @@ static inline void atm_return(struct atm_vcc *vcc,int truesize)
>>>  
>>>  static inline int atm_may_send(struct atm_vcc *vcc,unsigned int size)
>>>  {
>>> -       return (size + atomic_read(&sk_atm(vcc)->sk_wmem_alloc)) <
>>> +       return (size + refcount_read(&sk_atm(vcc)->sk_wmem_alloc)) <
>>>                sk_atm(vcc)->sk_sndbuf;
>>>  }
>>
>>
>> Hm, this (commit 14afee4b6092fd) may have broken PPPoATM. I did spend a
>> while staring hard at my own commit 9d02daf754238 which introduced
>> pppoatm_may_send(), but it's actually atm_may_send() which is never
>> allowing packets to be pushed.
>>
>> Debugging (which is ongoing) has so far shown that we are accounting
>> for a packet in pppoatm_send() which has skb->truesize==0x1c0, and
>> later end up in pppoatm_pop()→atm_raw_pop() with skb->truesize==0x2c0.
>>
>> This was always harmless before, but now it's a refcount_t it appears
>> to underflow and go into its "screw you" mode and never let any more
>> packets get sent.
>>
>> I'm staring hard at the special case in pskb_expand_head() to *not*
>> change skb->truesize under certain circumstances, and wondering if that
>> (is, should be) covering the case of ATM skbs:
>>
>>         /* It is not generally safe to change skb->truesize.
>>          * For the moment, we really care of rx path, or
>>          * when skb is orphaned (not attached to a socket).
>>          */
>>         if (!skb->sk || skb->destructor == sock_edemux)
>>                 skb->truesize += size - osize;
>>
>> Failing that, maybe we should copy the accounted value of skb->truesize 
>> into the struct skb_atm_data in skb->cb at the time we add it to
>> sk_wmem_alloc — and then subtract *that* value from sk_wmem_alloc in
>> atm_raw_pop() instead of the then current value of skb->truesize.
>>
>> Suggestions?
>>
> 
> Maybe ATM should make sure skb->sk is set ?
> 
> something like the following :
> 

Or simply use a new field in ATM_SKB(skb) to remember a stable truesize used in both sides (add/sub)