| Message ID | 1364209702-12437-3-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On 25/03/13 11:08, 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. [...] > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* Wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t. If skb->len is too big, drop it and alert user about > + * misconfiguration. > + */ > + if (unlikely(skb->len >= (uint16_t)(~0))) { Suggest a #define for this maximum added to include/xen/interface/io/netif.h David -- 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 25.03.13 at 15:23, David Vrabel <david.vrabel@citrix.com> wrote: > On 25/03/13 11:08, 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. > [...] >> --- a/drivers/net/xen-netfront.c >> +++ b/drivers/net/xen-netfront.c >> @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct > net_device *dev) >> unsigned int len = skb_headlen(skb); >> unsigned long flags; >> >> + /* Wire format of xen_netif_tx_request only supports skb->len >> + * < 64K, because size field in xen_netif_tx_request is >> + * uint16_t. If skb->len is too big, drop it and alert user about >> + * misconfiguration. >> + */ >> + if (unlikely(skb->len >= (uint16_t)(~0))) { > > Suggest a #define for this maximum added to include/xen/interface/io/netif.h I don't see a point in doing so. If you want the connection to be explicit, just use typeof() here instead of uint16_t. Jan -- 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
Hello. On 25-03-2013 15:08, 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. > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 5527663..3ae9dc1 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* Wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t. If skb->len is too big, drop it and alert user about > + * misconfiguration. > + */ > + if (unlikely(skb->len >= (uint16_t)(~0))) { Such types as 'uint16_t' are intended for userland -- use 'u16' instead. Better still, just use 0xffffu. WBR, Sergei -- 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
From: Wei Liu <wei.liu2@citrix.com> Date: Mon, 25 Mar 2013 11:08:18 +0000 > 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> This is effectively the default already, you don't need to change this value explicitly. ->gso_max_size is set by default to 65536 and then TCP performs this calculation: xmit_size_goal = ((sk->sk_gso_max_size - 1) - inet_csk(sk)->icsk_af_ops->net_header_len - inet_csk(sk)->icsk_ext_hdr_len - tp->tcp_header_len); thereby making it adhere to your limits just fine. -- 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 Mon, 2013-03-25 at 12:18 -0400, David Miller wrote: > > This is effectively the default already, you don't need to change this > value explicitly. > > ->gso_max_size is set by default to 65536 and then TCP performs this > calculation: > > xmit_size_goal = ((sk->sk_gso_max_size - 1) - > inet_csk(sk)->icsk_af_ops->net_header_len - > inet_csk(sk)->icsk_ext_hdr_len - > tp->tcp_header_len); > > thereby making it adhere to your limits just fine. For locally generated TCP traffic this is the case. However, GRO can build packets up to 65535 bytes, not including the Ethernet header. For such packets, it seems xen-netfront needs a segmentation. And we might have other providers as well (UFO for example ?), but I have not checked. -- 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 Mon, Mar 25, 2013 at 11:08:18AM +0000, 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. > Should it also CC stable@vger.kernel.org? > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 5527663..3ae9dc1 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > unsigned int len = skb_headlen(skb); > unsigned long flags; > > + /* Wire format of xen_netif_tx_request only supports skb->len > + * < 64K, because size field in xen_netif_tx_request is > + * uint16_t. If skb->len is too big, drop it and alert user about > + * misconfiguration. > + */ > + if (unlikely(skb->len >= (uint16_t)(~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)) { > @@ -1362,6 +1374,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 - ETH_HLEN); > + > np->netdev = netdev; > > netif_carrier_off(netdev); > -- > 1.7.10.4 > -- 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
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 25 Mar 2013 09:54:32 -0700 > On Mon, 2013-03-25 at 12:18 -0400, David Miller wrote: > >> >> This is effectively the default already, you don't need to change this >> value explicitly. >> >> ->gso_max_size is set by default to 65536 and then TCP performs this >> calculation: >> >> xmit_size_goal = ((sk->sk_gso_max_size - 1) - >> inet_csk(sk)->icsk_af_ops->net_header_len - >> inet_csk(sk)->icsk_ext_hdr_len - >> tp->tcp_header_len); >> >> thereby making it adhere to your limits just fine. > > For locally generated TCP traffic this is the case. Right, and also any other piece of code that is not interpreting the gso_max_size value the same way (as "(x - 1) - sizeof_headers") would need to be fixed. > However, GRO can build packets up to 65535 bytes, not including the > Ethernet header. If this GRO packet ends up being transmitted, the gso limit should be applied, otherwise we would be violating the device's advertised GSO limit value. Assume that this kind of check is performed (it must), then I don't see how GRO can cause any problems for Xen. -- 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 Mon, 2013-03-25 at 12:59 -0400, David Miller wrote: > If this GRO packet ends up being transmitted, the gso limit should be > applied, otherwise we would be violating the device's advertised GSO > limit value. > > Assume that this kind of check is performed (it must), then I don't > see how GRO can cause any problems for Xen. It seems nobody cared to perform this generic check. netif_skb_features() only deals with max_segs : if (skb_shinfo(skb)->gso_segs > skb->dev->gso_max_segs) features &= ~NETIF_F_GSO_MASK; dev->gso_max_size is currently only used to populate sk->sk_gso_max_size For regular 1500 MTU and at most 17 frags per skb, its hardly a problem, but it could happen with jumbo frames, or using loopback and splice() -- 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 Mon, Mar 25, 2013 at 04:18:09PM +0000, David Miller wrote: > From: Wei Liu <wei.liu2@citrix.com> > Date: Mon, 25 Mar 2013 11:08:18 +0000 > > > 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> > > This is effectively the default already, you don't need to change this > value explicitly. > > ->gso_max_size is set by default to 65536 and then TCP performs this > calculation: > > xmit_size_goal = ((sk->sk_gso_max_size - 1) - > inet_csk(sk)->icsk_af_ops->net_header_len - > inet_csk(sk)->icsk_ext_hdr_len - > tp->tcp_header_len); OK. But I see similar fix for a physical nic (commit b7e5887e0e414b), am I missing something here? And the symptom is that if I don't reserve headroom I see skb->len = 65538. Can you shed some light on this? Wei. > > thereby making it adhere to your limits just fine. -- 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
From: Wei Liu <wei.liu2@citrix.com> Date: Mon, 25 Mar 2013 18:32:06 +0000 > On Mon, Mar 25, 2013 at 04:18:09PM +0000, David Miller wrote: >> From: Wei Liu <wei.liu2@citrix.com> >> Date: Mon, 25 Mar 2013 11:08:18 +0000 >> >> > 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> >> >> This is effectively the default already, you don't need to change this >> value explicitly. >> >> ->gso_max_size is set by default to 65536 and then TCP performs this >> calculation: >> >> xmit_size_goal = ((sk->sk_gso_max_size - 1) - >> inet_csk(sk)->icsk_af_ops->net_header_len - >> inet_csk(sk)->icsk_ext_hdr_len - >> tp->tcp_header_len); > > OK. But I see similar fix for a physical nic (commit b7e5887e0e414b), am > I missing something here? > > And the symptom is that if I don't reserve headroom I see skb->len = > 65538. Can you shed some light on this? See Eric's reply. If a GRO frame is forwarded we don't make the GSO size checks on the send size as we should. -- 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 Mon, 2013-03-25 at 12:59 -0400, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 25 Mar 2013 09:54:32 -0700 > > > On Mon, 2013-03-25 at 12:18 -0400, David Miller wrote: > > > >> > >> This is effectively the default already, you don't need to change this > >> value explicitly. > >> > >> ->gso_max_size is set by default to 65536 and then TCP performs this > >> calculation: > >> > >> xmit_size_goal = ((sk->sk_gso_max_size - 1) - > >> inet_csk(sk)->icsk_af_ops->net_header_len - > >> inet_csk(sk)->icsk_ext_hdr_len - > >> tp->tcp_header_len); > >> > >> thereby making it adhere to your limits just fine. > > > > For locally generated TCP traffic this is the case. > > Right, and also any other piece of code that is not interpreting the > gso_max_size value the same way (as "(x - 1) - sizeof_headers") would > need to be fixed. [...] tcp_tso_should_defer() also ignores headers, though I don't think the check is particularly critical in that case. Ben.
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 5527663..3ae9dc1 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -547,6 +547,18 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) unsigned int len = skb_headlen(skb); unsigned long flags; + /* Wire format of xen_netif_tx_request only supports skb->len + * < 64K, because size field in xen_netif_tx_request is + * uint16_t. If skb->len is too big, drop it and alert user about + * misconfiguration. + */ + if (unlikely(skb->len >= (uint16_t)(~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)) { @@ -1362,6 +1374,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 - 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, 14 insertions(+)