Bo YU (2):
net: bonding: fix restricted __be16 degrades to integer
net: bonding: fix incorrect type in assignment
drivers/net/bonding/bond_main.c | 8 ++++----
drivers/net/bonding/bond_options.c | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
--
2.11.0
There are some warning when:
sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/
drivers/net/bonding/bond_main.c:2385:26: warning: restricted __be16 degrades to integer
drivers/net/bonding/bond_main.c:2391:20: warning: restricted __be16 degrades to integer
...
drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer
drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer
So fix it.
Signed-off-by: Bo YU <[email protected]>
---
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 b59708c35faf..135fec28daa9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2382,13 +2382,13 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
return;
}
- if (!tags || tags->vlan_proto == VLAN_N_VID)
+ if (!tags || be16_to_cpu(tags->vlan_proto) == VLAN_N_VID)
goto xmit;
tags++;
/* Go through all the tags backwards and add them to the packet */
- while (tags->vlan_proto != VLAN_N_VID) {
+ while (be16_to_cpu(tags->vlan_proto) != VLAN_N_VID) {
if (!tags->vlan_id) {
tags++;
continue;
@@ -3238,7 +3238,7 @@ static inline u32 bond_eth_hash(struct sk_buff *skb)
ep = skb_header_pointer(skb, 0, sizeof(hdr_tmp), &hdr_tmp);
if (ep)
- return ep->h_dest[5] ^ ep->h_source[5] ^ ep->h_proto;
+ return ep->h_dest[5] ^ ep->h_source[5] ^ be16_to_cpu(ep->h_proto);
return 0;
}
--
2.11.0
There are some warning when:
sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/
drivers/net/bonding/bond_main.c:2438:40: warning: incorrect type in assignment (different base types)
drivers/net/bonding/bond_main.c:2438:40: expected restricted __be16 [usertype] vlan_proto
drivers/net/bonding/bond_main.c:2438:40:
...
rivers/net/bonding/bond_options.c:1089:24: warning: incorrect type in assignment (different base types)
drivers/net/bonding/bond_options.c:1089:24: expected restricted __be32 [addressable] [usertype] target
drivers/net/bonding/bond_options.c:1089:24: got unsigned long long const [usertype] value
So fix it
Signed-off-by: Bo YU <[email protected]>
---
drivers/net/bonding/bond_main.c | 2 +-
drivers/net/bonding/bond_options.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 135fec28daa9..07e52d863e91 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2435,7 +2435,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;
}
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index da1fc17295d9..3a196999bd1b 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1086,7 +1086,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
else
netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n");
} else {
- target = newval->value;
+ target = cpu_to_be32(newval->value);
ret = bond_option_arp_ip_target_add(bond, target);
}
--
2.11.0
Bo YU <[email protected]> wrote:
>There are some warning when:
>
>sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/
>
>drivers/net/bonding/bond_main.c:2438:40: warning: incorrect type in assignment (different base types)
>drivers/net/bonding/bond_main.c:2438:40: expected restricted __be16 [usertype] vlan_proto
>drivers/net/bonding/bond_main.c:2438:40:
>...
>rivers/net/bonding/bond_options.c:1089:24: warning: incorrect type in assignment (different base types)
>drivers/net/bonding/bond_options.c:1089:24: expected restricted __be32 [addressable] [usertype] target
>drivers/net/bonding/bond_options.c:1089:24: got unsigned long long const [usertype] value
>
>So fix it
>
>Signed-off-by: Bo YU <[email protected]>
>---
> drivers/net/bonding/bond_main.c | 2 +-
> drivers/net/bonding/bond_options.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 135fec28daa9..07e52d863e91 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2435,7 +2435,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;
> }
>
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index da1fc17295d9..3a196999bd1b 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1086,7 +1086,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
> else
> netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n");
> } else {
>- target = newval->value;
>+ target = cpu_to_be32(newval->value);
> ret = bond_option_arp_ip_target_add(bond, target);
I'm not sure this is correct; if I'm reading the call path
correctly, bond_changelink will
if (data[IFLA_BOND_ARP_IP_TARGET]) {
[...]
__be32 target;
[...]
target = nla_get_be32(attr);
bond_opt_initval(&newval, (__force u64)target);
err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS,
&newval);
thus, newval.value is initially be32, but stored in a u64.
__bond_opt_set will call bond_opt_parse, which in turn will call
bond_option_arp_ip_targets_set (via .set), and the change above would
swap the newval.value back to host order (on little endian architectures
for which cpu_to_be32 is not a no-op).
Am I misunderstanding? Did you test this change on an x86 or
other little endian system?
-J
---
-Jay Vosburgh, [email protected]
On Fri, Mar 8, 2019 at 2:51 PM Jay Vosburgh <[email protected]> wrote:
>
> Bo YU <[email protected]> wrote:
>
> >There are some warning when:
> >
> >sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/
> >
> >drivers/net/bonding/bond_main.c:2438:40: warning: incorrect type in assignment (different base types)
> >drivers/net/bonding/bond_main.c:2438:40: expected restricted __be16 [usertype] vlan_proto
> >drivers/net/bonding/bond_main.c:2438:40:
> >...
> >rivers/net/bonding/bond_options.c:1089:24: warning: incorrect type in assignment (different base types)
> >drivers/net/bonding/bond_options.c:1089:24: expected restricted __be32 [addressable] [usertype] target
> >drivers/net/bonding/bond_options.c:1089:24: got unsigned long long const [usertype] value
> >
> >So fix it
> >
> >Signed-off-by: Bo YU <[email protected]>
> >---
> > drivers/net/bonding/bond_main.c | 2 +-
> > drivers/net/bonding/bond_options.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 135fec28daa9..07e52d863e91 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -2435,7 +2435,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;
> > }
> >
> >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> >index da1fc17295d9..3a196999bd1b 100644
> >--- a/drivers/net/bonding/bond_options.c
> >+++ b/drivers/net/bonding/bond_options.c
> >@@ -1086,7 +1086,7 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
> > else
> > netdev_err(bond->dev, "no command found in arp_ip_targets file - use +<addr> or -<addr>\n");
> > } else {
> >- target = newval->value;
> >+ target = cpu_to_be32(newval->value);
> > ret = bond_option_arp_ip_target_add(bond, target);
>
> I'm not sure this is correct; if I'm reading the call path
> correctly, bond_changelink will
>
> if (data[IFLA_BOND_ARP_IP_TARGET]) {
> [...]
> __be32 target;
> [...]
> target = nla_get_be32(attr);
>
> bond_opt_initval(&newval, (__force u64)target);
> err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS,
> &newval);
>
> thus, newval.value is initially be32, but stored in a u64.
> __bond_opt_set will call bond_opt_parse, which in turn will call
> bond_option_arp_ip_targets_set (via .set), and the change above would
> swap the newval.value back to host order (on little endian architectures
> for which cpu_to_be32 is not a no-op).
>
> Am I misunderstanding? Did you test this change on an x86 or
> other little endian system?
Hi,
After above patch:
I have enable BONDING config in .config and dmesg:
[ 0.247417] Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)
I don't have the HW, so just add printfk in bond_option_arp_ip_targets_set
However, i don't get what i want to print.
I do the test in qemu-system-x86_64.
I do not know how to hit the driver maybe.
So, not confirmed :(
>
> -J
>
> ---
> -Jay Vosburgh, [email protected]
Bo YU <[email protected]> wrote:
>There are some warning when:
>
>sudo make C=1 CF=-D__CHECK_ENDIAN__ drivers/net/bonding/
>
>drivers/net/bonding/bond_main.c:2385:26: warning: restricted __be16 degrades to integer
>drivers/net/bonding/bond_main.c:2391:20: warning: restricted __be16 degrades to integer
>...
>drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer
>drivers/net/bonding/bond_main.c:3241:60: warning: restricted __be16 degrades to integer
>
>So fix it.
>
>Signed-off-by: Bo YU <[email protected]>
>---
> 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 b59708c35faf..135fec28daa9 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2382,13 +2382,13 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
> return;
> }
>
>- if (!tags || tags->vlan_proto == VLAN_N_VID)
>+ if (!tags || be16_to_cpu(tags->vlan_proto) == VLAN_N_VID)
> goto xmit;
>
> tags++;
>
> /* Go through all the tags backwards and add them to the packet */
>- while (tags->vlan_proto != VLAN_N_VID) {
>+ while (be16_to_cpu(tags->vlan_proto) != VLAN_N_VID) {
I believe both of the above are incorrect, as vlan_proto is set
explicitly to VLAN_N_VID (in host byte order) by bond_verify_device_path
as a sentinel value. Byte swapping the tags->vlan_proto value would
cause the test or loop to miss the sentinel.
> if (!tags->vlan_id) {
> tags++;
> continue;
>@@ -3238,7 +3238,7 @@ static inline u32 bond_eth_hash(struct sk_buff *skb)
>
> ep = skb_header_pointer(skb, 0, sizeof(hdr_tmp), &hdr_tmp);
> if (ep)
>- return ep->h_dest[5] ^ ep->h_source[5] ^ ep->h_proto;
>+ return ep->h_dest[5] ^ ep->h_source[5] ^ be16_to_cpu(ep->h_proto);
This is probably harmless, other than adding work to the
transmit path.
-J
---
-Jay Vosburgh, [email protected]