Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

["Szapar-Mudlaw, Martyna" <martyna.szapar-mudlaw@linux.intel.com>]

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 <jan.glaza@intel.com>
>>>>
>>>> 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 <jedrzej.jagielski@intel.com>
>>>> Reviewed-by: Simon Horman <horms@kernel.org>
>>>> Signed-off-by: Jan Glaza <jan.glaza@intel.com>
>>>> Signed-off-by: Martyna Szapar-Mudlaw <martyna.szapar- 
>>>> mudlaw@linux.intel.com>
>>>> ---
>>>>   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
>