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 16:29:46 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Guillaume Nault <gnault@...hat.com>
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>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH net-next] selftests: fib offload: use sensible tos values

On 2/2/22 4:25 PM, Guillaume Nault wrote:
> 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.
> 

A separate test for invalid values makes sense. It will be easier to find
and report problems.

thanks,
-- Shuah