Patchwork [5/7] xen-netfront: reduce gso_max_size to account for ethernet header

login
register
mail settings
Submitter Wei Liu
Date April 9, 2013, 11:07 a.m.
Message ID <1365505655-8021-6-git-send-email-wei.liu2@citrix.com>
Download mbox | patch
Permalink /patch/235041/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Wei Liu - April 9, 2013, 11:07 a.m.
The maximum packet including ethernet header that can be handled by netfront /
netback wire format is 65535. Reduce gso_max_size accordingly.

Drop skb and print warning when skb->len > 65535. This can 1) save the effort
to send malformed packet to netback, 2) help spotting misconfiguration of
netfront in the future.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netfront.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
Wei Liu - April 11, 2013, 8:04 p.m.
On Tue, Apr 09, 2013 at 12:07:33PM +0100, Wei Liu wrote:
> The maximum packet including ethernet header that can be handled by netfront /
> netback wire format is 65535. Reduce gso_max_size accordingly.
> 
> Drop skb and print warning when skb->len > 65535. This can 1) save the effort
> to send malformed packet to netback, 2) help spotting misconfiguration of
> netfront in the future.
> 

Any opinion on how much space should be reserved? From a previous thread
Ben seemed to suggest 90 (Ethernet + VLAN tag + IPv6 + TCP + timestamp
option = 90 bytes).


Wei.
--
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
Ian Campbell - April 12, 2013, 8:18 a.m.
On Thu, 2013-04-11 at 21:04 +0100, Wei Liu wrote:
> On Tue, Apr 09, 2013 at 12:07:33PM +0100, Wei Liu wrote:
> > The maximum packet including ethernet header that can be handled by netfront /
> > netback wire format is 65535. Reduce gso_max_size accordingly.
> > 
> > Drop skb and print warning when skb->len > 65535. This can 1) save the effort
> > to send malformed packet to netback, 2) help spotting misconfiguration of
> > netfront in the future.
> > 
> 
> Any opinion on how much space should be reserved? From a previous thread
> Ben seemed to suggest 90 (Ethernet + VLAN tag + IPv6 + TCP + timestamp
> option = 90 bytes).

I trust Ben and that seems as good as anything to me.

Is this the sort of limit others might be interested in, should we have
a global #define?

Ian.

--
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
Wei Liu - April 12, 2013, 8:48 a.m.
On Fri, Apr 12, 2013 at 09:18:04AM +0100, Ian Campbell wrote:
> On Thu, 2013-04-11 at 21:04 +0100, Wei Liu wrote:
> > On Tue, Apr 09, 2013 at 12:07:33PM +0100, Wei Liu wrote:
> > > The maximum packet including ethernet header that can be handled by netfront /
> > > netback wire format is 65535. Reduce gso_max_size accordingly.
> > > 
> > > Drop skb and print warning when skb->len > 65535. This can 1) save the effort
> > > to send malformed packet to netback, 2) help spotting misconfiguration of
> > > netfront in the future.
> > > 
> > 
> > Any opinion on how much space should be reserved? From a previous thread
> > Ben seemed to suggest 90 (Ethernet + VLAN tag + IPv6 + TCP + timestamp
> > option = 90 bytes).
> 
> I trust Ben and that seems as good as anything to me.
> 
> Is this the sort of limit others might be interested in, should we have
> a global #define?
> 

We shall have a global define in this case.

#define XEN_NETFRONT_MAX_HEADER ? I'm bad at naming things.


Wei.

> Ian.
> 
--
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
Ian Campbell - April 12, 2013, 8:57 a.m.
On Fri, 2013-04-12 at 09:48 +0100, Wei Liu wrote:
> On Fri, Apr 12, 2013 at 09:18:04AM +0100, Ian Campbell wrote:
> > On Thu, 2013-04-11 at 21:04 +0100, Wei Liu wrote:
> > > On Tue, Apr 09, 2013 at 12:07:33PM +0100, Wei Liu wrote:
> > > > The maximum packet including ethernet header that can be handled by netfront /
> > > > netback wire format is 65535. Reduce gso_max_size accordingly.
> > > > 
> > > > Drop skb and print warning when skb->len > 65535. This can 1) save the effort
> > > > to send malformed packet to netback, 2) help spotting misconfiguration of
> > > > netfront in the future.
> > > > 
> > > 
> > > Any opinion on how much space should be reserved? From a previous thread
> > > Ben seemed to suggest 90 (Ethernet + VLAN tag + IPv6 + TCP + timestamp
> > > option = 90 bytes).
> > 
> > I trust Ben and that seems as good as anything to me.
> > 
> > Is this the sort of limit others might be interested in, should we have
> > a global #define?
> > 
> 
> We shall have a global define in this case.
> 
> #define XEN_NETFRONT_MAX_HEADER ? I'm bad at naming things.

