[net-next,1/3] virtchnl: fix fake 1-elem arrays in structs allocated as `nents + 1` - 1

Commit Message

Alexander Lobakin July 28, 2023, 3:52 p.m. UTC

The two most problematic virtchnl structures are virtchnl_rss_key and
virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
allocating / checking, the actual size is calculated as `sizeof +
nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
of such structure with proper flex array, it's two bytes larger due to
the padding. That said, their size is always 1 byte larger unless
there are no tail elements -- then it's +2 bytes.
Add virtchnl_struct_size() macro which will handle this case (and later
other cases as well). Make its calling conv the same as we call
struct_size() to allow it to be drop-in, even though it's unlikely to
become possible to switch to generic API. The macro will calculate a
proper size of a structure with a flex array at the end, so that it
becomes transparent for the compilers, but add the difference from the
old values, so that the real size of sorta-ABI-messages doesn't change.
Use it on the allocation side in IAVF and the receiving side (defined
as static inline in virtchnl.h) for the mentioned two structures.

Signed-off-by: Alexander Lobakin <[e-mail]>
---
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  6 ++--
 include/linux/avf/virtchnl.h                  | 31 ++++++++++++++-----
 2 files changed, 25 insertions(+), 12 deletions(-)

Comments

Kees Cook July 28, 2023, 10:43 p.m. UTC | #1

On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> The two most problematic virtchnl structures are virtchnl_rss_key and
> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
> allocating / checking, the actual size is calculated as `sizeof +
> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
> of such structure with proper flex array, it's two bytes larger due to
> the padding. That said, their size is always 1 byte larger unless
> there are no tail elements -- then it's +2 bytes.
> Add virtchnl_struct_size() macro which will handle this case (and later
> other cases as well). Make its calling conv the same as we call
> struct_size() to allow it to be drop-in, even though it's unlikely to
> become possible to switch to generic API. The macro will calculate a
> proper size of a structure with a flex array at the end, so that it
> becomes transparent for the compilers, but add the difference from the
> old values, so that the real size of sorta-ABI-messages doesn't change.
> Use it on the allocation side in IAVF and the receiving side (defined
> as static inline in virtchnl.h) for the mentioned two structures.

This all looks workable, but it's a unique solution in the kernel. That
is fine, of course, but would it be easier to maintain/test if it went
with the union style solutions?

struct foo {
	...
	union {
		type legacy_padding;
		DECLARE_FLEX_ARRAY(type, member);
	};
};

Then the size doesn't change and "member" can still be used. (i.e. no
collateral changes needed.)

-Kees

Alexander Lobakin Aug. 1, 2023, 1:08 p.m. UTC | #2

From: Kees Cook <[e-mail]>
Date: Fri, 28 Jul 2023 15:43:26 -0700

> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
>> The two most problematic virtchnl structures are virtchnl_rss_key and
>> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
>> allocating / checking, the actual size is calculated as `sizeof +
>> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
>> of such structure with proper flex array, it's two bytes larger due to
>> the padding. That said, their size is always 1 byte larger unless
>> there are no tail elements -- then it's +2 bytes.
>> Add virtchnl_struct_size() macro which will handle this case (and later
>> other cases as well). Make its calling conv the same as we call
>> struct_size() to allow it to be drop-in, even though it's unlikely to
>> become possible to switch to generic API. The macro will calculate a
>> proper size of a structure with a flex array at the end, so that it
>> becomes transparent for the compilers, but add the difference from the
>> old values, so that the real size of sorta-ABI-messages doesn't change.
>> Use it on the allocation side in IAVF and the receiving side (defined
>> as static inline in virtchnl.h) for the mentioned two structures.
> 
> This all looks workable, but it's a unique solution in the kernel. That
> is fine, of course, but would it be easier to maintain/test if it went
> with the union style solutions?
> 
> struct foo {
> 	...
> 	union {
> 		type legacy_padding;
> 		DECLARE_FLEX_ARRAY(type, member);
> 	};
> };
> 
> Then the size doesn't change and "member" can still be used. (i.e. no
> collateral changes needed.)

This wouldn't work unfortunately. I mean, you'd still need weird
calculations.
Speaking of e.g. virtchnl_rss_lut, it's always
`struct_size(nents + 1) - 1`, you can't just use the pattern above and
then struct_size(). Not only legacy padding is needed, but also
calculation adjustments.
Or did you mean define the structures as above and leave the
calculations as they are? It makes me feel we can miss something that
way. With the series, all the broken structures are processed in one
place and I thought _this_ would be actually easier to maintain...

> 
> -Kees
> 

Thanks,
Olek

Kees Cook Aug. 4, 2023, 8:27 a.m. UTC | #3

On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> The two most problematic virtchnl structures are virtchnl_rss_key and
> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
> allocating / checking, the actual size is calculated as `sizeof +
> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
> of such structure with proper flex array, it's two bytes larger due to
> the padding. That said, their size is always 1 byte larger unless
> there are no tail elements -- then it's +2 bytes.
> Add virtchnl_struct_size() macro which will handle this case (and later
> other cases as well). Make its calling conv the same as we call
> struct_size() to allow it to be drop-in, even though it's unlikely to
> become possible to switch to generic API. The macro will calculate a
> proper size of a structure with a flex array at the end, so that it
> becomes transparent for the compilers, but add the difference from the
> old values, so that the real size of sorta-ABI-messages doesn't change.
> Use it on the allocation side in IAVF and the receiving side (defined
> as static inline in virtchnl.h) for the mentioned two structures.
> 
> Signed-off-by: Alexander Lobakin <[e-mail]>

This is a novel approach to solving the ABI issues for a 1-elem
conversion, but I have been convinced it's a workable approach here. :)
Thanks for doing this conversion!

Reviewed-by: Kees Cook <[e-mail]>

Alexander Lobakin Aug. 4, 2023, 3:42 p.m. UTC | #4

From: Kees Cook <[e-mail]>
Date: Fri, 4 Aug 2023 01:27:02 -0700

> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
>> The two most problematic virtchnl structures are virtchnl_rss_key and
>> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
>> allocating / checking, the actual size is calculated as `sizeof +
>> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
>> of such structure with proper flex array, it's two bytes larger due to
>> the padding. That said, their size is always 1 byte larger unless
>> there are no tail elements -- then it's +2 bytes.
>> Add virtchnl_struct_size() macro which will handle this case (and later
>> other cases as well). Make its calling conv the same as we call
>> struct_size() to allow it to be drop-in, even though it's unlikely to
>> become possible to switch to generic API. The macro will calculate a
>> proper size of a structure with a flex array at the end, so that it
>> becomes transparent for the compilers, but add the difference from the
>> old values, so that the real size of sorta-ABI-messages doesn't change.
>> Use it on the allocation side in IAVF and the receiving side (defined
>> as static inline in virtchnl.h) for the mentioned two structures.
>>
>> Signed-off-by: Alexander Lobakin <[e-mail]>
> 
> This is a novel approach to solving the ABI issues for a 1-elem
> conversion, but I have been convinced it's a workable approach here. :)
> Thanks for doing this conversion!
> 
> Reviewed-by: Kees Cook <[e-mail]>
> 

Thanks a lot!
You gave Reviewed-by for patches #1 and #3, does it mean the whole
series or something is wrong with the patch #2? :D

Thanks,
Olek

Kees Cook Aug. 4, 2023, 5:29 p.m. UTC | #5

On Fri, Aug 04, 2023 at 05:42:19PM +0200, Alexander Lobakin wrote:
> From: Kees Cook <[e-mail]>
> Date: Fri, 4 Aug 2023 01:27:02 -0700
> 
> > On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> >> The two most problematic virtchnl structures are virtchnl_rss_key and
> >> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
> >> allocating / checking, the actual size is calculated as `sizeof +
> >> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
> >> of such structure with proper flex array, it's two bytes larger due to
> >> the padding. That said, their size is always 1 byte larger unless
> >> there are no tail elements -- then it's +2 bytes.
> >> Add virtchnl_struct_size() macro which will handle this case (and later
> >> other cases as well). Make its calling conv the same as we call
> >> struct_size() to allow it to be drop-in, even though it's unlikely to
> >> become possible to switch to generic API. The macro will calculate a
> >> proper size of a structure with a flex array at the end, so that it
> >> becomes transparent for the compilers, but add the difference from the
> >> old values, so that the real size of sorta-ABI-messages doesn't change.
> >> Use it on the allocation side in IAVF and the receiving side (defined
> >> as static inline in virtchnl.h) for the mentioned two structures.
> >>
> >> Signed-off-by: Alexander Lobakin <[e-mail]>
> > 
> > This is a novel approach to solving the ABI issues for a 1-elem
> > conversion, but I have been convinced it's a workable approach here. :)
> > Thanks for doing this conversion!
> > 
> > Reviewed-by: Kees Cook <[e-mail]>
> > 
> 
> Thanks a lot!
> You gave Reviewed-by for patches #1 and #3, does it mean the whole
> series or something is wrong with the patch #2? :D

