On 3/4/2025 12:51 PM, Paul Menzel wrote:
Dear Martyna,
Thank you for your quick reply.
Am 04.03.25 um 12:45 schrieb Szapar-Mudlaw, Martyna:
On 3/4/2025 12:15 PM, Paul Menzel wrote:
Am 04.03.25 um 12:08 schrieb Martyna Szapar-Mudlaw:
From: Jan Glaza <[email protected]>
The count field in virtchnl_proto_hdrs and virtchnl_filter_action_set
should never be negative while still being valid. Changing it from
int to u32 ensures proper handling of values in virtchnl messages in
driverrs and prevents unintended behavior.
In its current signed form, a negative count does not trigger
an error in ice driver but instead results in it being treated as 0.
This can lead to unexpected outcomes when processing messages.
By using u32, any invalid values will correctly trigger -EINVAL,
making error detection more robust.
Fixes: 1f7ea1cd6a374 ("ice: Enable FDIR Configure for AVF")
Reviewed-by: Jedrzej Jagielski <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Signed-off-by: Jan Glaza <[email protected]>
Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar-
[email protected]>
---
include/linux/avf/virtchnl.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/
virtchnl.h
index 4811b9a14604..cf0afa60e4a7 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -1343,7 +1343,7 @@ struct virtchnl_proto_hdrs {
* 2 - from the second inner layer
* ....
**/
- int count; /* the proto layers must <
VIRTCHNL_MAX_NUM_PROTO_HDRS */
+ u32 count; /* the proto layers must <
VIRTCHNL_MAX_NUM_PROTO_HDRS */
Why limit the length, and not use unsigned int?
u32 range is completely sufficient for number of proto hdrs (as said:
"the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS") and I believe
it is recommended to use fixed sized variables where possible
Do you have a pointer to the recommendation? I heard the opposite, that
fixed length is only useful for register writes. Otherwise, you should
use the “generic” types [1].
Thanks for sharing the source and your perspective, you are right, as a
general rule, using generic types is preferred - I actually learned
something new from this.
That said, I still believe there are exceptions, and in this case, using
u32 is the right choice. When dealing with protocols or data formats
using a fixed-width type makes sense.
Additionally, throughout this file, we consistently use u32/u16 for
similar cases, so also here we're keeping it aligned with the existing
codebase.
Thank you for your review and appreciate the discussion on best practices.
Regards,
Martyna
union {
struct virtchnl_proto_hdr
proto_hdr[VIRTCHNL_MAX_NUM_PROTO_HDRS];
@@ -1395,7 +1395,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(36,
virtchnl_filter_action);
struct virtchnl_filter_action_set {
/* action number must be less then VIRTCHNL_MAX_NUM_ACTIONS */
- int count;
+ u32 count;
struct virtchnl_filter_action actions[VIRTCHNL_MAX_NUM_ACTIONS];
};
Kind regards,
Paul
[1]: https://notabs.org/coding/smallIntsBigPenalty.htm
