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:27:46 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     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 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 :

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 21d9d341a6199255a017437954e4b688f1ba5bfd..4863d02939a26ce0a5816da86779336255d550bd 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -350,7 +350,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
                return 1;
        }
 
-       refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
+       skb_set_owner_w(skb, &sk_atm(ATM_SKB(skb)->vcc));
        ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
        pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
                 skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);

Or something more sophisticated, but this might need more thinking

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 21d9d341a6199255a017437954e4b688f1ba5bfd..80902a0b56a1d15d2851a07f067490d99136d214 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -350,7 +350,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
                return 1;
        }
 
-       refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
+       if (!skb->sk)
+               skb_set_owner_w(skb, &sk_atm(ATM_SKB(skb)->vcc));
+-      else
+               refcount_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
        ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
        pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
                 skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);