| Message ID | 20230420-bonding-be-vlan-proto-v2-1-9f594fabdbd9@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c1bc7d73c96425db12bb92fbf24c37fd1c9ac329 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] bonding: Always assign be16 value to vlan_proto | expand |
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. > >It also seems somewhat odd to store a VLAN ID value in a field that is >otherwise used to store Ether types. > >Address this issue by defining BOND_VLAN_PROTO_NONE, a big endian value. >0xffff was chosen somewhat arbitrarily. What is important is that it >doesn't overlap with any valid VLAN Ether types. As I think you mentioned, 0xffff is marked as a reserved ethertype. >I don't believe the problems described above are a bug because >VLAN_N_VID in both little-endian and big-endian byte order does not >conflict with any supported VLAN Ether types 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> Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com> -J >--- >Changes in v2: >- Decribe Ether Type aspect of problem in patch description >- Use an Ether Type rather than VID valie as sential >- Link to v1: https://lore.kernel.org/r/20230420-bonding-be-vlan-proto-v1-1-754399f51d01@kernel.org >--- > drivers/net/bonding/bond_main.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 3fed888629f7..ebf61c19dcef 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -2871,6 +2871,8 @@ static bool bond_has_this_ip(struct bonding *bond, __be32 ip) > return ret; > } > >+#define BOND_VLAN_PROTO_NONE cpu_to_be16(0xffff) >+ > static bool bond_handle_vlan(struct slave *slave, struct bond_vlan_tag *tags, > struct sk_buff *skb) > { >@@ -2878,13 +2880,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 == BOND_VLAN_PROTO_NONE) > 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 != BOND_VLAN_PROTO_NONE) { > if (!tags->vlan_id) { > tags++; > continue; >@@ -2960,7 +2962,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 = BOND_VLAN_PROTO_NONE; > return tags; > } > > > --- -Jay Vosburgh, jay.vosburgh@canonical.com
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Thu, 11 May 2023 17:07:12 +0200 you 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. > > It also seems somewhat odd to store a VLAN ID value in a field that is > otherwise used to store Ether types. > > [...] Here is the summary with links: - [net-next,v2] bonding: Always assign be16 value to vlan_proto https://git.kernel.org/netdev/net-next/c/c1bc7d73c964 You are awesome, thank you!
On Thu, May 11, 2023 at 04:43:57PM -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. > > > >It also seems somewhat odd to store a VLAN ID value in a field that is > >otherwise used to store Ether types. > > > >Address this issue by defining BOND_VLAN_PROTO_NONE, a big endian value. > >0xffff was chosen somewhat arbitrarily. What is important is that it > >doesn't overlap with any valid VLAN Ether types. > > As I think you mentioned, 0xffff is marked as a reserved ethertype. Yes, it seems that I did. It is reserved in RFC-1701. I can work it into the patch description if you like - there is no particular reason I didn't for v2. [1] https://lore.kernel.org/all/ZEI0zpDyJtfogO7s@kernel.org/ [2] https://www.rfc-editor.org/rfc/rfc1701.html [3] https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml > >I don't believe the problems described above are a bug because > >VLAN_N_VID in both little-endian and big-endian byte order does not > >conflict with any supported VLAN Ether types 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> > > Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com> > > -J > > >--- > >Changes in v2: > >- Decribe Ether Type aspect of problem in patch description > >- Use an Ether Type rather than VID valie as sential > >- Link to v1: https://lore.kernel.org/r/20230420-bonding-be-vlan-proto-v1-1-754399f51d01@kernel.org > >--- > > drivers/net/bonding/bond_main.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > >index 3fed888629f7..ebf61c19dcef 100644 > >--- a/drivers/net/bonding/bond_main.c > >+++ b/drivers/net/bonding/bond_main.c > >@@ -2871,6 +2871,8 @@ static bool bond_has_this_ip(struct bonding *bond, __be32 ip) > > return ret; > > } > > > >+#define BOND_VLAN_PROTO_NONE cpu_to_be16(0xffff) > >+ > > static bool bond_handle_vlan(struct slave *slave, struct bond_vlan_tag *tags, > > struct sk_buff *skb) > > { > >@@ -2878,13 +2880,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 == BOND_VLAN_PROTO_NONE) > > 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 != BOND_VLAN_PROTO_NONE) { > > if (!tags->vlan_id) { > > tags++; > > continue; > >@@ -2960,7 +2962,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 = BOND_VLAN_PROTO_NONE; > > return tags; > > } > > > > > > > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com >
On Fri, May 12, 2023 at 11:29:06AM +0200, Simon Horman wrote: > On Thu, May 11, 2023 at 04:43:57PM -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. > > > > > >It also seems somewhat odd to store a VLAN ID value in a field that is > > >otherwise used to store Ether types. > > > > > >Address this issue by defining BOND_VLAN_PROTO_NONE, a big endian value. > > >0xffff was chosen somewhat arbitrarily. What is important is that it > > >doesn't overlap with any valid VLAN Ether types. > > > > As I think you mentioned, 0xffff is marked as a reserved ethertype. > > Yes, it seems that I did. It is reserved in RFC-1701. > > I can work it into the patch description if you like - there is no > particular reason I didn't for v2. > > [1] https://lore.kernel.org/all/ZEI0zpDyJtfogO7s@kernel.org/ > [2] https://www.rfc-editor.org/rfc/rfc1701.html > [3] https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml I guess there will be no v2 as v2 was accepted :) - [net-next,v2] bonding: Always assign be16 value to vlan_proto https://git.kernel.org/netdev/net-next/c/c1bc7d73c964 In any case, this is now documented in the ML archives.
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 3fed888629f7..ebf61c19dcef 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2871,6 +2871,8 @@ static bool bond_has_this_ip(struct bonding *bond, __be32 ip) return ret; } +#define BOND_VLAN_PROTO_NONE cpu_to_be16(0xffff) + static bool bond_handle_vlan(struct slave *slave, struct bond_vlan_tag *tags, struct sk_buff *skb) { @@ -2878,13 +2880,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 == BOND_VLAN_PROTO_NONE) 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 != BOND_VLAN_PROTO_NONE) { if (!tags->vlan_id) { tags++; continue; @@ -2960,7 +2962,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 = BOND_VLAN_PROTO_NONE; return tags; }
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. It also seems somewhat odd to store a VLAN ID value in a field that is otherwise used to store Ether types. Address this issue by defining BOND_VLAN_PROTO_NONE, a big endian value. 0xffff was chosen somewhat arbitrarily. What is important is that it doesn't overlap with any valid VLAN Ether types. I don't believe the problems described above are a bug because VLAN_N_VID in both little-endian and big-endian byte order does not conflict with any supported VLAN Ether types 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> --- Changes in v2: - Decribe Ether Type aspect of problem in patch description - Use an Ether Type rather than VID valie as sential - Link to v1: https://lore.kernel.org/r/20230420-bonding-be-vlan-proto-v1-1-754399f51d01@kernel.org --- drivers/net/bonding/bond_main.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)