netdev - Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX checksum offloading

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:   Thu, 7 Jun 2018 11:43:15 -0500
From:   Jennifer Dahm <jennifer.dahm@...com>
To:     Nicolas Ferre <nicolas.ferre@...rochip.com>,
        netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>
Cc:     Nathan Sullivan <nathan.sullivan@...com>
Subject: Re: [RFC PATCH 1/2] net: macb: Add CAP to disable hardware TX
 checksum offloading

Hi Nicolas,

On 06/04/2018 10:13 AM, Nicolas Ferre wrote:
> On 25/05/2018 at 23:44, Jennifer Dahm wrote:
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 3e93df5..a5d564b 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3360,8 +3360,12 @@ static int macb_init(struct platform_device 
>> *pdev)
>>           dev->hw_features |= MACB_NETIF_LSO;
>>         /* Checksum offload is only available on gem with packet 
>> buffer */
>> -    if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
>> -        dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>> +    if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
>> +        if (!(bp->caps & MACB_CAPS_DISABLE_TX_HW_CSUM))
>
> Why not the other way around? negating a "disabled" feature is always 
> challenge ;-)
>
>> +            dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>> +        else
>> +            dev->hw_features |= NETIF_F_RXCSUM;
>> +    }
>>       if (bp->caps & MACB_CAPS_SG_DISABLED)
>>           dev->hw_features &= ~NETIF_F_SG;
>>       dev->features = dev->hw_features;

I can switch the ordering of the if-else clauses if that's what you're 
nitpicking. ;)

Alternatively, if you're asking why the flag is used to disable rather 
than enable checksum offloading: I was working under the assumption that 
this was an isolated bug, and so an opt-out would require less 
maintainance than an opt-in. If we discover that this is a problem 
across a wide variety of Cadence IP, it would definitely make sense to 
replace it with an opt-in (i.e. MACB_CAPS_TX_HW_CSUM_ENABLED).

Best,
Jennifer Dahm