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, 3 Feb 2022 00:25:55 +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 02:10:15PM -0700, Shuah Khan wrote:
> On 2/2/22 1:16 PM, Guillaume Nault wrote:
> > 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?
> 
> This is for sure. Remove the invalid values in sync with the kernel code.
> 
> > Or to write two versions of the
> > test, one with invalid values, the other with correct ones?
> > 
> 
> This one makes sense if it adds value in testing to make sure we continue
> to reject invalid values.
> 
> > 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.
> > 
> 
> If kernel still accepts these values, then the test is valid as long as
> kernel still doesn't flag these values as invalid.
> 
> I might be missing something. Don't you want to test with invalid values
> so make sure they are indeed rejected?

Testing invalid values makes sense, but in another selftest IMHO. This
file is used to test hardware offload behaviour (although it lives
under selftests/net/, it's only called from other scripts living under
selftests/drivers/). Testing for accepted/rejected values should be
done in a network generic selftest, not in driver specific ones.

I'm currently working on a patch series that'd include such tests (as
part of a larger project aimed at fixing conflicting interpretations of
ECN bits). But for fib_offload_lib.sh, I'd really prefer if we could
keep it focused on testing driver features.

> 
> thanks,
> -- Shuah
> 

Powered by blists - more mailing lists