Re: [PATCH] atm: Preserve value of skb->truesize when accounting to vcc

[Eric Dumazet <eric.dumazet@gmail.com>]

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()