| Message ID | 1551178601-16564-1-git-send-email-michael.chan@broadcom.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC] net: Validate size of non-TSO packets in validate_xmit_skb(). | expand |
On 02/26/2019 02:56 AM, Michael Chan wrote: > There have been reports of oversize UDP packets being sent to the > driver to be transmitted, causing error conditions. The issue is > likely caused by the dst of the SKB switching between 'lo' with > 64K MTU and the hardware device with a smaller MTU. Patches are > being proposed by Mahesh Bandewar <maheshb@google.com> to fix the > issue. > > Separately, we should add a length check in validate_xmit_skb() > to drop these oversize packets before they reach the driver. Why ? We keep adding checks in the 'fast path' and make slower and slower after each release. We need to fix the root cause really. > This patch only validates non-TSO packets. Complete validation > of segmented TSO packet size will probably be too slow. > > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > --- > net/core/dev.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 5d03889..50c5174 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3373,6 +3373,13 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device > } > } > > + if (!skb_is_gso(skb) && > + skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)) { > + net_warn_ratelimited("%s(): Dropping %d bytes oversize skb.\n", > + __func__, skb->len); > + goto out_kfree_skb; > + } > + > skb = validate_xmit_xfrm(skb, features, again); > > return skb; >
From: Michael Chan <michael.chan@broadcom.com> Date: Tue, 26 Feb 2019 05:56:41 -0500 > There have been reports of oversize UDP packets being sent to the > driver to be transmitted, causing error conditions. The issue is > likely caused by the dst of the SKB switching between 'lo' with > 64K MTU and the hardware device with a smaller MTU. Patches are > being proposed by Mahesh Bandewar <maheshb@google.com> to fix the > issue. > > Separately, we should add a length check in validate_xmit_skb() > to drop these oversize packets before they reach the driver. > This patch only validates non-TSO packets. Complete validation > of segmented TSO packet size will probably be too slow. > > Signed-off-by: Michael Chan <michael.chan@broadcom.com> Anything which changes the dst of an SKB really is responsible for fixing up whatever became "incompatible" in the new path. So like Eric I want to see this out of the fast path.
On Tue, Feb 26, 2019 at 9:13 AM David Miller <davem@davemloft.net> wrote: > > From: Michael Chan <michael.chan@broadcom.com> > Date: Tue, 26 Feb 2019 05:56:41 -0500 > > > There have been reports of oversize UDP packets being sent to the > > driver to be transmitted, causing error conditions. The issue is > > likely caused by the dst of the SKB switching between 'lo' with > > 64K MTU and the hardware device with a smaller MTU. Patches are > > being proposed by Mahesh Bandewar <maheshb@google.com> to fix the > > issue. > > > > Separately, we should add a length check in validate_xmit_skb() > > to drop these oversize packets before they reach the driver. > > This patch only validates non-TSO packets. Complete validation > > of segmented TSO packet size will probably be too slow. > > > > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > > Anything which changes the dst of an SKB really is responsible for > fixing up whatever became "incompatible" in the new path. > > So like Eric I want to see this out of the fast path. Ok. In the meantime, will you take a 2-line bnxt_en patch that will prevent this issue in kernel 5.0?
From: Michael Chan <michael.chan@broadcom.com> Date: Tue, 26 Feb 2019 10:22:42 -0800 > On Tue, Feb 26, 2019 at 9:13 AM David Miller <davem@davemloft.net> wrote: >> >> From: Michael Chan <michael.chan@broadcom.com> >> Date: Tue, 26 Feb 2019 05:56:41 -0500 >> >> > There have been reports of oversize UDP packets being sent to the >> > driver to be transmitted, causing error conditions. The issue is >> > likely caused by the dst of the SKB switching between 'lo' with >> > 64K MTU and the hardware device with a smaller MTU. Patches are >> > being proposed by Mahesh Bandewar <maheshb@google.com> to fix the >> > issue. >> > >> > Separately, we should add a length check in validate_xmit_skb() >> > to drop these oversize packets before they reach the driver. >> > This patch only validates non-TSO packets. Complete validation >> > of segmented TSO packet size will probably be too slow. >> > >> > Signed-off-by: Michael Chan <michael.chan@broadcom.com> >> >> Anything which changes the dst of an SKB really is responsible for >> fixing up whatever became "incompatible" in the new path. >> >> So like Eric I want to see this out of the fast path. > > Ok. In the meantime, will you take a 2-line bnxt_en patch that will > prevent this issue in kernel 5.0? Sure, but we will have to remember to remove it when it is no longer necessary...
diff --git a/net/core/dev.c b/net/core/dev.c index 5d03889..50c5174 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3373,6 +3373,13 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device } } + if (!skb_is_gso(skb) && + skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)) { + net_warn_ratelimited("%s(): Dropping %d bytes oversize skb.\n", + __func__, skb->len); + goto out_kfree_skb; + } + skb = validate_xmit_xfrm(skb, features, again); return skb;
There have been reports of oversize UDP packets being sent to the driver to be transmitted, causing error conditions. The issue is likely caused by the dst of the SKB switching between 'lo' with 64K MTU and the hardware device with a smaller MTU. Patches are being proposed by Mahesh Bandewar <maheshb@google.com> to fix the issue. Separately, we should add a length check in validate_xmit_skb() to drop these oversize packets before they reach the driver. This patch only validates non-TSO packets. Complete validation of segmented TSO packet size will probably be too slow. Signed-off-by: Michael Chan <michael.chan@broadcom.com> --- net/core/dev.c | 7 +++++++ 1 file changed, 7 insertions(+)