| Message ID | 1286493453-21784-1-git-send-email-leitao@linux.vnet.ibm.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 ! -- 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
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 -- 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
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. -- 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: 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. -- 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 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. -- 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 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 -- 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: 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. -- 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
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