I meant an include/linux/skbuff.h (or some suitable header) #define
SKB_MAX_FOO type thing...

Ian.

--
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
Wei Liu - April 12, 2013, 9:34 a.m.
On Fri, Apr 12, 2013 at 09:57:15AM +0100, Ian Campbell wrote:
> On Fri, 2013-04-12 at 09:48 +0100, Wei Liu wrote:
> > On Fri, Apr 12, 2013 at 09:18:04AM +0100, Ian Campbell wrote:
> > > On Thu, 2013-04-11 at 21:04 +0100, Wei Liu wrote:
> > > > On Tue, Apr 09, 2013 at 12:07:33PM +0100, Wei Liu wrote:
> > > > > The maximum packet including ethernet header that can be handled by netfront /
> > > > > netback wire format is 65535. Reduce gso_max_size accordingly.
> > > > > 
> > > > > Drop skb and print warning when skb->len > 65535. This can 1) save the effort
> > > > > to send malformed packet to netback, 2) help spotting misconfiguration of
> > > > > netfront in the future.
> > > > > 
> > > > 
> > > > Any opinion on how much space should be reserved? From a previous thread
> > > > Ben seemed to suggest 90 (Ethernet + VLAN tag + IPv6 + TCP + timestamp
> > > > option = 90 bytes).
> > > 
> > > I trust Ben and that seems as good as anything to me.
> > > 
> > > Is this the sort of limit others might be interested in, should we have
> > > a global #define?
> > > 
> > 
> > We shall have a global define in this case.
> > 
> > #define XEN_NETFRONT_MAX_HEADER ? I'm bad at naming things.
> 
> I meant an include/linux/skbuff.h (or some suitable header) #define
> SKB_MAX_FOO type thing...
> 

But we don't have handle on this. If I understand correctly the
discussion in other thread, 90 is empirical value, not something
documented.


Wei.

> Ian.
> 
--
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
Ian Campbell - April 12, 2013, 9:43 a.m.
On Fri, 2013-04-12 at 10:34 +0100, Wei Liu wrote:
> On Fri, Apr 12, 2013 at 09:57:15AM +0100, Ian Campbell wrote:
> > On Fri, 2013-04-12 at 09:48 +0100, Wei Liu wrote:
> > > On Fri, Apr 12, 2013 at 09:18:04AM +0100, Ian Campbell wrote:
> > > > On Thu, 2013-04-11 at 21:04 +0100, Wei Liu wrote:
> > > > > On Tue, Apr 09, 2013 at 12:07:33PM +0100, Wei Liu wrote:
> > > > > > The maximum packet including ethernet header that can be handled by netfront /
> > > > > > netback wire format is 65535. Reduce gso_max_size accordingly.
> > > > > > 
> > > > > > Drop skb and print warning when skb->len > 65535. This can 1) save the effort
> > > > > > to send malformed packet to netback, 2) help spotting misconfiguration of
> > > > > > netfront in the future.
> > > > > > 
> > > > > 
> > > > > Any opinion on how much space should be reserved? From a previous thread
> > > > > Ben seemed to suggest 90 (Ethernet + VLAN tag + IPv6 + TCP + timestamp
> > > > > option = 90 bytes).
> > > > 
> > > > I trust Ben and that seems as good as anything to me.
> > > > 
> > > > Is this the sort of limit others might be interested in, should we have
> > > > a global #define?
> > > > 
> > > 
> > > We shall have a global define in this case.
> > > 
> > > #define XEN_NETFRONT_MAX_HEADER ? I'm bad at naming things.
> > 
> > I meant an include/linux/skbuff.h (or some suitable header) #define
> > SKB_MAX_FOO type thing...
> > 
> 
> But we don't have handle on this. If I understand correctly the
> discussion in other thread, 90 is empirical value, not something
> documented.

My original question was effectively "is anyone else going to be
interested in this empirical value", if so then it seems like it would
be useful to have it centrally defined.

Ian.

--
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
Eric Dumazet - April 12, 2013, 12:58 p.m.
On Fri, 2013-04-12 at 10:43 +0100, Ian Campbell wrote:
> On Fri, 2013-04-12 at 10:34 +0100, Wei Liu wrote:

> > But we don't have handle on this. If I understand correctly the
> > discussion in other thread, 90 is empirical value, not something
> > documented.
> 
> My original question was effectively "is anyone else going to be
> interested in this empirical value", if so then it seems like it would
> be useful to have it centrally defined.
> 

This could be MAX_TCP_HEADER. Probably a bit overestimated but do we
care ?



