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

[Guillaume Nault <gnault@redhat.com>]

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
>