netdev - Re: [PATCH net-next] selftests: drv-net: ping: make sure the ping test restores checksum offload

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]

Message-ID: <d8693a9b-697a-49d7-a064-fc7349ec2d63@davidwei.uk>
Date: Thu, 8 May 2025 21:10:13 -0700
From: David Wei <dw@...idwei.uk>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 shuah@...nel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next] selftests: drv-net: ping: make sure the ping
 test restores checksum offload

On 5/8/25 18:37, Jakub Kicinski wrote:
> On Thu, 8 May 2025 14:59:12 -0700 David Wei wrote:
>>> +def _schedule_checksum_reset(cfg, netnl) -> None:
>>> +    features = ethtool(f"-k {cfg.ifname}", json=True)
>>> +    setting = ""
>>> +    for side in ["tx", "rx"]:
>>> +        f = features[0][side + "-checksumming"]
>>> +        if not f["fixed"]:
>>
>> I checked and found that "fixed" is a ternary:
>>
>>           "rx-checksumming": {
>>               "active": true,
>>               "fixed": false,
>>               "requested": true
>>           },
>>           "tx-checksumming": {
>>               "active": true,
>>               "fixed": null,
>>               "requested": null
>>           },
>>
>> Python loads this JSON as False and None types respectively, and `not
>> f["fixed"]` is true for both False and None. Maybe this doesn't matter
>> but flagging it.
> 
> I think so, yes.
> 
>>> +            setting += " " + side
>>> +            setting += " " + ("on" if f["requested"] or f["active"] else "off")
>>> +    defer(ethtool, f" -K {cfg.ifname} " + setting)
>>
>> This does rx/tx-gro too even if not explicitly requested. I assume that
>> is okay?
> 
> You mean because those are automatically updated when we change
> checksumming? If so then yes.

Thanks for responding.

Reviewed-by: David Wei <dw@...idwei.uk>