netdev - Re: [PATCH 4/4] selftests: can: Document test_raw

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]

Message-ID: <a28ff624-c73a-4e16-867a-66e423315c29@redhat.com>
Date: Thu, 24 Apr 2025 16:02:19 +0200
From: Felix Maurer <fmaurer@...hat.com>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: socketcan@...tkopp.net, mkl@...gutronix.de, shuah@...nel.org,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, horms@...nel.org, linux-can@...r.kernel.org,
 netdev@...r.kernel.org, linux-kselftest@...r.kernel.org,
 dcaratti@...hat.com, fstornio@...hat.com
Subject: Re: [PATCH 4/4] selftests: can: Document test_raw_filter test cases

On 24.04.25 09:44, Vincent Mailhol wrote:
> On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <fmaurer@...hat.com> wrote:
[...]
>> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
>> index 7fe11e020a1c..8d43053824d2 100644
>> --- a/tools/testing/selftests/net/can/test_raw_filter.c
>> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
>> @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) {
>>         int exp_num_rx;
>>         int exp_rxbits;
>>  };
>> +#define T_EFF (CAN_EFF_FLAG >> 28)
>> +#define T_RTR (CAN_RTR_FLAG >> 28)
> 
> I do not like this
> 
>   >> 28
> 
> shift. I understand that it is part of the original design, but for
> me, this is just obfuscation.
> 
> Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the
> expected values? What benefit does this shift add?

I agree, that looks like magic numbers and the original design is not
very nice here. The main reason for the >>28 is that later on values are
shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
>>28, the shift value later is in the range 0-14). See below for a
slightly different idea.

>> +/* Ignore EFF flag in filter ID if not covered by filter mask */
>>  FIXTURE_VARIANT_ADD(can_filters, base_eff) {
>>         .testcase = 2,
>>         .id = ID | CAN_EFF_FLAG,
>>         .mask = CAN_SFF_MASK,
>>         .exp_num_rx = 4,
>> -       .exp_rxbits = 4369,
>> +       .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
>                          ^
> What is the meaning of this 1?

The 1 means that a packet will be received with no flags set.

The whole rxbit thing took me a while to understand and the result now
is not straightforward either. Let's see if we can come up with
something better.

The exp_rxbits is basically a bitfield that describes which flags should
be set on the received frames. Maybe this could be made more explicit
with something like this:

.exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,

And in the receive loop something like this:

rxbits |= FRAME_RCVD(frame.can_id);

Of course, the definitions of these macros would still have the >>28,
but at a central point, with better explanation. Do you think that's
more understandable? Or do you have a different idea?

Thanks,
   Felix