[PATCH] bonding: Always assign be16 value to vlan_proto

[PATCH] bonding: Always assign be16 value to vlan_proto

[Simon Horman]

The type of the vlan_proto field is __be16.
And most users of the field use it as such.

In the case of setting or testing the field for the
special VLAN_N_VID value, host byte order is used.
Which seems incorrect.

Address this issue by converting VLAN_N_VID to __be16.

I don't believe this is a bug because VLAN_N_VID in
both little-endian (and big-endian) byte order does
not conflict with any valid values (0 through VLAN_N_VID - 1)
in big-endian byte order.

Reported by sparse as:

 .../bond_main.c:2857:26: warning: restricted __be16 degrades to integer
 .../bond_main.c:2863:20: warning: restricted __be16 degrades to integer
 .../bond_main.c:2939:40: warning: incorrect type in assignment (different base types)
 .../bond_main.c:2939:40:    expected restricted __be16 [usertype] vlan_proto
 .../bond_main.c:2939:40:    got int

No functional changes intended.
Compile tested only.

Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/bonding/bond_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index db7e650d9ebb..7f4c75fe58e1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2854,13 +2854,13 @@ static bool bond_handle_vlan(struct slave *slave, struct bond_vlan_tag *tags,
 	struct net_device *slave_dev = slave->dev;
 	struct bond_vlan_tag *outer_tag = tags;
 
-	if (!tags || tags->vlan_proto == VLAN_N_VID)
+	if (!tags || tags->vlan_proto == cpu_to_be16(VLAN_N_VID))
 		return true;
 
 	tags++;
 
 	/* Go through all the tags backwards and add them to the packet */
-	while (tags->vlan_proto != VLAN_N_VID) {
+	while (tags->vlan_proto != cpu_to_be16(VLAN_N_VID)) {
 		if (!tags->vlan_id) {
 			tags++;
 			continue;
@@ -2936,7 +2936,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
 		tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC);
 		if (!tags)
 			return ERR_PTR(-ENOMEM);
-		tags[level].vlan_proto = VLAN_N_VID;
+		tags[level].vlan_proto = cpu_to_be16(VLAN_N_VID);
 		return tags;
 	}
 

Re: [PATCH] bonding: Always assign be16 value to vlan_proto

[Jay Vosburgh]

Simon Horman <horms@kernel.org> wrote:

>The type of the vlan_proto field is __be16.
>And most users of the field use it as such.
>
>In the case of setting or testing the field for the
>special VLAN_N_VID value, host byte order is used.
>Which seems incorrect.
>
>Address this issue by converting VLAN_N_VID to __be16.
>
>I don't believe this is a bug because VLAN_N_VID in
>both little-endian (and big-endian) byte order does
>not conflict with any valid values (0 through VLAN_N_VID - 1)
>in big-endian byte order.

	Is that true for all cases, or am I just confused?  Doesn't VLAN
ID 16 match VLAN_N_VID (which is 4096) if byte swapped?

	I.e., on a little endian host, VLAN_N_VID is 0x1000 natively,
and network byte order (big endian) of VLAN ID 16 is also 0x1000.

	Either way, I think the change is fine; VLAN_N_VID is being used
as a sentinel value here, so the only real requirement is that it not
match an actual VLAN ID in network byte order.

	-J

>Reported by sparse as:
>
> .../bond_main.c:2857:26: warning: restricted __be16 degrades to integer
> .../bond_main.c:2863:20: warning: restricted __be16 degrades to integer
> .../bond_main.c:2939:40: warning: incorrect type in assignment (different base types)
> .../bond_main.c:2939:40:    expected restricted __be16 [usertype] vlan_proto
> .../bond_main.c:2939:40:    got int
>
>No functional changes intended.
>Compile tested only.
>
>Signed-off-by: Simon Horman <horms@kernel.org>
>---
> drivers/net/bonding/bond_main.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index db7e650d9ebb..7f4c75fe58e1 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2854,13 +2854,13 @@ static bool bond_handle_vlan(struct slave *slave, struct bond_vlan_tag *tags,
> 	struct net_device *slave_dev = slave->dev;
> 	struct bond_vlan_tag *outer_tag = tags;
> 
>-	if (!tags || tags->vlan_proto == VLAN_N_VID)
>+	if (!tags || tags->vlan_proto == cpu_to_be16(VLAN_N_VID))
> 		return true;
> 
> 	tags++;
> 
> 	/* Go through all the tags backwards and add them to the packet */
>-	while (tags->vlan_proto != VLAN_N_VID) {
>+	while (tags->vlan_proto != cpu_to_be16(VLAN_N_VID)) {
> 		if (!tags->vlan_id) {
> 			tags++;
> 			continue;
>@@ -2936,7 +2936,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
> 		tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC);
> 		if (!tags)
> 			return ERR_PTR(-ENOMEM);
>-		tags[level].vlan_proto = VLAN_N_VID;
>+		tags[level].vlan_proto = cpu_to_be16(VLAN_N_VID);
> 		return tags;
> 	}

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH] bonding: Always assign be16 value to vlan_proto

[Vladimir Oltean]

On Thu, Apr 20, 2023 at 12:47:33PM -0700, Jay Vosburgh wrote:
> Simon Horman <horms@kernel.org> wrote:
> 
> >The type of the vlan_proto field is __be16.
> >And most users of the field use it as such.
> >
> >In the case of setting or testing the field for the
> >special VLAN_N_VID value, host byte order is used.
> >Which seems incorrect.
> >
> >Address this issue by converting VLAN_N_VID to __be16.
> >
> >I don't believe this is a bug because VLAN_N_VID in
> >both little-endian (and big-endian) byte order does
> >not conflict with any valid values (0 through VLAN_N_VID - 1)
> >in big-endian byte order.
> 
> 	Is that true for all cases, or am I just confused?  Doesn't VLAN
> ID 16 match VLAN_N_VID (which is 4096) if byte swapped?
> 
> 	I.e., on a little endian host, VLAN_N_VID is 0x1000 natively,
> and network byte order (big endian) of VLAN ID 16 is also 0x1000.
> 
> 	Either way, I think the change is fine; VLAN_N_VID is being used
> as a sentinel value here, so the only real requirement is that it not
> match an actual VLAN ID in network byte order.
> 
> 	-J

In a strange twist of events, VLAN_N_VID is assigned as a sentinel value
to a variable which usually holds the output of vlan_dev_vlan_proto(),
or i.o.w. values like htons(ETH_P_8021Q), htons(ETH_P_8021AD). It is
certainly a confusion of types to assign VLAN_N_VID to it, but at least
it's not a valid VLAN protocol.

To answer your question, tags->vlan_proto is never compared against a
VLAN ID.

Re: [PATCH] bonding: Always assign be16 value to vlan_proto

[Jay Vosburgh]

Vladimir Oltean <olteanv@gmail.com> wrote:

>On Thu, Apr 20, 2023 at 12:47:33PM -0700, Jay Vosburgh wrote:
>> Simon Horman <horms@kernel.org> wrote:
>> 
>> >The type of the vlan_proto field is __be16.
>> >And most users of the field use it as such.
>> >
>> >In the case of setting or testing the field for the
>> >special VLAN_N_VID value, host byte order is used.
>> >Which seems incorrect.
>> >
>> >Address this issue by converting VLAN_N_VID to __be16.
>> >
>> >I don't believe this is a bug because VLAN_N_VID in
>> >both little-endian (and big-endian) byte order does
>> >not conflict with any valid values (0 through VLAN_N_VID - 1)
>> >in big-endian byte order.
>> 
>> 	Is that true for all cases, or am I just confused?  Doesn't VLAN
>> ID 16 match VLAN_N_VID (which is 4096) if byte swapped?
>> 
>> 	I.e., on a little endian host, VLAN_N_VID is 0x1000 natively,
>> and network byte order (big endian) of VLAN ID 16 is also 0x1000.
>> 
>> 	Either way, I think the change is fine; VLAN_N_VID is being used
>> as a sentinel value here, so the only real requirement is that it not
>> match an actual VLAN ID in network byte order.
>> 
>> 	-J
>
>In a strange twist of events, VLAN_N_VID is assigned as a sentinel value
>to a variable which usually holds the output of vlan_dev_vlan_proto(),
>or i.o.w. values like htons(ETH_P_8021Q), htons(ETH_P_8021AD). It is
>certainly a confusion of types to assign VLAN_N_VID to it, but at least
>it's not a valid VLAN protocol.
>
>To answer your question, tags->vlan_proto is never compared against a
>VLAN ID.

	Yah, looking again I see that now; I was checking the math on
Simon's statement about "0 through VLAN_N_VID - 1".

	So, I think the patch is correct, but the commit message should
really explain the reality.  And, perhaps we should use 0 or 0xffff for
the sentinel, since neither are valid Ethernet protocol IDs.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH] bonding: Always assign be16 value to vlan_proto

[Simon Horman]

On Thu, Apr 20, 2023 at 02:23:32PM -0700, Jay Vosburgh wrote:
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> >On Thu, Apr 20, 2023 at 12:47:33PM -0700, Jay Vosburgh wrote:
> >> Simon Horman <horms@kernel.org> wrote:
> >> 
> >> >The type of the vlan_proto field is __be16.
> >> >And most users of the field use it as such.
> >> >
> >> >In the case of setting or testing the field for the
> >> >special VLAN_N_VID value, host byte order is used.
> >> >Which seems incorrect.
> >> >
> >> >Address this issue by converting VLAN_N_VID to __be16.
> >> >
> >> >I don't believe this is a bug because VLAN_N_VID in
> >> >both little-endian (and big-endian) byte order does
> >> >not conflict with any valid values (0 through VLAN_N_VID - 1)
> >> >in big-endian byte order.
> >> 
> >> 	Is that true for all cases, or am I just confused?  Doesn't VLAN
> >> ID 16 match VLAN_N_VID (which is 4096) if byte swapped?
> >> 
> >> 	I.e., on a little endian host, VLAN_N_VID is 0x1000 natively,
> >> and network byte order (big endian) of VLAN ID 16 is also 0x1000.
> >> 
> >> 	Either way, I think the change is fine; VLAN_N_VID is being used
> >> as a sentinel value here, so the only real requirement is that it not
> >> match an actual VLAN ID in network byte order.
> >> 
> >> 	-J
> >
> >In a strange twist of events, VLAN_N_VID is assigned as a sentinel value
> >to a variable which usually holds the output of vlan_dev_vlan_proto(),
> >or i.o.w. values like htons(ETH_P_8021Q), htons(ETH_P_8021AD). It is
> >certainly a confusion of types to assign VLAN_N_VID to it, but at least
> >it's not a valid VLAN protocol.
> >
> >To answer your question, tags->vlan_proto is never compared against a
> >VLAN ID.
> 
> 	Yah, looking again I see that now; I was checking the math on
> Simon's statement about "0 through VLAN_N_VID - 1".
> 
> 	So, I think the patch is correct, but the commit message should
> really explain the reality.  And, perhaps we should use 0 or 0xffff for
> the sentinel, since neither are valid Ethernet protocol IDs.

Hi Jay and Vladimir,

Thanks for your review.

Firstly, sorry for the distraction about the VLAN_N_VID math.  I agree it
was incorrect. I had an out by one bug in my thought process which was
about 0x0fff instead of 0x1000.

Secondly, sorry for missing the central issue that it is a bit weird
to use a VID related value as a sentinel for a protocol field.
I agree it would be best to chose a different value.

In reference to the list of EtherTypes [1]. I think 0 might be ok,
but perhaps not ideal as technically it means a value of 0 for the
IEEE802.3 Length Field (although perhaps it can never mean that in this
context).

OTOH, 0xffff, is 'reserved' ([1] references RFC1701 [2]),
so perhaps it is a good choice.

In any case, I'm open to suggestions.
I'll probably hold off until the v6.5 cycle before reposting,
unless -rc8 appears next week. I'd rather not rush this one
given that I seem to have already got it wrong once.

[1] https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml#ieee-802-numbers-1
[2] https://www.rfc-editor.org/rfc/rfc1701.html

Re: [PATCH] bonding: Always assign be16 value to vlan_proto

[Vladimir Oltean]

On Fri, Apr 21, 2023 at 09:01:34AM +0200, Simon Horman wrote:
> Hi Jay and Vladimir,
> 
> Thanks for your review.
> 
> Firstly, sorry for the distraction about the VLAN_N_VID math.  I agree it
> was incorrect. I had an out by one bug in my thought process which was
> about 0x0fff instead of 0x1000.
> 
> Secondly, sorry for missing the central issue that it is a bit weird
> to use a VID related value as a sentinel for a protocol field.
> I agree it would be best to chose a different value.
> 
> In reference to the list of EtherTypes [1]. I think 0 might be ok,
> but perhaps not ideal as technically it means a value of 0 for the
> IEEE802.3 Length Field (although perhaps it can never mean that in this
> context).
> 
> OTOH, 0xffff, is 'reserved' ([1] references RFC1701 [2]),
> so perhaps it is a good choice.
> 
> In any case, I'm open to suggestions.
> I'll probably hold off until the v6.5 cycle before reposting,
> unless -rc8 appears next week. I'd rather not rush this one
> given that I seem to have already got it wrong once.
> 
> [1] https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml#ieee-802-numbers-1
> [2] https://www.rfc-editor.org/rfc/rfc1701.html

Any value would work as long as it's not a valid VLAN protocol.
I would #define BOND_VLAN_PROTO_NONE htons(0xffff) and use that.

Re: [PATCH] bonding: Always assign be16 value to vlan_proto

[Jay Vosburgh]

Vladimir Oltean <olteanv@gmail.com> wrote:

>On Fri, Apr 21, 2023 at 09:01:34AM +0200, Simon Horman wrote:
>> Hi Jay and Vladimir,
>> 
>> Thanks for your review.
>> 
>> Firstly, sorry for the distraction about the VLAN_N_VID math.  I agree it
>> was incorrect. I had an out by one bug in my thought process which was
>> about 0x0fff instead of 0x1000.
>> 
>> Secondly, sorry for missing the central issue that it is a bit weird
>> to use a VID related value as a sentinel for a protocol field.
>> I agree it would be best to chose a different value.
>> 
>> In reference to the list of EtherTypes [1]. I think 0 might be ok,
>> but perhaps not ideal as technically it means a value of 0 for the
>> IEEE802.3 Length Field (although perhaps it can never mean that in this
>> context).
>> 
>> OTOH, 0xffff, is 'reserved' ([1] references RFC1701 [2]),
>> so perhaps it is a good choice.
>> 
>> In any case, I'm open to suggestions.
>> I'll probably hold off until the v6.5 cycle before reposting,
>> unless -rc8 appears next week. I'd rather not rush this one
>> given that I seem to have already got it wrong once.
>> 
>> [1] https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml#ieee-802-numbers-1
>> [2] https://www.rfc-editor.org/rfc/rfc1701.html
>
>Any value would work as long as it's not a valid VLAN protocol.
>I would #define BOND_VLAN_PROTO_NONE htons(0xffff) and use that.

	All of the above is fine with me; this isn't an urgent change.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com