[v2] can: netlink: preparation before introduction of CAN XL step 2/2

[PATCH v2 04/20] can: netlink: refactor can_validate_bittiming()

Posted by Vincent Mailhol 3 weeks, 1 day ago

Whenever can_validate_bittiming() is called, it is always preceded by
some boilerplate code which was copy pasted all over the place. Move
that repeated code directly inside can_validate_bittiming().

Finally, the mempcy() is not needed. Just use the pointer returned by
nla_data() as-is.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index d9f6ab3efb9767409c318b714f19df8a30e51137..bc91df8d75ac41381fefea895d7e490a965d3f7b 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -36,13 +36,20 @@ static const struct nla_policy can_tdc_policy[IFLA_CAN_TDC_MAX + 1] = {
 	[IFLA_CAN_TDC_TDCF] = { .type = NLA_U32 },
 };
 
-static int can_validate_bittiming(const struct can_bittiming *bt,
-				  struct netlink_ext_ack *extack)
+static int can_validate_bittiming(struct nlattr *data[],
+				  struct netlink_ext_ack *extack,
+				  int ifla_can_bittiming)
 {
+	struct can_bittiming *bt;
+
+	if (!data[ifla_can_bittiming])
+		return 0;
+
+	bt = nla_data(data[ifla_can_bittiming]);
+
 	/* sample point is in one-tenth of a percent */
 	if (bt->sample_point >= 1000) {
 		NL_SET_ERR_MSG(extack, "sample point must be between 0 and 100%");
-
 		return -EINVAL;
 	}
 
@@ -105,14 +112,9 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 		}
 	}
 
-	if (data[IFLA_CAN_BITTIMING]) {
-		struct can_bittiming bt;
-
-		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
-		err = can_validate_bittiming(&bt, extack);
-		if (err)
-			return err;
-	}
+	err = can_validate_bittiming(data, extack, IFLA_CAN_BITTIMING);
+	if (err)
+		return err;
 
 	if (is_can_fd) {
 		if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING])
@@ -124,14 +126,9 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			return -EOPNOTSUPP;
 	}
 
-	if (data[IFLA_CAN_DATA_BITTIMING]) {
-		struct can_bittiming bt;
-
-		memcpy(&bt, nla_data(data[IFLA_CAN_DATA_BITTIMING]), sizeof(bt));
-		err = can_validate_bittiming(&bt, extack);
-		if (err)
-			return err;
-	}
+	err = can_validate_bittiming(data, extack, IFLA_CAN_DATA_BITTIMING);
+	if (err)
+		return err;
 
 	return 0;
 }

-- 
2.49.1

Re: [PATCH v2 04/20] can: netlink: refactor can_validate_bittiming()

Posted by Marc Kleine-Budde 3 weeks, 1 day ago

On 10.09.2025 15:03:29, Vincent Mailhol wrote:
> Whenever can_validate_bittiming() is called, it is always preceded by
> some boilerplate code which was copy pasted all over the place. Move
> that repeated code directly inside can_validate_bittiming().
> 
> Finally, the mempcy() is not needed. Just use the pointer returned by
> nla_data() as-is.

The memcpy()'ed struct is guaranteed to be properly aligned, is this
also the case for the casted nla_data() pointer?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: [PATCH v2 04/20] can: netlink: refactor can_validate_bittiming()

Posted by Vincent Mailhol 3 weeks, 1 day ago

On 10/09/2025 at 15:13, Marc Kleine-Budde wrote:
> On 10.09.2025 15:03:29, Vincent Mailhol wrote:
>> Whenever can_validate_bittiming() is called, it is always preceded by
>> some boilerplate code which was copy pasted all over the place. Move
>> that repeated code directly inside can_validate_bittiming().
>>
>> Finally, the mempcy() is not needed. Just use the pointer returned by
>> nla_data() as-is.
> 
> The memcpy()'ed struct is guaranteed to be properly aligned, is this
> also the case for the casted nla_data() pointer?

