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

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

[David Woodhouse]

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")
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Tested-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 include/linux/atmdev.h | 15 +++++++++++++++
 net/atm/br2684.c       |  3 +--
 net/atm/clip.c         |  3 +--
 net/atm/common.c       |  3 +--
 net/atm/lec.c          |  3 +--
 net/atm/mpc.c          |  3 +--
 net/atm/pppoatm.c      |  3 +--
 net/atm/raw.c          |  4 ++--
 8 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 0c27515d2cf6..8124815eb121 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -214,6 +214,7 @@ struct atmphy_ops {
 struct atm_skb_data {
 	struct atm_vcc	*vcc;		/* ATM VCC */
 	unsigned long	atm_options;	/* ATM layer options */
+	unsigned int	acct_truesize;  /* truesize accounted to vcc */
 };
 
 #define VCC_HTABLE_SIZE 32
@@ -241,6 +242,20 @@ void vcc_insert_socket(struct sock *sk);
 
 void atm_dev_release_vccs(struct atm_dev *dev);
 
+static inline void atm_account_tx(struct atm_vcc *vcc, struct sk_buff *skb)
+{
+	/*
+	 * Because ATM skbs may not belong to a sock (and we don't
+	 * necessarily want to), skb->truesize may be adjusted,
+	 * escaping the hack in pskb_expand_head() which avoids
+	 * doing so for some cases. So stash the value of truesize
+	 * at the time we accounted it, and atm_pop_raw() can use
+	 * that value later, in case it changes.
+	 */
+	refcount_add(skb->truesize, &sk_atm(vcc)->sk_wmem_alloc);
+	ATM_SKB(skb)->acct_truesize = skb->truesize;
+	ATM_SKB(skb)->atm_options = vcc->atm_options;
+}
 
 static inline void atm_force_charge(struct atm_vcc *vcc,int truesize)
 {
diff --git a/net/atm/br2684.c b/net/atm/br2684.c
index 36b3adacc0dd..10462de734ea 100644
--- a/net/atm/br2684.c
+++ b/net/atm/br2684.c
@@ -252,8 +252,7 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct net_device *dev,
 
 	ATM_SKB(skb)->vcc = atmvcc = brvcc->atmvcc;
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, atmvcc->dev);
-	refcount_add(skb->truesize, &sk_atm(atmvcc)->sk_wmem_alloc);
-	ATM_SKB(skb)->atm_options = atmvcc->atm_options;
+	atm_account_tx(atmvcc, skb);
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;
 
diff --git a/net/atm/clip.c b/net/atm/clip.c
index 66caa48a27c2..d795b9c5aea4 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -381,8 +381,7 @@ static netdev_tx_t clip_start_xmit(struct sk_buff *skb,
 		memcpy(here, llc_oui, sizeof(llc_oui));
 		((__be16 *) here)[3] = skb->protocol;
 	}
-	refcount_add(skb->truesize, &sk_atm(vcc)->sk_wmem_alloc);
-	ATM_SKB(skb)->atm_options = vcc->atm_options;
+	atm_account_tx(vcc, skb);
 	entry->vccs->last_use = jiffies;
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, vcc, vcc->dev);
 	old = xchg(&entry->vccs->xoff, 1);	/* assume XOFF ... */
diff --git a/net/atm/common.c b/net/atm/common.c
index 1f2af59935db..ff5748b2190f 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -630,10 +630,9 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
 		goto out;
 	}
 	pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
-	refcount_add(skb->truesize, &sk->sk_wmem_alloc);
+	atm_account_tx(vcc, skb);
 
 	skb->dev = NULL; /* for paths shared with net_device interfaces */
-	ATM_SKB(skb)->atm_options = vcc->atm_options;
 	if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
 		kfree_skb(skb);
 		error = -EFAULT;
diff --git a/net/atm/lec.c b/net/atm/lec.c
index 5a95fcf6f9b6..d7f5cf5b7594 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -182,9 +182,8 @@ lec_send(struct atm_vcc *vcc, struct sk_buff *skb)
 	struct net_device *dev = skb->dev;
 
 	ATM_SKB(skb)->vcc = vcc;
-	ATM_SKB(skb)->atm_options = vcc->atm_options;
+	atm_account_tx(vcc, skb);
 
-	refcount_add(skb->truesize, &sk_atm(vcc)->sk_wmem_alloc);
 	if (vcc->send(vcc, skb) < 0) {
 		dev->stats.tx_dropped++;
 		return;
diff --git a/net/atm/mpc.c b/net/atm/mpc.c
index 75620c2f2617..24b53c4c39c6 100644
--- a/net/atm/mpc.c
+++ b/net/atm/mpc.c
@@ -555,8 +555,7 @@ static int send_via_shortcut(struct sk_buff *skb, struct mpoa_client *mpc)
 					sizeof(struct llc_snap_hdr));
 	}
 