Hm, maybe delivery was delayed? I see it on lore:
https://lore.kernel.org/all/202308040128.667940394B@keescook/

Alexander Lobakin Aug. 4, 2023, 5:33 p.m. UTC | #6

From: Kees Cook <[e-mail]>
Date: Fri, 4 Aug 2023 10:29:48 -0700

> On Fri, Aug 04, 2023 at 05:42:19PM +0200, Alexander Lobakin wrote:
>> From: Kees Cook <[e-mail]>
>> Date: Fri, 4 Aug 2023 01:27:02 -0700
>>
>>> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
>>>> The two most problematic virtchnl structures are virtchnl_rss_key and
>>>> virtchnl_rss_lut. Their "flex" arrays have the type of u8, thus, when
>>>> allocating / checking, the actual size is calculated as `sizeof +
>>>> nents - 1 byte`. But their sizeof() is not 1 byte larger than the size
>>>> of such structure with proper flex array, it's two bytes larger due to
>>>> the padding. That said, their size is always 1 byte larger unless
>>>> there are no tail elements -- then it's +2 bytes.
>>>> Add virtchnl_struct_size() macro which will handle this case (and later
>>>> other cases as well). Make its calling conv the same as we call
>>>> struct_size() to allow it to be drop-in, even though it's unlikely to
>>>> become possible to switch to generic API. The macro will calculate a
>>>> proper size of a structure with a flex array at the end, so that it
>>>> becomes transparent for the compilers, but add the difference from the
>>>> old values, so that the real size of sorta-ABI-messages doesn't change.
>>>> Use it on the allocation side in IAVF and the receiving side (defined
>>>> as static inline in virtchnl.h) for the mentioned two structures.
>>>>
>>>> Signed-off-by: Alexander Lobakin <[e-mail]>
>>>
>>> This is a novel approach to solving the ABI issues for a 1-elem
>>> conversion, but I have been convinced it's a workable approach here. :)
>>> Thanks for doing this conversion!
>>>
>>> Reviewed-by: Kees Cook <[e-mail]>
>>>
>>
>> Thanks a lot!
>> You gave Reviewed-by for patches #1 and #3, does it mean the whole
>> series or something is wrong with the patch #2? :D
> 
> Hm, maybe delivery was delayed? I see it on lore:
> https://lore.kernel.org/all/202308040128.667940394B@keescook/
> 

Nevermind, my mail rules did put it in the folder other than the one
where the main thread was, sorry :s
Much thanks, I'm now a fan of _Generic() too :D

