Xen devel - Mar 2013 - [PATCH 2/6] 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, 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

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.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>
> index 5527663..3ae9dc1 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -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);
Is it worth ETH_HLEN actually being VLAN_ETH_HLEN so that we can support 
VLAN''s and GSO?
> +
>   	np->netdev = netdev;
>   
>   	netif_carrier_off(netdev);

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

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

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

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.

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.

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
>

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.

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

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.

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.

On 25/03/13 18:39, David Miller wrote:
> 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?
I think the problem is that the netback wire protocol includes the 
ethernet header.

The xmit_size_goal is taking into account the IP header, IP options and 
TCP header but not the ethernet header itself.

The Emulex driver seems to have similar problem with maximum size of 
packet with ethernet header,
http://patchwork.ozlabs.org/patch/164818/

I think we should subtract VLAN_ETH_HLEN so that we can trunk VLAN 
through netback safely

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.

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