netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ehea: Fix a checksum issue on the receive path
@ 2010-10-07 23:17 leitao
  2010-10-08  4:45 ` Eric Dumazet
  0 siblings, 1 reply; 8+ messages in thread
From: leitao @ 2010-10-07 23:17 UTC (permalink / raw)
  To: davem; +Cc: netdev, Breno Leitao, Jay Vosburgh

Currently we set all skbs with CHECKSUM_UNNECESSARY, even
those whose protocol we don't know. This patch just
add the CHECKSUM_COMPLETE tag for non TCP/UDP packets.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 drivers/net/ehea/ehea_main.c |    9 ++++++++-
 drivers/net/ehea/ehea_qmr.h  |    1 +
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ehea/ehea_main.c b/drivers/net/ehea/ehea_main.c
index 0471cae..45fd045 100644
--- a/drivers/net/ehea/ehea_main.c
+++ b/drivers/net/ehea/ehea_main.c
@@ -533,8 +533,15 @@ static inline void ehea_fill_skb(struct net_device *dev,
 	int length = cqe->num_bytes_transfered - 4;	/*remove CRC */
 
 	skb_put(skb, length);
-	skb->ip_summed = CHECKSUM_UNNECESSARY;
 	skb->protocol = eth_type_trans(skb, dev);
+
+	/* The packet was not an IPV4 packet so a complemented checksum was
+	   calculated. The value is found in the Internet Checksum field. */
+	if (cqe->status & EHEA_CQE_BLIND_CKSUM) {
+		skb->ip_summed = CHECKSUM_COMPLETE;
+		skb->csum = csum_unfold(~cqe->inet_checksum_value);
+	} else
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
 }
 
 static inline struct sk_buff *get_skb_by_index(struct sk_buff **skb_array,
diff --git a/drivers/net/ehea/ehea_qmr.h b/drivers/net/ehea/ehea_qmr.h
index f608a6c..3810473 100644
--- a/drivers/net/ehea/ehea_qmr.h
+++ b/drivers/net/ehea/ehea_qmr.h
@@ -150,6 +150,7 @@ struct ehea_rwqe {
 #define EHEA_CQE_TYPE_RQ           0x60
 #define EHEA_CQE_STAT_ERR_MASK     0x700F
 #define EHEA_CQE_STAT_FAT_ERR_MASK 0xF
+#define EHEA_CQE_BLIND_CKSUM       0x8000
 #define EHEA_CQE_STAT_ERR_TCP      0x4000
 #define EHEA_CQE_STAT_ERR_IP       0x2000
 #define EHEA_CQE_STAT_ERR_CRC      0x1000
-- 
1.6.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ehea: Fix a checksum issue on the receive path
  2010-10-07 23:17 [PATCH] ehea: Fix a checksum issue on the receive path leitao
@ 2010-10-08  4:45 ` Eric Dumazet
  2010-10-08 14:14   ` Breno Leitao
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2010-10-08  4:45 UTC (permalink / raw)
  To: leitao; +Cc: davem, netdev, Jay Vosburgh

Le jeudi 07 octobre 2010 à 19:17 -0400, leitao@linux.vnet.ibm.com a
écrit :
> Currently we set all skbs with CHECKSUM_UNNECESSARY, even
> those whose protocol we don't know. This patch just
> add the CHECKSUM_COMPLETE tag for non TCP/UDP packets.
> 
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
> ---
>  drivers/net/ehea/ehea_main.c |    9 ++++++++-
>  drivers/net/ehea/ehea_qmr.h  |    1 +
>  2 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ehea/ehea_main.c b/drivers/net/ehea/ehea_main.c
> index 0471cae..45fd045 100644
> --- a/drivers/net/ehea/ehea_main.c
> +++ b/drivers/net/ehea/ehea_main.c
> @@ -533,8 +533,15 @@ static inline void ehea_fill_skb(struct net_device *dev,
>  	int length = cqe->num_bytes_transfered - 4;	/*remove CRC */
>  
>  	skb_put(skb, length);
> -	skb->ip_summed = CHECKSUM_UNNECESSARY;
>  	skb->protocol = eth_type_trans(skb, dev);
> +
> +	/* The packet was not an IPV4 packet so a complemented checksum was
> +	   calculated. The value is found in the Internet Checksum field. */
> +	if (cqe->status & EHEA_CQE_BLIND_CKSUM) {
> +		skb->ip_summed = CHECKSUM_COMPLETE;
> +		skb->csum = csum_unfold(~cqe->inet_checksum_value);
> +	} else
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
>  }
>  

Hi Breno

Just to be clear : packets with wrong checksums are not given to upper
stack, so a tcpdump can not display them ? I am not sure many drivers do
that.

(EHEA_CQE_STAT_ERR_TCP, EHEA_CQE_STAT_ERR_IP)

Thanks !



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ehea: Fix a checksum issue on the receive path
  2010-10-08  4:45 ` Eric Dumazet
