netdev - Re: [PATCH 4/6] net/macb: add RX checksum offload feature

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] [day] [month] [year] [list]

Date:	Thu, 24 Jul 2014 13:59:21 +0200
From:	Cyrille Pitchen <cyrille.pitchen@...el.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	<nicolas.ferre@...el.com>, <davem@...emloft.net>,
	<linux-arm-kernel@...ts.infradead.org>, <netdev@...r.kernel.org>,
	<soren.brinkmann@...inx.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/6] net/macb: add RX checksum offload feature

Le 23/07/2014 00:58, Eric Dumazet a écrit :
> On Tue, 2014-07-22 at 18:27 +0200, Cyrille Pitchen wrote:
>> Le 18/07/2014 17:36, Eric Dumazet a écrit :
>>> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@...el.com>
>>>> ---
>>>>  drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
>>>>  drivers/net/ethernet/cadence/macb.h |  2 ++
>>>>  2 files changed, 30 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>>>> index 9bdcd1b..6acd6e2 100644
>>>> --- a/drivers/net/ethernet/cadence/macb.c
>>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
>>>>  
>>>>  		skb->protocol = eth_type_trans(skb, bp->dev);
>>>>  		skb_checksum_none_assert(skb);
>>>> +		if (bp->dev->features & NETIF_F_RXCSUM)
>>>> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>>  
>>>
>>>
>>> Really ?
>>>
>>> If this is what you meant, this deserves a big and fat comment.
>>>
>>>
>>>
>> Hi Eric,
>>
>> Isn't it the proper way to do? According to Cadence documentation
>> about RX checksum offload:
>> "If any of the checksums (IP, TCP or UDP) are verified incorrect by
>> the GEM, the packet is discarded."
>>
>> If I understand, when RX checksum offload is enabled setting bit 24 of
>> the Network Configuration Register, the driver only receives RX frames
>> with correct checksum. Then it tells the kernel that there's no need
>> to verify the checksum again in software. This is done setting
>> skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the
>> same in e1000_rx_checksum() function.
>>
>> Also bit 24 of the Network Configuration Register is updated by
>> macb_open() and macb_set_features() so it always matches the state of
>> NETIF_F_RXCSUM flag in dev->features, once the network interface is
>> up. That's why I'd rather read from dev->features than read from
>> register.
>>
>> Did I make a mistake? Is it the kind of comment you expect to be
>> added?
>>
>> Regards,
> 
> Usually, frames are not discarded, and a bit is present in rx descriptor
> to tell us if (TCP) checksum was validated by the NIC
> 
> We want tcpdump to get a copy of corrupted TCP frames, even if TCP stack
> drops them later.
> 
> If this NIC drops incorrect frames, I am afraid we cant use this RX
> offload at all.
> 
> 
> 
OK thanks for your comments. In the next series of patches I'll allow RX 
checksum offload only when promiscuous mode is disabled. Also I'll check the
status returned by the GEM through the buffer descriptor.

Regards,

Cyrille
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html