| Message ID | 20250304110833.95997-4-martyna.szapar-mudlaw@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: fix validation issues in virtchnl parameters | expand |
Dear Jan, dear Martina, Thank you for the patch. 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? > 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
On 3/4/2025 12:15 PM, Paul Menzel wrote: > Dear Jan, dear Martina, > > > Thank you for the patch. > > 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 >> 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 >
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]. >>> 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
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 >
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 */ 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]; };