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 <[email protected]>
---
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);
--
1.9.3
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 <[email protected]>
> ---
> 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()
From: Eric Dumazet <[email protected]>
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 <[email protected]>
...
> 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?
On Wed, 2014-11-26 at 12:06 -0500, David Miller wrote:
> From: Eric Dumazet <[email protected]>
> 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 <[email protected]>
> ...
> > 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()
From: Eric Dumazet <[email protected]>
Date: Wed, 26 Nov 2014 10:44:19 -0800
> On Wed, 2014-11-26 at 12:06 -0500, David Miller wrote:
>> From: Eric Dumazet <[email protected]>
>> 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 <[email protected]>
>> ...
>> > 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?
> From: David Miller [mailto:[email protected]]
> 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
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;
On Fri, 2014-12-19 at 09:36 -0800, Eric Dumazet wrote:
> 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;
Could you try following patch ?
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 7df221788cd4..0346bcfe72a5 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -314,7 +314,7 @@ static rx_handler_result_t macvtap_handle_frame(struct sk_buff **pskb)
*/
if (q->flags & IFF_VNET_HDR)
features |= vlan->tap_features;
- if (netif_needs_gso(dev, skb, features)) {
+ if (netif_needs_gso(dev, skb, &features)) {
struct sk_buff *segs = __skb_gso_segment(skb, features, false);
if (IS_ERR(segs))
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 22bcb4e12e2a..9cacabaea175 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -578,6 +578,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
unsigned long flags;
struct netfront_queue *queue = NULL;
unsigned int num_queues = dev->real_num_tx_queues;
+ netdev_features_t features;
u16 queue_index;
/* Drop the packet if no queues are set up */
@@ -611,9 +612,10 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
spin_lock_irqsave(&queue->tx_lock, flags);
+ features = netif_skb_features(skb);
if (unlikely(!netif_carrier_ok(dev) ||
(slots > 1 && !xennet_can_sg(dev)) ||
- netif_needs_gso(dev, skb, netif_skb_features(skb)))) {
+ netif_needs_gso(dev, skb, &features))) {
spin_unlock_irqrestore(&queue->tx_lock, flags);
goto drop;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c31f74d76ebd..fb1f8c900df9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3608,13 +3608,19 @@ static inline bool skb_gso_ok(struct sk_buff *skb, netdev_features_t features)
}
static inline bool netif_needs_gso(struct net_device *dev, struct sk_buff *skb,
- netdev_features_t features)
+ netdev_features_t *features)
{
- return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
- (dev->netdev_ops->ndo_gso_check &&
- !dev->netdev_ops->ndo_gso_check(skb, dev)) ||
- unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
- (skb->ip_summed != CHECKSUM_UNNECESSARY)));
+ if (!skb_is_gso(skb))
+ return false;
+ if (!skb_gso_ok(skb, *features))
+ return true;
+ if (dev->netdev_ops->ndo_gso_check &&
+ !dev->netdev_ops->ndo_gso_check(skb, dev)) {
+ *features &= ~NETIF_F_GSO_MASK;
+ return true;
+ }
+ return skb->ip_summed != CHECKSUM_PARTIAL &&
+ skb->ip_summed != CHECKSUM_UNNECESSARY;
}
static inline void netif_set_gso_max_size(struct net_device *dev,
diff --git a/net/core/dev.c b/net/core/dev.c
index f411c28d0a66..b61c26b45bb7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2668,7 +2668,7 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
if (skb->encapsulation)
features &= dev->hw_enc_features;
- if (netif_needs_gso(dev, skb, features)) {
+ if (netif_needs_gso(dev, skb, &features)) {
struct sk_buff *segs;
segs = skb_gso_segment(skb, features);
Eric Dumazet [mailto:[email protected]]
> Sent: Saturday, December 20, 2014 2:14 AM
[...]
> Could you try following patch ?
Thank you. I would test it.
Best Regards,
Hayes
> -----Original Message-----
> From: Hayes Wang
> Sent: Monday, December 22, 2014 10:23 AM
> To: 'Eric Dumazet'
> Cc: Tom Herbert; David Miller; [email protected];
> nic_swsd; [email protected]; [email protected]
> Subject: RE: [PATCH net] r8152: drop the tx packet with invalid length
>
> Eric Dumazet [mailto:[email protected]]
> > Sent: Saturday, December 20, 2014 2:14 AM
> [...]
> > Could you try following patch ?
>
> Thank you. I would test it.
It works for me. Thanks.
Best Regards,
Hayes