--
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
Wei Liu - April 12, 2013, 1:29 p.m.
On Fri, Apr 12, 2013 at 01:58:19PM +0100, Eric Dumazet wrote:
> On Fri, 2013-04-12 at 10:43 +0100, Ian Campbell wrote:
> > On Fri, 2013-04-12 at 10:34 +0100, Wei Liu wrote:
> 
> > > But we don't have handle on this. If I understand correctly the
> > > discussion in other thread, 90 is empirical value, not something
> > > documented.
> > 
> > My original question was effectively "is anyone else going to be
> > interested in this empirical value", if so then it seems like it would
> > be useful to have it centrally defined.
> > 
> 
> This could be MAX_TCP_HEADER. Probably a bit overestimated but do we
> care ?
> 

I don't think we care. MAX_TCP_HEADER is as good as any. Reserving a few
more bytes won't hurt. I just want to make sure the value doesn't look
like something randomly comes up in my mind. :-)

Ian, what do you think?


Wei.
--
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
Ian Campbell - April 12, 2013, 1:36 p.m.
On Fri, 2013-04-12 at 14:29 +0100, Wei Liu wrote:
> On Fri, Apr 12, 2013 at 01:58:19PM +0100, Eric Dumazet wrote:
> > On Fri, 2013-04-12 at 10:43 +0100, Ian Campbell wrote:
> > > On Fri, 2013-04-12 at 10:34 +0100, Wei Liu wrote:
> > 
> > > > But we don't have handle on this. If I understand correctly the
> > > > discussion in other thread, 90 is empirical value, not something
> > > > documented.
> > > 
> > > My original question was effectively "is anyone else going to be
> > > interested in this empirical value", if so then it seems like it would
> > > be useful to have it centrally defined.
> > > 
> > 
> > This could be MAX_TCP_HEADER. Probably a bit overestimated but do we
> > care ?
> > 
> 
> I don't think we care. MAX_TCP_HEADER is as good as any. Reserving a few
> more bytes won't hurt. I just want to make sure the value doesn't look
> like something randomly comes up in my mind. :-)
> 
> Ian, what do you think?

It could be up to 256 bytes from the looks of things, depending
on .config. That's probably ok.

Ian.


--
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
Ben Hutchings - April 12, 2013, 4:17 p.m.
On Fri, 2013-04-12 at 09:18 +0100, Ian Campbell wrote:
> On Thu, 2013-04-11 at 21:04 +0100, Wei Liu wrote:
> > On Tue, Apr 09, 2013 at 12:07:33PM +0100, Wei Liu wrote:
> > > The maximum packet including ethernet header that can be handled by netfront /
> > > netback wire format is 65535. Reduce gso_max_size accordingly.
> > > 
> > > Drop skb and print warning when skb->len > 65535. This can 1) save the effort
> > > to send malformed packet to netback, 2) help spotting misconfiguration of
> > > netfront in the future.
> > > 
> > 
> > Any opinion on how much space should be reserved? From a previous thread
> > Ben seemed to suggest 90 (Ethernet + VLAN tag + IPv6 + TCP + timestamp
> > option = 90 bytes).
> 
> I trust Ben and that seems as good as anything to me.

I don't know the TCP or the GSO forwarding code well enough to be
certain.  So don't simply trust me on this.

Ben.

> Is this the sort of limit others might be interested in, should we have
> a global #define?

Patch

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 1bb2e20..42ef4a8 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -547,6 +547,16 @@  static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	unsigned int len = skb_headlen(skb);
 	unsigned long flags;
 
+	/* If skb->len is too big for wire format, drop skb and alert
+	 * user about misconfiguration.
+	 */
+	if (unlikely(skb->len > (typeof(tx->size))(~0))) {
+		net_alert_ratelimited(
+			"xennet: skb->len = %u, too big for wire format\n",
+			skb->len);
+		goto drop;
+	}
+
 	slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) +
 		xennet_count_skb_frag_slots(skb);
 	if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
@@ -1058,7 +1068,7 @@  err:
 
 static int xennet_change_mtu(struct net_device *dev, int mtu)
 {
-	int max = xennet_can_sg(dev) ? 65535 - ETH_HLEN : ETH_DATA_LEN;
+	int max = xennet_can_sg(dev) ? 65535 - VLAN_ETH_HLEN : ETH_DATA_LEN;
 
 	if (mtu > max)
 		return -EINVAL;
@@ -1362,6 +1372,8 @@  static struct net_device *xennet_create_dev(struct xenbus_device *dev)
 	SET_ETHTOOL_OPS(netdev, &xennet_ethtool_ops);
 	SET_NETDEV_DEV(netdev, &dev->dev);
 
+	netif_set_gso_max_size(netdev, 65535 - VLAN_ETH_HLEN);
+
 	np->netdev = netdev;
 
 	netif_carrier_off(netdev);