Olek

Romanowski, Rafal Aug. 16, 2023, 12:48 p.m. UTC | #7

> -----Original Message-----
> From: Intel-wired-lan <[e-mail]> On Behalf Of
> Alexander Lobakin
> Sent: piątek, 4 sierpnia 2023 19:34
> To: Kees Cook <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>; Zaremba,
> Larysa <[e-mail]>; [e-mail]; Gustavo A. R.
> Silva <[e-mail]>; [e-mail]; Eric Dumazet
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; Jakub Kicinski <[e-mail]>; Paolo Abeni
> <[e-mail]>; David S. Miller <[e-mail]>
> Subject: Re: [Intel-wired-lan] [PATCH net-next 1/3] virtchnl: fix fake 1-elem
> arrays in structs allocated as `nents + 1` - 1
> 
> From: Kees Cook <[e-mail]>
> Date: Fri, 4 Aug 2023 10:29:48 -0700
> 
> > On Fri, Aug 04, 2023 at 05:42:19PM +0200, Alexander Lobakin wrote:
> >> From: Kees Cook <[e-mail]>
> >> Date: Fri, 4 Aug 2023 01:27:02 -0700
> >>
> >>> On Fri, Jul 28, 2023 at 05:52:05PM +0200, Alexander Lobakin wrote:
> >>>> The two most problematic virtchnl structures are virtchnl_rss_key
> >>>> and virtchnl_rss_lut. Their "flex" arrays have the type of u8,
> >>>> thus, when allocating / checking, the actual size is calculated as
> >>>> `sizeof + nents - 1 byte`. But their sizeof() is not 1 byte larger
> >>>> than the size of such structure with proper flex array, it's two
> >>>> bytes larger due to the padding. That said, their size is always 1
> >>>> byte larger unless there are no tail elements -- then it's +2 bytes.
> >>>> Add virtchnl_struct_size() macro which will handle this case (and
> >>>> later other cases as well). Make its calling conv the same as we
> >>>> call
> >>>> struct_size() to allow it to be drop-in, even though it's unlikely
> >>>> to become possible to switch to generic API. The macro will
> >>>> calculate a proper size of a structure with a flex array at the
> >>>> end, so that it becomes transparent for the compilers, but add the
> >>>> difference from the old values, so that the real size of sorta-ABI-
> messages doesn't change.
> >>>> Use it on the allocation side in IAVF and the receiving side
> >>>> (defined as static inline in virtchnl.h) for the mentioned two structures.
> >>>>
> >>>> Signed-off-by: Alexander Lobakin <[e-mail]>
> >>>
> >>> This is a novel approach to solving the ABI issues for a 1-elem
> >>> conversion, but I have been convinced it's a workable approach here.
> >>> :) Thanks for doing this conversion!
> >>>
> >>> Reviewed-by: Kees Cook <[e-mail]>
> >>>
> >>
> >> Thanks a lot!
> >> You gave Reviewed-by for patches #1 and #3, does it mean the whole
> >> series or something is wrong with the patch #2? :D
> >
> > Hm, maybe delivery was delayed? I see it on lore:
> > https://lore.kernel.org/all/202308040128.667940394B@keescook/
> >
> 
> Nevermind, my mail rules did put it in the folder other than the one where
> the main thread was, sorry :s Much thanks, I'm now a fan of _Generic() too
> :D
> 
> Olek
> _______________________________________________
> Intel-wired-lan mailing list
> [e-mail]
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Tested-by: Rafal Romanowski <[e-mail]>

Patch

diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index be3c007ce90a..10f03054a603 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1085,8 +1085,7 @@  void iavf_set_rss_key(struct iavf_adapter *adapter)
 			adapter->current_op);
 		return;
 	}
