[PATCH] ehea: Fix a checksum issue on the receive path

[PATCH] ehea: Fix a checksum issue on the receive path

[leitao]

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

Re: [PATCH] ehea: Fix a checksum issue on the receive path

[Eric Dumazet]

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 !

Re: [PATCH] ehea: Fix a checksum issue on the receive path

[Breno Leitao]

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

Re: [PATCH] ehea: Fix a checksum issue on the receive path

[Eric Dumazet]

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.

Re: [PATCH] ehea: Fix a checksum issue on the receive path

[David Miller]

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.

Re: [PATCH] ehea: Fix a checksum issue on the receive path

[Stephen Hemminger]

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.

Re: [PATCH] ehea: Fix a checksum issue on the receive path

[Breno Leitao]

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

Re: [PATCH] ehea: Fix a checksum issue on the receive path

[David Miller]

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.