@ 2010-10-08 14:14   ` Breno Leitao
  2010-10-08 14:36     ` Eric Dumazet
  0 siblings, 1 reply; 8+ messages in thread
From: Breno Leitao @ 2010-10-08 14:14 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, netdev, Jay Vosburgh

Hi Eric

On 10/08/2010 01:45 AM, Eric Dumazet wrote:
> Just to be clear : packets with wrong checksums are not given to upper
> stack, so a tcpdump can not display them ? I am not sure many drivers do
> that.
Well, what my code does is: 1) if the current packet is a UDP/TCP, then 
the checksum is not necessary, since we would check the checksum on 
ehea_proc_rwqes(), specific at this part of the code:

                if (!ehea_check_cqe(cqe, &rq)) {
			// Send the packet to the up layers

And ehea_check_cqe() checks for wrong checksumed packets on:
	
         if ((cqe->status & EHEA_CQE_STAT_ERR_MASK) == 0)
                 return 0;


Botton line, TCP/UDP packets with wrong checksums are dropped by 
ehea_proc_rwqes(), others go to the up layer.

So, back to your question, you are saying that we shouldn't do that, 
meaning that we should send to the upper layers all packets ? even those 
that have the wrong checksum ?

Thanks
Breno

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ehea: Fix a checksum issue on the receive path
  2010-10-08 14:14   ` Breno Leitao
@ 2010-10-08 14:36     ` Eric Dumazet
  2010-10-09 16:20       ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2010-10-08 14:36 UTC (permalink / raw)
  To: Breno Leitao; +Cc: davem, netdev, Jay Vosburgh

Le vendredi 08 octobre 2010 à 11:14 -0300, Breno Leitao a écrit :
> Hi Eric
> 
> On 10/08/2010 01:45 AM, Eric Dumazet wrote:
> > Just to be clear : packets with wrong checksums are not given to upper
> > stack, so a tcpdump can not display them ? I am not sure many drivers do
> > that.
> Well, what my code does is: 1) if the current packet is a UDP/TCP, then 
> the checksum is not necessary, since we would check the checksum on 
> ehea_proc_rwqes(), specific at this part of the code:
> 
>                 if (!ehea_check_cqe(cqe, &rq)) {
> 			// Send the packet to the up layers
> 
> And ehea_check_cqe() checks for wrong checksumed packets on:
> 	
>          if ((cqe->status & EHEA_CQE_STAT_ERR_MASK) == 0)
>                  return 0;
> 
> 
> Botton line, TCP/UDP packets with wrong checksums are dropped by 
> ehea_proc_rwqes(), others go to the up layer.
> 
> So, back to your question, you are saying that we shouldn't do that, 
> meaning that we should send to the upper layers all packets ? even those 
> that have the wrong checksum ?
> 

I am pretty sure most (if not all) netdev drivers pass the packet with
invalid checksum to upper stack, so that we can increment appropriate
SNMP counters, in IP stack or UDP/TCP/whatever stack.

tg3, bnx2, e1000, skge, sky2, bnx2x, niu, r8169, igb, ... seems to do
that.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ehea: Fix a checksum issue on the receive path
  2010-10-08 14:36     ` Eric Dumazet
@ 2010-10-09 16:20       ` David Miller
  2010-10-09 17:31         ` Stephen Hemminger
  0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2010-10-09 16:20 UTC (permalink / raw)
  To: eric.dumazet; +Cc: leitao, netdev, fubar

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 08 Oct 2010 16:36:17 +0200

> I am pretty sure most (if not all) netdev drivers pass the packet with
> invalid checksum to upper stack, so that we can increment appropriate
> SNMP counters, in IP stack or UDP/TCP/whatever stack.
> 
> tg3, bnx2, e1000, skge, sky2, bnx2x, niu, r8169, igb, ... seems to do
> that.

Drivers _must_ send up all packets, even those with bad checksums,
without exception.

Otherwise protocol statistics get lost, netfilter log entries go
missing, etc.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ehea: Fix a checksum issue on the receive path
  2010-10-09 16:20       ` David Miller
@ 2010-10-09 17:31         ` Stephen Hemminger
  2010-10-13 14:06           ` Breno Leitao
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2010-10-09 17:31 UTC (permalink / raw)
  To: David Miller; +Cc: eric.dumazet, leitao, netdev, fubar

On Sat, 09 Oct 2010 09:20:43 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 08 Oct 2010 16:36:17 +0200
> 
> > I am pretty sure most (if not all) netdev drivers pass the packet with
> > invalid checksum to upper stack, so that we can increment appropriate
> > SNMP counters, in IP stack or UDP/TCP/whatever stack.
> > 
> > tg3, bnx2, e1000, skge, sky2, bnx2x, niu, r8169, igb, ... seems to do
> > that.
> 
> Drivers _must_ send up all packets, even those with bad checksums,
> without exception.
> 
> Otherwise protocol statistics get lost, netfilter log entries go
> missing, etc.

Also hardware checksum can be wrong/broken. By passing up a packet
which the driver thinks is bad, the software can still work.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ehea: Fix a checksum issue on the receive path
  2010-10-09 17:31         ` Stephen Hemminger
@ 2010-10-13 14:06           ` Breno Leitao
  2010-10-13 21:25             ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Breno Leitao @ 2010-10-13 14:06 UTC (permalink / raw)
  To: David Miller; +Cc: Stephen Hemminger, eric.dumazet, netdev, fubar

On 10/09/2010 02:31 PM, Stephen Hemminger wrote:
> Also hardware checksum can be wrong/broken. By passing up a packet
> which the driver thinks is bad, the software can still work.
I see. Unfortunately ehea driver is dropping the packets that have a 
wrong checksum. I will work on the fix, and soon I will send the patch.

David,

Just to clarify, this patch that started this thead is not invalidated 
by this "problem". So, I'd like to see this patch committed on your 
tree. Does it make sense?

Thanks
Breno

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ehea: Fix a checksum issue on the receive path
  2010-10-13 14:06           ` Breno Leitao
@ 2010-10-13 21:25             ` David Miller
  0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2010-10-13 21:25 UTC (permalink / raw)
  To: leitao; +Cc: shemminger, eric.dumazet, netdev, fubar

From: Breno Leitao <leitao@linux.vnet.ibm.com>
Date: Wed, 13 Oct 2010 11:06:14 -0300

> Just to clarify, this patch that started this thead is not invalidated
> by this "problem". So, I'd like to see this patch committed on your
> tree. Does it make sense?

Yep, I've added the fix to net-2.6, thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2010-10-13 21:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-07 23:17 [PATCH] ehea: Fix a checksum issue on the receive path leitao
2010-10-08  4:45 ` Eric Dumazet
2010-10-08 14:14   ` Breno Leitao
2010-10-08 14:36     ` Eric Dumazet
2010-10-09 16:20       ` David Miller
2010-10-09 17:31         ` Stephen Hemminger
2010-10-13 14:06           ` Breno Leitao
2010-10-13 21:25             ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).