[PATCH net] r8152: drop the tx packet with invalid length

[PATCH net] r8152: drop the tx packet with invalid length

[Hayes Wang]

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);
-- 
1.9.3

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

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()

Re: [PATCH net] r8152: drop the tx packet with invalid length

[David Miller]

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?

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

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()

Re: [PATCH net] r8152: drop the tx packet with invalid length

[David Miller]

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?

RE: [PATCH net] r8152: drop the tx packet with invalid length

[Hayes Wang]

> 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

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

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;

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

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);

RE: [PATCH net] r8152: drop the tx packet with invalid length

[Hayes Wang]

 Eric Dumazet [mailto:eric.dumazet@gmail.com] 
> Sent: Saturday, December 20, 2014 2:14 AM
[...]
> Could you try following patch ?

Thank you. I would test it.
 
Best Regards,
Hayes

RE: [PATCH net] r8152: drop the tx packet with invalid length

[Hayes Wang]

> -----Original Message-----
> From: Hayes Wang 
> Sent: Monday, December 22, 2014 10:23 AM
> To: 'Eric Dumazet'
> Cc: Tom Herbert; David Miller; netdev@vger.kernel.org; 
> nic_swsd; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: RE: [PATCH net] r8152: drop the tx packet with invalid length
> 
>  Eric Dumazet [mailto:eric.dumazet@gmail.com] 
> > 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

[PATCH net] net: ndo_gso_check() must force segmentation

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

If ndo_gso_check() returns true, it means a driver wants the stack
to perform software segmentation, even if device features initially
claimed hardware was able handle a TSO packet. 

This means netif_needs_gso() needs to modify the features.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Hayes Wang <hayeswang@realtek.com>
Tested-by: Hayes Wang <hayeswang@realtek.com>
Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")
---
 drivers/net/macvtap.c      |    2 +-
 drivers/net/xen-netfront.c |    4 +++-
 include/linux/netdevice.h  |   18 ++++++++++++------
 net/core/dev.c             |    2 +-
 4 files changed, 17 insertions(+), 9 deletions(-)

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);

Re: [PATCH net] net: ndo_gso_check() must force segmentation

[Jesse Gross]

On Mon, Dec 22, 2014 at 10:21 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> If ndo_gso_check() returns true, it means a driver wants the stack
> to perform software segmentation, even if device features initially
> claimed hardware was able handle a TSO packet.
>
> This means netif_needs_gso() needs to modify the features.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Hayes Wang <hayeswang@realtek.com>
> Tested-by: Hayes Wang <hayeswang@realtek.com>
> Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")

There's actually another problem with ndo_gso_check() - it doesn't
deal with other features like checksum offloading which usually have
similar constraints. As a result, the GSO code generates segments with
checksums offloading that immediately fails.

I have a patch that I was working on last night that behaves similarly
to yours but is also generalized to handle both cases. Let me send it
out now.