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]

Message-ID: <1529065762.27158.40.camel@infradead.org>
Date:   Fri, 15 Jun 2018 13:29:22 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     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 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?

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5213 bytes)