-	len = sizeof(struct virtchnl_rss_key) +
-	      (adapter->rss_key_size * sizeof(u8)) - 1;
+	len = virtchnl_struct_size(vrk, key, adapter->rss_key_size);
 	vrk = kzalloc(len, GFP_KERNEL);
 	if (!vrk)
 		return;
@@ -1117,8 +1116,7 @@  void iavf_set_rss_lut(struct iavf_adapter *adapter)
 			adapter->current_op);
 		return;
 	}
-	len = sizeof(struct virtchnl_rss_lut) +
-	      (adapter->rss_lut_size * sizeof(u8)) - 1;
+	len = virtchnl_struct_size(vrl, lut, adapter->rss_lut_size);
 	vrl = kzalloc(len, GFP_KERNEL);
 	if (!vrl)
 		return;
diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index c15221dcb75e..3ab207c14809 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -866,18 +866,20 @@  VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_promisc_info);
 struct virtchnl_rss_key {
 	u16 vsi_id;
 	u16 key_len;
-	u8 key[1];         /* RSS hash key, packed bytes */
+	u8 key[];          /* RSS hash key, packed bytes */
 };
 
-VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_rss_key);
+#define virtchnl_rss_key_LEGACY_SIZEOF	6
 
 struct virtchnl_rss_lut {
 	u16 vsi_id;
 	u16 lut_entries;
-	u8 lut[1];        /* RSS lookup table */
+	u8 lut[];         /* RSS lookup table */
 };
 
-VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_lut);
+VIRTCHNL_CHECK_STRUCT_LEN(4, virtchnl_rss_lut);
+#define virtchnl_rss_lut_LEGACY_SIZEOF	6
 
 /* VIRTCHNL_OP_GET_RSS_HENA_CAPS
  * VIRTCHNL_OP_SET_RSS_HENA
@@ -1367,6 +1369,17 @@  struct virtchnl_fdir_del {
 
 VIRTCHNL_CHECK_STRUCT_LEN(12, virtchnl_fdir_del);
 
+#define __vss_byone(p, member, count, old)				      \
+	(struct_size(p, member, count) + (old - 1 - struct_size(p, member, 0)))
+
+#define __vss(type, func, p, member, count)		\
+	struct type: func(p, member, count, type##_LEGACY_SIZEOF)
+
+#define virtchnl_struct_size(p, m, c)					      \
+	_Generic(*p,							      \
+		 __vss(virtchnl_rss_key, __vss_byone, p, m, c),		      \
+		 __vss(virtchnl_rss_lut, __vss_byone, p, m, c))
+
 /**
  * virtchnl_vc_validate_vf_msg
  * @ver: Virtchnl version info
@@ -1479,19 +1492,21 @@  virtchnl_vc_validate_vf_msg(struct virtchnl_version_info *ver, u32 v_opcode,
 		}
 		break;
 	case VIRTCHNL_OP_CONFIG_RSS_KEY:
-		valid_len = sizeof(struct virtchnl_rss_key);
+		valid_len = virtchnl_rss_key_LEGACY_SIZEOF;
 		if (msglen >= valid_len) {
 			struct virtchnl_rss_key *vrk =
 				(struct virtchnl_rss_key *)msg;
-			valid_len += vrk->key_len - 1;
+			valid_len = virtchnl_struct_size(vrk, key,
+							 vrk->key_len);
 		}
 		break;
 	case VIRTCHNL_OP_CONFIG_RSS_LUT:
-		valid_len = sizeof(struct virtchnl_rss_lut);
+		valid_len = virtchnl_rss_lut_LEGACY_SIZEOF;
 		if (msglen >= valid_len) {
 			struct virtchnl_rss_lut *vrl =
 				(struct virtchnl_rss_lut *)msg;
-			valid_len += vrl->lut_entries - 1;
+			valid_len = virtchnl_struct_size(vrl, lut,
+							 vrl->lut_entries);
 		}
 		break;
 	case VIRTCHNL_OP_GET_RSS_HENA_CAPS: