netdev - Re: [PATCH/RFT net] ravb: expand rx descriptor data to accommodate hw checksum

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]

Message-ID: <364a7abd-6050-eaa6-5dbe-1c2aaf31c0bc@cogentembedded.com>
Date:   Mon, 10 Dec 2018 22:51:15 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     David Miller <davem@...emloft.net>, horms+renesas@...ge.net.au
Cc:     magnus.damm@...il.com, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH/RFT net] ravb: expand rx descriptor data to accommodate hw
 checksum

On 12/10/2018 10:44 PM, David Miller wrote:

>> +#define RAVB_CSUM_LEN 2
>> +
>  ...
>>  	priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) +
>> -		ETH_HLEN + VLAN_HLEN;
>> +		ETH_HLEN + VLAN_HLEN + RAVB_CSUM_LEN;
>  ...
>> +	if (unlikely(skb->len < RAVB_CSUM_LEN))
>  ...
>> -	hw_csum = skb_tail_pointer(skb) - 2;
>> +	hw_csum = skb_tail_pointer(skb) - RAVB_CSUM_LEN;
>  ...
>> -	skb_trim(skb, skb->len - 2);
>> +	skb_trim(skb, skb->len - RAVB_CSUM_LEN);
> 
> Unlike Sergei, I think this macro define should be kept in the fix.
> 
> It is absolutely crucial for anyone reading this code to understand
> what this value is all about.
> 
> People reading the code aren't able to go automatically back to a
> commit to learn what this value means, and even if they could they
> shouldn't have to do so for a bunch of magic '2' constants placed all
> over.

   We already have a comment in ravb_tx_csum(), I only asked for another one
(the place where we fix up the packet size). 
 
> Even in the most fundamental way, the macro is required to satisfy
> the "no magic constants" rule for kernel code.

   I'm also somewhat opposed to the RAVB_ prefix on something not really h/w
or driver specific... but I guess we don't have such #define anywhere in the
TCP/IP stack. Well, your call, anyway...

MBR. Sergei