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

2014-12-22 Thread Hayes Wang 
 -Original Message-
 From: Hayes Wang 
 Sent: Monday, December 22, 2014 10:23 AM
 To: 'Eric Dumazet'
 Cc: Tom Herbert; David Miller; net...@vger.kernel.org; 
 nic_swsd; linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org
 Subject: RE: [PATCH net] r8152: drop the tx packet with invalid length
 
  Eric Dumazet [mailto:eric.duma...@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
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-12-21 Thread Hayes Wang 
 Eric Dumazet [mailto:eric.duma...@gmail.com] 
 Sent: Saturday, December 20, 2014 2:14 AM
[...]
 Could you try following patch ?

Thank you. I would test it.
 
Best Regards,
Hayes
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-12-19 Thread Eric Dumazet 
On Fri, 2014-12-19 at 06:42 +, 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 linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-12-19 Thread Eric Dumazet 
On Fri, 2014-12-19 at 09:36 -0800, Eric Dumazet wrote:
 On Fri, 2014-12-19 at 06:42 +, 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);


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-12-18 Thread Hayes Wang 
 From: David Miller [mailto:da...@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 linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-26 Thread 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 hayesw...@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 linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-26 Thread David Miller 
From: Eric Dumazet eric.duma...@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 hayesw...@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 linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-26 Thread Eric Dumazet 
On Wed, 2014-11-26 at 12:06 -0500, David Miller wrote:
 From: Eric Dumazet eric.duma...@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 hayesw...@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 linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-26 Thread David Miller 
From: Eric Dumazet eric.duma...@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.duma...@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 hayesw...@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 linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html