Xen devel - Apr 2013 - [PATCH 5/7] xen-netfront: reduce gso_max_size to account for ethernet header

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

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

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.

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.

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.
>

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.

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.
>

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.

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 ?

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.

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.

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?
-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that''s the marketing
department''s job.
They asked us to note that Solarflare product names are trademarked.