From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH] atm: Preserve value of skb->truesize when accounting to vcc Date: Sat, 16 Jun 2018 05:57:40 -0700 Message-ID: <62fbc51a-4738-6c21-8c53-d84fee7a9bbd@gmail.com> References: <20180616105544.246714-1-dwmw2@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit To: David Woodhouse , netdev@vger.kernel.org, ldir@darbyshire-bryant.me.uk Return-path: Received: from mail-pg0-f67.google.com ([74.125.83.67]:43599 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932779AbeFPM5n (ORCPT ); Sat, 16 Jun 2018 08:57:43 -0400 Received: by mail-pg0-f67.google.com with SMTP id a14-v6so5563407pgw.10 for ; Sat, 16 Jun 2018 05:57:43 -0700 (PDT) In-Reply-To: <20180616105544.246714-1-dwmw2@infradead.org> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/16/2018 03:55 AM, David Woodhouse wrote: > ATM accounts for in-flight TX packets in sk_wmem_alloc of the VCC on > which they are to be sent. But it doesn't take ownership of those > packets from the sock (if any) which originally owned them. They should > remain owned by their actual sender until they've left the box. > > There's a hack in pskb_expand_head() to avoid adjusting skb->truesize > for certain skbs, precisely to avoid messing up sk_wmem_alloc > accounting. Ideally that hack would cover the ATM use case too, but it > doesn't — skbs which aren't owned by any sock, for example PPP control > frames, still get their truesize adjusted when the low-level ATM driver > adds headroom. > > This has always been an issue, it seems. The truesize of a packet > increases, and sk_wmem_alloc on the VCC goes negative. But this wasn't > for normal traffic, only for control frames. So I think we just got away > with it, and we probably needed to send 2GiB of LCP echo frames before > the misaccounting would ever have caused a problem and caused > atm_may_send() to start refusing packets. > > Commit 14afee4b609 ("net: convert sock.sk_wmem_alloc from atomic_t to > refcount_t") did exactly what it was intended to do, and turned this > mostly-theoretical problem into a real one, causing PPPoATM to fail > immediately as sk_wmem_alloc underflows and atm_may_send() *immediately* > starts refusing to allow new packets. > > The least intrusive solution to this problem is to stash the value of > skb->truesize that was accounted to the VCC, in a new member of the > ATM_SKB(skb) structure. Then in atm_pop_raw() subtract precisely that > value instead of the then-current value of skb->truesize. > > Fixes: 14afee4b ("net: convert sock.sk_wmem_alloc from atomic_t to refcount_t") This Fixes tag shoots the messenger really. I suggest to instead use : Fixes: 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()") Because even without the conversion to refcount_t, we could have a LOCKDEP splat in : filter = rcu_dereference_check(sk->sk_filter, atomic_read(&sk->sk_wmem_alloc) == 0); Note that some places make a further check even when LOCKDEP is not used. net/ipv4/af_inet.c:154: WARN_ON(refcount_read(&sk->sk_wmem_alloc)); net/iucv/af_iucv.c:405: WARN_ON(refcount_read(&sk->sk_wmem_alloc)); net/key/af_key.c:112: WARN_ON(refcount_read(&sk->sk_wmem_alloc)); net/netlink/af_netlink.c:410: WARN_ON(refcount_read(&sk->sk_wmem_alloc)); net/packet/af_packet.c:1286: WARN_ON(refcount_read(&sk->sk_wmem_alloc)); net/rxrpc/af_rxrpc.c:852: WARN_ON(refcount_read(&sk->sk_wmem_alloc)); net/unix/af_unix.c:490: WARN_ON(refcount_read(&sk->sk_wmem_alloc)); We might factorize these checks into __sk_destruct()