linux-kernel - Re: [PATCH net 1/3] net: hns: Add TX CSUM check when fill TX description

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Jul 2017 16:28:13 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     David Miller <davem@...emloft.net>
CC:     <huangdaode@...ilicon.com>, <xuwei5@...ilicon.com>,
        <liguozhu@...ilicon.com>, <Yisen.Zhuang@...wei.com>,
        <gabriele.paoloni@...wei.com>, <john.garry@...wei.com>,
        <linuxarm@...wei.com>, <salil.mehta@...wei.com>,
        <lipeng321@...wei.com>, <tremyfr@...il.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net 1/3] net: hns: Add TX CSUM check when fill TX
 description

Hi, David

On 2017/7/4 18:28, David Miller wrote:
> From: Lin Yun Sheng <linyunsheng@...wei.com>
> Date: Tue, 4 Jul 2017 18:47:31 +0800
> 
>> From: Yunsheng Lin <linyunsheng@...wei.com>
>>
>> If driver support checksum offload, should check netdev feature
>> before fill TX description and get CSUM err bit from RX
>> description. HNS driver do the check in RX derction but it doesn't
>> do the check in TX direction.
>>
>> Signed-off-by: lipeng <lipeng321@...wei.com>
>> Reviewed-by: Daode Huang <huangdaode@...ilicon.com>
>> Reviewed-by: Yunsheng Lin <linyunsheng@...wei.com>
> 
> This is not correct.
> 
> You should be checking the skb->checksum field to decide if you should
> offload the TX checksum of the packet or not.
> 
> Correct drivers, as far as I am aware, do not check the feature flags
> so I wonder where you got this idea from.  Always use other well
> established existing drivers as a model for how to handle things like
> this.
> 
> And this makes sense.  An SKB can have it's checksumming determination
> made first, then the netdev feature change is made afterwards.  For
> correctness you still need to TX checksum offload that SKB otherwise
> it will be emitted without a correctly computed checksum.
> 
You are right, thanks for pointing out.
driver/net/ethernet/hisilicon/hns/hns_enet.c
fill_desc:
		if (skb->ip_summed == CHECKSUM_PARTIAL) {
			protocol = skb->protocol;
			ip_offset = ETH_HLEN;

			/*if it is a SW VLAN check the next protocol*/
			if (protocol == htons(ETH_P_8021Q)) {
				ip_offset += VLAN_HLEN;
				protocol = vlan_get_protocol(skb);
				skb->protocol = protocol;
			}

			if (skb->protocol == htons(ETH_P_IP)) {
				flag_ipoffset |= 1 << HNS_TXD_L3CS_B;
				/* check for tcp/udp header */
				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;

			} else if (skb->protocol == htons(ETH_P_IPV6)) {
				/* ipv6 has not l3 cs, check for L4 header */
				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;
			}

			flag_ipoffset |= ip_offset << HNS_TXD_IPOFFSET_S;
		}

the hns driver has already check the skb->ip_summed, and checking the ndev->feature is
not correct.

Will remove this patch next revision.

Best Regards
Yunsheng Lin