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] [day] [month] [year] [list]

Message-ID: <69d35507-f4aa-484c-8e1f-f2a766b4ffd1@redhat.com>
Date: Wed, 30 Apr 2025 11:36:35 +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 17:08, Vincent Mailhol wrote:
> On 24/04/2025 at 23:02, Felix Maurer wrote:
>> 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.
> 
> OK. Now I understand.
> 
>> 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,
> 
> This is better. But yet, how would this scale in the future if we introduce the
> CAN FD? For n flags, you have n combinations.
> 
>> 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?
> 
> The
> 
>   >> 28
> 
> trick just allows to save a couple line but by doing so, adds a ton of
> complexity. What is wrong in writing this:

I don't see anything wrong with it, I like it :) I'll send an updated
version of the patches soon (probably squashed as well).

>   FIXTURE_VARIANT(can_filters) {
>   	int testcase;
>   	canid_t id;
>   	canid_t mask;
>   	int exp_num_rx;
>   	canid_t exp_flags[];
>   };
> 
>   /* Receive all frames when filtering for the ID in standard frame format */
>   FIXTURE_VARIANT_ADD(can_filters, base) {
>   	.testcase = 1,
>   	.id = ID,
>   	.mask = CAN_SFF_MASK,
>   	.exp_num_rx = 4,
>   	.exp_flags = {
>   		0,
>   		CAN_EFF_FLAG,
>   		CAN_RTR_FLAG,
>   		CAN_EFF_FLAG | CAN_RTR_FLAG,
>   	},
>   };
> 
> And then, in your TEST_F(), the do {} while loops becomes a:
> 
>   for (int i = 0; i <= variant->exp_num_rx; i++) {
>   	/* FD logic here */
>   	ret = FD_ISSET(self->sock, &rdfs);
> 	if (i == variant->exp_num_rx) {
>   		ASSERT_EQ(ret == 0);
>   	} else (i < variant->exp_num_rx)
>   		/* other relevant checks */
>   		ASSERT_EQ(frame.can_id & ~CAN_ERR_MASK ==
>   		          variant->exp_flags[i]);
>   	}
>   }
> 
> Here, you even check that the frames are received in order.
> 
> OK, the bitmap saved some memory, but here, we are speaking of selftests. The
> priority is readability. I will happily get rid of the bitmap and just simplify
> the logic.

I fully agree, thank you!
   Felix