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

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

[Guillaume Nault]

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.

Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 .../selftests/net/forwarding/fib_offload_lib.sh      | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/fib_offload_lib.sh b/tools/testing/selftests/net/forwarding/fib_offload_lib.sh
index e134a5f529c9..1b3b46292179 100644
--- a/tools/testing/selftests/net/forwarding/fib_offload_lib.sh
+++ b/tools/testing/selftests/net/forwarding/fib_offload_lib.sh
@@ -99,15 +99,15 @@ fib_ipv4_tos_test()
 	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 0 metric 1024" false
 	check_err $? "Route not in hardware when should"
 
-	ip -n $ns route add 192.0.2.0/24 dev dummy1 tos 2 metric 1024
-	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 2 metric 1024" false
+	ip -n $ns route add 192.0.2.0/24 dev dummy1 tos 8 metric 1024
+	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 8 metric 1024" false
 	check_err $? "Highest TOS route not in hardware when should"
 
 	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 0 metric 1024" true
 	check_err $? "Lowest TOS route still in hardware when should not"
 
-	ip -n $ns route add 192.0.2.0/24 dev dummy1 tos 1 metric 1024
-	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 1 metric 1024" true
+	ip -n $ns route add 192.0.2.0/24 dev dummy1 tos 4 metric 1024
+	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 4 metric 1024" true
 	check_err $? "Middle TOS route in hardware when should not"
 
 	log_test "IPv4 routes with TOS"
@@ -277,11 +277,11 @@ fib_ipv4_replay_tos_test()
 	ip -n $ns link set dev dummy1 up
 
 	ip -n $ns route add 192.0.2.0/24 dev dummy1 tos 0
-	ip -n $ns route add 192.0.2.0/24 dev dummy1 tos 1
+	ip -n $ns route add 192.0.2.0/24 dev dummy1 tos 4
 
 	devlink -N $ns dev reload $devlink_dev
 
-	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 1" false
+	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 4" false
 	check_err $? "Highest TOS route not in hardware when should"
 
 	fib4_trap_check $ns "192.0.2.0/24 dev dummy1 tos 0" true
-- 
2.21.3

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

[Ido Schimmel]

On Wed, Feb 02, 2022 at 07:30:28PM +0100, 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.
> 
> Signed-off-by: Guillaume Nault <gnault@redhat.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Thanks

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

[Shuah Khan]

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.

thanks,
-- Shuah

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

[Guillaume Nault]

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
> 

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

[Shuah Khan]

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?

thanks,
-- Shuah

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

[Guillaume Nault]

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
> 

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

[Shuah Khan]

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 2 Feb 2022 19:30:28 +0100 you 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).
> 
> [...]

Here is the summary with links:
  - [net-next] selftests: fib offload: use sensible tos values
    https://git.kernel.org/netdev/net-next/c/bafe517af299

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html