| Message ID | mailman.4968.1529312884.25356.openwrt-devel@lists.openwrt.org |
|---|---|
| State | Accepted |
| Delegated to: | John Crispin |
| Headers | show |
| Series | [OpenWrt-Devel] kernel: atm: pppoatm fix vc-mux connection failures | expand |
that's really quick, congratulations ! > Op 18 jun. 2018, om 11:08 heeft Kevin Darbyshire-Bryant via openwrt-devel <openwrt-devel@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@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@lists.openwrt.org > Kopie: Kevin Darbyshire-Bryant <ldir@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@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@kresin.me> for debugging > assistance and to David Woodhouse <dwmw2@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@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@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@infradead.org> > +Tested-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> > +Signed-off-by: David S. Miller <davem@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@lists.openwrt.org > https://lists.openwrt.org/listinfo/openwrt-devel
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@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@infradead.org> +Tested-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> +Signed-off-by: David S. Miller <davem@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); + }
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. 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@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@kresin.me> for debugging assistance and to David Woodhouse <dwmw2@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@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