| Message ID | 1394712342-15778-104-Taiwan-albertk@realtek.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote: > Drop the tx packet which is more than the size of agg_buf_sz. When > creating a bridge with the device, we may get the tx packet with > TSO and the length is more than the gso_max_size which is set by > the driver through netif_set_gso_max_size(). Such packets couldn't > be transmitted and should be dropped directly. > > Signed-off-by: Hayes Wang <hayeswang@realtek.com> > --- > drivers/net/usb/r8152.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index c6554c7..ebdaff7 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -1897,6 +1897,15 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb, > { > struct r8152 *tp = netdev_priv(netdev); > > + if ((skb->len + sizeof(struct tx_desc)) > agg_buf_sz) { > + struct net_device_stats *stats = &netdev->stats; > + > + dev_kfree_skb_any(skb); > + stats->tx_dropped++; > + WARN_ON_ONCE(1); > + return NETDEV_TX_OK; > + } > + > skb_tx_timestamp(skb); > > skb_queue_tail(&tp->tx_queue, skb); Looks like a candidate for ndo_gso_check(), so that we do not drop, but instead segment from netif_needs_gso()/validate_xmit_skb() -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 26 Nov 2014 08:52:28 -0800 > On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote: >> Drop the tx packet which is more than the size of agg_buf_sz. When >> creating a bridge with the device, we may get the tx packet with >> TSO and the length is more than the gso_max_size which is set by >> the driver through netif_set_gso_max_size(). Such packets couldn't >> be transmitted and should be dropped directly. >> >> Signed-off-by: Hayes Wang <hayeswang@realtek.com> ... > Looks like a candidate for ndo_gso_check(), so that we do not drop, but > instead segment from netif_needs_gso()/validate_xmit_skb() You mean have the bridge implement the ndo_gso_check() method right? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-11-26 at 12:06 -0500, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Wed, 26 Nov 2014 08:52:28 -0800 > > > On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote: > >> Drop the tx packet which is more than the size of agg_buf_sz. When > >> creating a bridge with the device, we may get the tx packet with > >> TSO and the length is more than the gso_max_size which is set by > >> the driver through netif_set_gso_max_size(). Such packets couldn't > >> be transmitted and should be dropped directly. > >> > >> Signed-off-by: Hayes Wang <hayeswang@realtek.com> > ... > > Looks like a candidate for ndo_gso_check(), so that we do not drop, but > > instead segment from netif_needs_gso()/validate_xmit_skb() > > You mean have the bridge implement the ndo_gso_check() method right? No, I meant this particular driver. Note that netif_skb_features() does only this check : if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs) features &= ~NETIF_F_GSO_MASK; Ie not testing gso_max_size It looks like all these particular tests should be moved on ndo_gso_check(), to remove code from netif_skb_features() -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 26 Nov 2014 10:44:19 -0800 > On Wed, 2014-11-26 at 12:06 -0500, David Miller wrote: >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Wed, 26 Nov 2014 08:52:28 -0800 >> >> > On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote: >> >> Drop the tx packet which is more than the size of agg_buf_sz. When >> >> creating a bridge with the device, we may get the tx packet with >> >> TSO and the length is more than the gso_max_size which is set by >> >> the driver through netif_set_gso_max_size(). Such packets couldn't >> >> be transmitted and should be dropped directly. >> >> >> >> Signed-off-by: Hayes Wang <hayeswang@realtek.com> >> ... >> > Looks like a candidate for ndo_gso_check(), so that we do not drop, but >> > instead segment from netif_needs_gso()/validate_xmit_skb() >> >> You mean have the bridge implement the ndo_gso_check() method right? > > No, I meant this particular driver. > > Note that netif_skb_features() does only this check : > > if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs) > features &= ~NETIF_F_GSO_MASK; > > Ie not testing gso_max_size > > It looks like all these particular tests should be moved on > ndo_gso_check(), to remove code from netif_skb_features() A check against gso_max_size is generic enough that it ought to be put right into netif_needs_gso() rather then duplicating it into every driver's ndo_gso_check() method don't you think? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: David Miller [mailto:davem@davemloft.net] > Sent: Thursday, November 27, 2014 4:34 AM [...] > >> > Looks like a candidate for ndo_gso_check(), so that we do not drop, but > >> > instead segment from netif_needs_gso()/validate_xmit_skb() > >> > >> You mean have the bridge implement the ndo_gso_check() method right? > > > > No, I meant this particular driver. > > > > Note that netif_skb_features() does only this check : > > > > if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs) > > features &= ~NETIF_F_GSO_MASK; > > > > Ie not testing gso_max_size > > > > It looks like all these particular tests should be moved on > > ndo_gso_check(), to remove code from netif_skb_features() > > A check against gso_max_size is generic enough that it ought to be put > right into netif_needs_gso() rather then duplicating it into every > driver's ndo_gso_check() method don't you think? Excuse me. I try to implement ndo_gso_check() with kernel 3.18. However, I still get packets with gso and their skb lengths are more than the acceptable one. Do I miss something? +static bool rtl8152_gso_check(struct sk_buff *skb, struct net_device *dev) +{ + if ((skb->len + sizeof(struct tx_desc)) > agg_buf_sz) + return false; + + return true; +} @@ -5861,6 +5876,9 @@ static const struct net_device_ops rtl8152_netdev_ops = { .ndo_set_mac_address = rtl8152_set_mac_address, .ndo_change_mtu = rtl8152_change_mtu, .ndo_validate_addr = eth_validate_addr, + .ndo_gso_check = rtl8152_gso_check, }; Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-12-19 at 06:42 +0000, Hayes Wang wrote: > Excuse me. I try to implement ndo_gso_check() with kernel 3.18. > However, I still get packets with gso and their skb lengths are more > than the acceptable one. Do I miss something? > > +static bool rtl8152_gso_check(struct sk_buff *skb, struct net_device *dev) > +{ > + if ((skb->len + sizeof(struct tx_desc)) > agg_buf_sz) > + return false; > + > + return true; > +} > > @@ -5861,6 +5876,9 @@ static const struct net_device_ops rtl8152_netdev_ops = { > .ndo_set_mac_address = rtl8152_set_mac_address, > .ndo_change_mtu = rtl8152_change_mtu, > .ndo_validate_addr = eth_validate_addr, > + .ndo_gso_check = rtl8152_gso_check, > }; You are right, it seems ndo_gso_check() is buggy at this moment. Presumably this method should alter %features so that we really segment the packets in software. features &= ~NETIF_F_GSO_MASK; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index c6554c7..ebdaff7 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -1897,6 +1897,15 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb, { struct r8152 *tp = netdev_priv(netdev); + if ((skb->len + sizeof(struct tx_desc)) > agg_buf_sz) { + struct net_device_stats *stats = &netdev->stats; + + dev_kfree_skb_any(skb); + stats->tx_dropped++; + WARN_ON_ONCE(1); + return NETDEV_TX_OK; + } + skb_tx_timestamp(skb); skb_queue_tail(&tp->tx_queue, skb);
Drop the tx packet which is more than the size of agg_buf_sz. When creating a bridge with the device, we may get the tx packet with TSO and the length is more than the gso_max_size which is set by the driver through netif_set_gso_max_size(). Such packets couldn't be transmitted and should be dropped directly. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/usb/r8152.c | 9 +++++++++ 1 file changed, 9 insertions(+)