The NLA attributes are aligned on 4 bytes, c.f. NLA_ALIGNTO:

https://elixir.bootlin.com/linux/v6.16.5/source/include/uapi/linux/netlink.h#L248

Which is sufficient for struct can_bittiming which also requires just 4 bytes of
alignment as proven by the fact the the code would still compile if I add this
static assert:

  static_assert(_Alignof(typeof(*bt)) <= NLA_ALIGNTO);

But I have to admit that you caught me off guard. I did not think of that. Maybe
I should add above static assertions to the code to document that what we are
doing is correct?


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 04/20] can: netlink: refactor can_validate_bittiming()

Posted by Marc Kleine-Budde 3 weeks, 1 day ago

On 10.09.2025 15:43:00, Vincent Mailhol wrote:
> On 10/09/2025 at 15:13, Marc Kleine-Budde wrote:
> > On 10.09.2025 15:03:29, Vincent Mailhol wrote:
> >> Whenever can_validate_bittiming() is called, it is always preceded by
> >> some boilerplate code which was copy pasted all over the place. Move
> >> that repeated code directly inside can_validate_bittiming().
> >>
> >> Finally, the mempcy() is not needed. Just use the pointer returned by
> >> nla_data() as-is.
> > 
> > The memcpy()'ed struct is guaranteed to be properly aligned, is this
> > also the case for the casted nla_data() pointer?
> 
> The NLA attributes are aligned on 4 bytes, c.f. NLA_ALIGNTO:
> 
> https://elixir.bootlin.com/linux/v6.16.5/source/include/uapi/linux/netlink.h#L248
> 
> Which is sufficient for struct can_bittiming which also requires just 4 bytes of
> alignment as proven by the fact the the code would still compile if I add this
> static assert:
> 
>   static_assert(_Alignof(typeof(*bt)) <= NLA_ALIGNTO);
> 
> But I have to admit that you caught me off guard. I did not think of that. Maybe
> I should add above static assertions to the code to document that what we are
> doing is correct?

Yes, make it so!

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Re: [PATCH v2 04/20] can: netlink: refactor can_validate_bittiming()

Posted by Vincent Mailhol 3 weeks, 1 day ago

On 10/09/2025 at 19:55, Marc Kleine-Budde wrote:
> On 10.09.2025 15:43:00, Vincent Mailhol wrote:
>> On 10/09/2025 at 15:13, Marc Kleine-Budde wrote:
>>> On 10.09.2025 15:03:29, Vincent Mailhol wrote:
>>>> Whenever can_validate_bittiming() is called, it is always preceded by
>>>> some boilerplate code which was copy pasted all over the place. Move
>>>> that repeated code directly inside can_validate_bittiming().
>>>>
>>>> Finally, the mempcy() is not needed. Just use the pointer returned by
>>>> nla_data() as-is.
>>>
>>> The memcpy()'ed struct is guaranteed to be properly aligned, is this
>>> also the case for the casted nla_data() pointer?
>>
>> The NLA attributes are aligned on 4 bytes, c.f. NLA_ALIGNTO:
>>
>> https://elixir.bootlin.com/linux/v6.16.5/source/include/uapi/linux/netlink.h#L248
>>
>> Which is sufficient for struct can_bittiming which also requires just 4 bytes of
>> alignment as proven by the fact the the code would still compile if I add this
>> static assert:
>>
>>   static_assert(_Alignof(typeof(*bt)) <= NLA_ALIGNTO);
>>
>> But I have to admit that you caught me off guard. I did not think of that. Maybe
>> I should add above static assertions to the code to document that what we are
>> doing is correct?
> 
> Yes, make it so!

I applied the changes locally.

Let me know when you are done with the review of the other patches. I will wait
for your other comments (if any) before sending v3.


Yours sincerely,
Vincent Mailhol