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: <08cb8dd7-5685-4a41-b6a9-c8758a804ed1@wanadoo.fr>
Date: Fri, 25 Apr 2025 00:08:54 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Felix Maurer <fmaurer@...hat.com>
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/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:


  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.



Yours sincerely,
Vincent Mailhol