netdev - Re: [PATCH net-next] selftests: fib offload: use sensible tos values

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, 2 Feb 2022 21:16:14 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        Ido Schimmel <idosch@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next] selftests: fib offload: use sensible tos values

On Wed, Feb 02, 2022 at 12:46:10PM -0700, Shuah Khan wrote:
> On 2/2/22 11:30 AM, Guillaume Nault wrote:
> > Although both iproute2 and the kernel accept 1 and 2 as tos values for
> > new routes, those are invalid. These values only set ECN bits, which
> > are ignored during IPv4 fib lookups. Therefore, no packet can actually
> > match such routes. This selftest therefore only succeeds because it
> > doesn't verify that the new routes do actually work in practice (it
> > just checks if the routes are offloaded or not).
> > 
> > It makes more sense to use tos values that don't conflict with ECN.
> > This way, the selftest won't be affected if we later decide to warn or
> > even reject invalid tos configurations for new routes.
> 
> Wouldn't it make sense to leave these invalid values in the test though.
> Removing these makes this test out of sync withe kernel.

Do you mean keeping the test as is and only modify it when (if) we
decide to reject such invalid values? Or to write two versions of the
test, one with invalid values, the other with correct ones?

I don't get what keeping a test with the invalid values could bring us.
It's confusing for the reader, and might break in the future. This
patch makes the test future proof, without altering its intent and code
coverage. It still works on current (and past) kernels, so I don't see
what this patch could make out of sync.

Or did I misunderstand something?

> thanks,
> -- Shuah
>