[PATCH] netpoll: Fix skb tail pointer in netpoll_send_udp()

[PATCH] netpoll: Fix skb tail pointer in netpoll_send_udp()

[Bogdan Hamciuc]

As skb->tail wasn't updated after skb_copy_to_linear_data(), subsequent
calls to skb_realloc_headroom() (as made by an ethernet driver's
ndo_start_xmit routine) would only effectively copy the packet headers,
leaving garbage in the payload.

In the process, removed some unnecessary code.

Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@freescale.com>
---
 net/core/netpoll.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 3d84fb9..9a08068 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -362,22 +362,22 @@ EXPORT_SYMBOL(netpoll_send_skb_on_dev);
 
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 {
-	int total_len, eth_len, ip_len, udp_len;
+	int total_len, ip_len, udp_len;
 	struct sk_buff *skb;
 	struct udphdr *udph;
 	struct iphdr *iph;
 	struct ethhdr *eth;
 
 	udp_len = len + sizeof(*udph);
-	ip_len = eth_len = udp_len + sizeof(*iph);
-	total_len = eth_len + ETH_HLEN + NET_IP_ALIGN;
+	ip_len = udp_len + sizeof(*iph);
+	total_len = ip_len + ETH_HLEN + NET_IP_ALIGN;
 
 	skb = find_skb(np, total_len, total_len - len);
 	if (!skb)
 		return;
 
 	skb_copy_to_linear_data(skb, msg, len);
-	skb->len += len;
+	skb_put(skb, len);
 
 	skb_push(skb, sizeof(*udph));
 	skb_reset_transport_header(skb);
-- 
1.5.6.3

[PATCH] netpoll: Add support for hardware checksumming on egress

[Bogdan Hamciuc]

Netpoll used to compute its own csum; but if the device supports, we
should let it do the checksum itself.

Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@freescale.com>
---
 net/core/netpoll.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9a08068..f5d00b4 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -385,13 +385,19 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 	udph->source = htons(np->local_port);
 	udph->dest = htons(np->remote_port);
 	udph->len = htons(udp_len);
-	udph->check = 0;
-	udph->check = csum_tcpudp_magic(np->local_ip,
+
+	/* Only querying the IPv4 csumming capabilities */
+	if (np->dev->features & NETIF_F_IP_CSUM)
+		skb->ip_summed = CHECKSUM_PARTIAL;
+	else {
+		skb->ip_summed = CHECKSUM_NONE;
+		udph->check = csum_tcpudp_magic(np->local_ip,
 					np->remote_ip,
 					udp_len, IPPROTO_UDP,
 					csum_partial(udph, udp_len, 0));
-	if (udph->check == 0)
-		udph->check = CSUM_MANGLED_0;
+		if (udph->check == 0)
+			udph->check = CSUM_MANGLED_0;
+	}
 
 	skb_push(skb, sizeof(*iph));
 	skb_reset_network_header(skb);
-- 
1.5.6.3

Re: [PATCH] netpoll: Add support for hardware checksumming on egress

[Eric Dumazet]

On Tue, 2012-06-12 at 13:26 +0300, Bogdan Hamciuc wrote:
> Netpoll used to compute its own csum; but if the device supports, we
> should let it do the checksum itself.
> 
> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@freescale.com>
> ---
>  net/core/netpoll.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 9a08068..f5d00b4 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -385,13 +385,19 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
>  	udph->source = htons(np->local_port);
>  	udph->dest = htons(np->remote_port);
>  	udph->len = htons(udp_len);
> -	udph->check = 0;
> -	udph->check = csum_tcpudp_magic(np->local_ip,
> +
> +	/* Only querying the IPv4 csumming capabilities */
> +	if (np->dev->features & NETIF_F_IP_CSUM)
> +		skb->ip_summed = CHECKSUM_PARTIAL;
> +	else {
> +		skb->ip_summed = CHECKSUM_NONE;
> +		udph->check = csum_tcpudp_magic(np->local_ip,
>  					np->remote_ip,
>  					udp_len, IPPROTO_UDP,
>  					csum_partial(udph, udp_len, 0));
> -	if (udph->check == 0)
> -		udph->check = CSUM_MANGLED_0;
> +		if (udph->check == 0)
> +			udph->check = CSUM_MANGLED_0;
> +	}
>  
>  	skb_push(skb, sizeof(*iph));
>  	skb_reset_network_header(skb);

Hi Bogdan

I cant see how this can possibly work ?

Which NIC was able to send a good UDP frame after this patch ?

Re: [PATCH] netpoll: Fix skb tail pointer in netpoll_send_udp()

[Eric Dumazet]

On Tue, 2012-06-12 at 13:26 +0300, Bogdan Hamciuc wrote:
> As skb->tail wasn't updated after skb_copy_to_linear_data(), subsequent
> calls to skb_realloc_headroom() (as made by an ethernet driver's
> ndo_start_xmit routine) would only effectively copy the packet headers,
> leaving garbage in the payload.
> 
> In the process, removed some unnecessary code.
> 
> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@freescale.com>
> ---
>  net/core/netpoll.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 3d84fb9..9a08068 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -362,22 +362,22 @@ EXPORT_SYMBOL(netpoll_send_skb_on_dev);
>  
>  void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
>  {
> -	int total_len, eth_len, ip_len, udp_len;
> +	int total_len, ip_len, udp_len;
>  	struct sk_buff *skb;
>  	struct udphdr *udph;
>  	struct iphdr *iph;
>  	struct ethhdr *eth;
>  
>  	udp_len = len + sizeof(*udph);
> -	ip_len = eth_len = udp_len + sizeof(*iph);
> -	total_len = eth_len + ETH_HLEN + NET_IP_ALIGN;
> +	ip_len = udp_len + sizeof(*iph);
> +	total_len = ip_len + ETH_HLEN + NET_IP_ALIGN;
>  
>  	skb = find_skb(np, total_len, total_len - len);
>  	if (!skb)
>  		return;
>  
>  	skb_copy_to_linear_data(skb, msg, len);
> -	skb->len += len;
> +	skb_put(skb, len);
>  
>  	skb_push(skb, sizeof(*udph));
>  	skb_reset_transport_header(skb);

Hmm, real question is why skb_realloc_headroom() is even necessary...

I suspect we need to reserve more bytes.

	total_len = ip_len + ETH_HLEN + NET_IP_ALIGN + NET_SKB_PAD;

or something like that ?

Which driver triggers the bug ?

Re: [PATCH] netpoll: Fix skb tail pointer in netpoll_send_udp()

[Eric Dumazet]

On Tue, 2012-06-12 at 14:22 +0200, Eric Dumazet wrote:
> On Tue, 2012-06-12 at 13:26 +0300, Bogdan Hamciuc wrote:
> > As skb->tail wasn't updated after skb_copy_to_linear_data(), subsequent
> > calls to skb_realloc_headroom() (as made by an ethernet driver's
> > ndo_start_xmit routine) would only effectively copy the packet headers,
> > leaving garbage in the payload.
> > 
> > In the process, removed some unnecessary code.
> > 
> > Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@freescale.com>
> > ---
> >  net/core/netpoll.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> > index 3d84fb9..9a08068 100644
> > --- a/net/core/netpoll.c
> > +++ b/net/core/netpoll.c
> > @@ -362,22 +362,22 @@ EXPORT_SYMBOL(netpoll_send_skb_on_dev);
> >  
> >  void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
> >  {
> > -	int total_len, eth_len, ip_len, udp_len;
> > +	int total_len, ip_len, udp_len;
> >  	struct sk_buff *skb;
> >  	struct udphdr *udph;
> >  	struct iphdr *iph;
> >  	struct ethhdr *eth;
> >  
> >  	udp_len = len + sizeof(*udph);
> > -	ip_len = eth_len = udp_len + sizeof(*iph);
> > -	total_len = eth_len + ETH_HLEN + NET_IP_ALIGN;
> > +	ip_len = udp_len + sizeof(*iph);
> > +	total_len = ip_len + ETH_HLEN + NET_IP_ALIGN;
> >  
> >  	skb = find_skb(np, total_len, total_len - len);
> >  	if (!skb)
> >  		return;
> >  
> >  	skb_copy_to_linear_data(skb, msg, len);
> > -	skb->len += len;
> > +	skb_put(skb, len);
> >  
> >  	skb_push(skb, sizeof(*udph));
> >  	skb_reset_transport_header(skb);
> 
> Hmm, real question is why skb_realloc_headroom() is even necessary...
> 
> I suspect we need to reserve more bytes.
> 
> 	total_len = ip_len + ETH_HLEN + NET_IP_ALIGN + NET_SKB_PAD;
> 
> or something like that ?
> 
> Which driver triggers the bug ?
> 

Please try the following :

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 3d84fb9..1c6fb72 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -362,17 +362,18 @@ EXPORT_SYMBOL(netpoll_send_skb_on_dev);
 
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 {
-	int total_len, eth_len, ip_len, udp_len;
+	int total_len, ip_len, udp_len;
 	struct sk_buff *skb;
 	struct udphdr *udph;
 	struct iphdr *iph;
 	struct ethhdr *eth;
 
 	udp_len = len + sizeof(*udph);
-	ip_len = eth_len = udp_len + sizeof(*iph);
-	total_len = eth_len + ETH_HLEN + NET_IP_ALIGN;
+	ip_len = udp_len + sizeof(*iph);
+	total_len = ip_len + LL_RESERVED_SPACE(np->dev);
 
-	skb = find_skb(np, total_len, total_len - len);
+	skb = find_skb(np, total_len + np->dev->needed_tailroom,
+		       total_len - len);
 	if (!skb)
 		return;
 

Re: [PATCH] netpoll: Fix skb tail pointer in netpoll_send_udp()

[Eric Dumazet]

On Tue, 2012-06-12 at 15:15 +0200, Eric Dumazet wrote:
> On Tue, 2012-06-12 at 14:22 +0200, Eric Dumazet wrote:

> > Hmm, real question is why skb_realloc_headroom() is even necessary...
> > 
> > I suspect we need to reserve more bytes.
> > 
> > 	total_len = ip_len + ETH_HLEN + NET_IP_ALIGN + NET_SKB_PAD;
> > 
> > or something like that ?
> > 
> > Which driver triggers the bug ?
> > 

In case you wonder why I try so hard to avoid the
skb_realloc_headroom() :

netpoll has complicated^Wspecial^Wnice skb cache, to make sure it can
work even if memory is exhausted.

But if we trigger skb_realloc_headroom() the whole thing is useless. 

Thanks

RE: [PATCH] netpoll: Fix skb tail pointer in netpoll_send_udp()

[Hamciuc Bogdan-BHAMCIU1]

Hi Eric,

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Tuesday, June 12, 2012 4:34 PM
> To: Hamciuc Bogdan-BHAMCIU1
> Cc: davem@davemloft.net; netdev@vger.kernel.org
> Subject: Re: [PATCH] netpoll: Fix skb tail pointer in netpoll_send_udp()
> 
> On Tue, 2012-06-12 at 15:15 +0200, Eric Dumazet wrote:
> > On Tue, 2012-06-12 at 14:22 +0200, Eric Dumazet wrote:
> 
> > > Hmm, real question is why skb_realloc_headroom() is even necessary...
Our driver (Freescale P4080, unfortunately not upstream yet) needs a minimum amount of headroom in order to communicate metadata to the NIC.
For instance, frame offsets to the protocol headers are stored there, which the NIC then uses to fill in the egress checksum.
Originating frames, on the other hand, don't always have that amount of headroom, so we occasionally need to realloc. 
> > >
> > > I suspect we need to reserve more bytes.
> > >
> > > 	total_len = ip_len + ETH_HLEN + NET_IP_ALIGN + NET_SKB_PAD;
> > >
> > > or something like that ?
Indeed, your counter-proposal patch (adding LL_RESERVED_SPACE()) worked fine.

> > >
> > > Which driver triggers the bug ?
> > >
> 
> In case you wonder why I try so hard to avoid the
> skb_realloc_headroom() :
> 
> netpoll has complicated^Wspecial^Wnice skb cache, to make sure it can
> work even if memory is exhausted.
> 
> But if we trigger skb_realloc_headroom() the whole thing is useless.
> 
Thank you,
Bogdan