| Message ID | 1365505655-8021-6-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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
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
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?
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);
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(-)