[OpenWrt-Devel] [PATCH] kernel: atm: pppoatm fix vc-mux connection failures

Paul Oranje por at oranjevos.nl
Thu Jun 21 16:27:10 EDT 2018


that's really quick, congratulations !

> Op 18 jun. 2018, om 11:08 heeft Kevin Darbyshire-Bryant via openwrt-devel <openwrt-devel at lists.openwrt.org> het volgende geschreven:
> 
> The sender domain has a DMARC Reject/Quarantine policy which disallows
> sending mailing list messages using the original "From" header.
> 
> To mitigate this problem, the original message has been wrapped
> automatically by the mailing list software.
> Van: Kevin Darbyshire-Bryant <ldir at darbyshire-bryant.me.uk>
> Onderwerp: [PATCH] kernel: atm: pppoatm fix vc-mux connection failures
> Datum: 18 juni 2018 om 11:07:41 CEST
> Aan: openwrt-devel at lists.openwrt.org
> Kopie: Kevin Darbyshire-Bryant <ldir at darbyshire-bryant.me.uk>
> 
> 
> Backport a hot off the press upstream kernel ATM fix:
> 
> Preserve value of skb->truesize when accounting to vcc
> 
> "There's a hack in pskb_expand_head() to avoid adjusting skb->truesize
> for certain skbs. Ideally it would cover ATM too. It doesn't. Just
> stashing the accounted value and using it in atm_raw_pop() is probably
> the easiest way to cope."
> 
> The issue was exposed by upstream with:
> 
> commit 14afee4b6092fde451ee17604e5f5c89da33e71e
> Author: Reshetova, Elena <elena.reshetova at intel.com>
> Date:   Fri Jun 30 13:08:00 2017 +0300
> 
>    net: convert sock.sk_wmem_alloc from atomic_t to refcount_t
> 
> But an earlier commit left the ticking timebomb:
> 
> 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()
> 
> Sincerest thanks to Mathias Kresin <dev at kresin.me> for debugging
> assistance and to David Woodhouse <dwmw2 at infradead.org> for further
> guidance, cajoling & patience in interpreting the debug I was giving him
> and producing a fix!
> 
> Fixes FS#1567
> 
> Signed-off-by: Kevin Darbyshire-Bryant <ldir at darbyshire-bryant.me.uk>
> ---
> ...-sock.sk_wmem_alloc-from-atomic_t-to-refc.patch | 172 +++++++++++++++++++++
> 1 file changed, 172 insertions(+)
> create mode 100644 target/linux/generic/backport-4.14/080-net-convert-sock.sk_wmem_alloc-from-atomic_t-to-refc.patch
> 
> diff --git a/target/linux/generic/backport-4.14/080-net-convert-sock.sk_wmem_alloc-from-atomic_t-to-refc.patch b/target/linux/generic/backport-4.14/080-net-convert-sock.sk_wmem_alloc-from-atomic_t-to-refc.patch
> new file mode 100644
> index 0000000000..06d00886f1
> --- /dev/null
> +++ b/target/linux/generic/backport-4.14/080-net-convert-sock.sk_wmem_alloc-from-atomic_t-to-refc.patch
> @@ -0,0 +1,172 @@
> +From 9bbe60a67be5a1c6f79b3c9be5003481a50529ff Mon Sep 17 00:00:00 2001
> +From: David Woodhouse <dwmw2 at infradead.org>
> +Date: Sat, 16 Jun 2018 11:55:44 +0100
> +Subject: atm: Preserve value of skb->truesize when accounting to vcc
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=UTF-8
> +Content-Transfer-Encoding: 8bit
> +
> +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: 158f323b9868 ("net: adjust skb->truesize in pskb_expand_head()")
> +Signed-off-by: David Woodhouse <dwmw2 at infradead.org>
> +Tested-by: Kevin Darbyshire-Bryant <ldir at darbyshire-bryant.me.uk>
> +Signed-off-by: David S. Miller <davem at davemloft.net>
> +---
> + 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(-)
> +
> +--- 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)
> + {
> +--- a/net/atm/br2684.c
> ++++ b/net/atm/br2684.c
> +@@ -252,8 +252,7 @@ static int br2684_xmit_vcc(struct sk_buf
> + 
> + 	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;
> + 
> +--- a/net/atm/clip.c
> ++++ b/net/atm/clip.c
> +@@ -381,8 +381,7 @@ static netdev_tx_t clip_start_xmit(struc
> + 		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 ... */
> +--- a/net/atm/common.c
> ++++ b/net/atm/common.c
> +@@ -630,10 +630,9 @@ int vcc_sendmsg(struct socket *sock, str
> + 		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;
> +--- a/net/atm/lec.c
> ++++ b/net/atm/lec.c
> +@@ -182,9 +182,8 @@ lec_send(struct atm_vcc *vcc, struct sk_
> + 	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;
> +--- a/net/atm/mpc.c
> ++++ b/net/atm/mpc.c
> +@@ -555,8 +555,7 @@ static int send_via_shortcut(struct sk_b
> + 					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);
> +--- a/net/atm/pppoatm.c
> ++++ b/net/atm/pppoatm.c
> +@@ -350,8 +350,7 @@ static int pppoatm_send(struct ppp_chann
> + 		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)
> +--- a/net/atm/raw.c
> ++++ b/net/atm/raw.c
> +@@ -35,8 +35,8 @@ static void atm_pop_raw(struct atm_vcc *
> + 	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.15.2 (Apple Git-101.1)
> 
> 
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/listinfo/openwrt-devel


_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/listinfo/openwrt-devel


More information about the openwrt-devel mailing list