-	refcount_add(skb->truesize, &sk_atm(entry->shortcut)->sk_wmem_alloc);
-	ATM_SKB(skb)->atm_options = entry->shortcut->atm_options;
+	atm_account_tx(entry->shortcut, skb);
 	entry->shortcut->send(entry->shortcut, skb);
 	entry->packets_fwded++;
 	mpc->in_ops->put(entry);
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 21d9d341a619..af8c4b38b746 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -350,8 +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);
-	ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
+	atm_account_tx(vcc, skb);
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
 		 skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);
 	ret = ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb)
diff --git a/net/atm/raw.c b/net/atm/raw.c
index ee10e8d46185..b3ba44aab0ee 100644
--- a/net/atm/raw.c
+++ b/net/atm/raw.c
@@ -35,8 +35,8 @@ static void atm_pop_raw(struct atm_vcc *vcc, struct sk_buff *skb)
 	struct sock *sk = sk_atm(vcc);
 
 	pr_debug("(%d) %d -= %d\n",
-		 vcc->vci, sk_wmem_alloc_get(sk), skb->truesize);
-	WARN_ON(refcount_sub_and_test(skb->truesize, &sk->sk_wmem_alloc));
+		 vcc->vci, sk_wmem_alloc_get(sk), ATM_SKB(skb)->acct_truesize);
+	WARN_ON(refcount_sub_and_test(ATM_SKB(skb)->acct_truesize, &sk->sk_wmem_alloc));
 	dev_kfree_skb_any(skb);
 	sk->sk_write_space(sk);
 }
-- 
2.17.0

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

[Eric Dumazet]

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

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

[David Woodhouse]

>> 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.

 ...

>> Fixes: 14afee4b ("net: convert sock.sk_wmem_alloc from atomic_t to
>> refcount_t")
>
> This Fixes tag shoots the messenger really.

A little bit, yes. The text hopefully made that clear.

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


How many of those were likely to trigger in practice on an ATM VCC though?
If we are taking the Fixes: tag as a hint about which stable kernels we
might want to backport to, rather than a moral assignment of blame, then
14afee4b is probably not the worst place to point it. But I don't mind
much...

-- 
dwmw2

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

[David Woodhouse]

> This Fixes tag shoots the messenger really.
>
> I suggest to instead use :
>
> Fixes: 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()")


Oh, I hadn't realised how recent that was. Sure, let's blame you instead :)


-- 
dwmw2

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

[David Miller]

From: "David Woodhouse" <dwmw2@infradead.org>
Date: Sat, 16 Jun 2018 20:52:33 -0000

>> This Fixes tag shoots the messenger really.
>>
>> I suggest to instead use :
>>
>> Fixes: 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()")
> 
> 
> Oh, I hadn't realised how recent that was. Sure, let's blame you instead :)

Patch applied with adjusted Fixes: tag, and queued up for -stable.

Thanks.

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

On Sat, 2018-06-16 at 16:27 -0700, David Miller wrote:
> From: "David Woodhouse" <dwmw2@infradead.org>
> Date: Sat, 16 Jun 2018 20:52:33 -0000
> 
> >> This Fixes tag shoots the messenger really.
> >>
> >> I suggest to instead use :
> >>
> >> Fixes: 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()")
> > 
> > 
> > Oh, I hadn't realised how recent that was. Sure, let's blame you instead :)
> 
> Patch applied with adjusted Fixes: tag, and queued up for -stable.

Thanks.... gentle prod about the "stable" part of that. OpenWRT is
lining up for a release it'd be good to ingest the patch properly if
possible.

I periodically whine at them about the number of outstanding patches
not in upstream. It helps if one of them doesn't have my name on :)

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

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

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 05 Jul 2018 09:44:22 +0100

> On Sat, 2018-06-16 at 16:27 -0700, David Miller wrote:
>> From: "David Woodhouse" <dwmw2@infradead.org>
>> Date: Sat, 16 Jun 2018 20:52:33 -0000
>> 
>> >> This Fixes tag shoots the messenger really.
>> >>
>> >> I suggest to instead use :
>> >>
>> >> Fixes: 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()")
>> > 
>> > 
>> > Oh, I hadn't realised how recent that was. Sure, let's blame you instead :)
>> 
>> Patch applied with adjusted Fixes: tag, and queued up for -stable.
> 
> Thanks.... gentle prod about the "stable" part of that. OpenWRT is
> lining up for a release it'd be good to ingest the patch properly if
> possible.
> 
> I periodically whine at them about the number of outstanding patches
> not in upstream. It helps if one of them doesn't have my name on :)

It's in my next batch of -stable submissions. :)