All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch net 0/3] net: fix a stack out-of-bound access
@ 2017-04-26  5:03 Cong Wang
  2017-04-26  5:03 ` [Patch net 1/3] net: check mac address length for dev_set_mac_address() Cong Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Cong Wang @ 2017-04-26  5:03 UTC (permalink / raw)
  To: netdev; +Cc: andreyknvl, Cong Wang

This patchset fixes the following kernel bug reported by Andrey:

 BUG: KASAN: stack-out-of-bounds in bond_enslave+0xe0a/0x4ef0 at addr
 ffff8800666b7792
 Write of size 16 by task a.out/3894
 page:ffffea000199adc0 count:0 mapcount:0 mapping:          (null) index:0x0
 flags: 0x100000000000000()
 raw: 0100000000000000 0000000000000000 0000000000000000 00000000ffffffff
 raw: 0000000000000000 ffffea000199ade0 0000000000000000 0000000000000000
 page dumped because: kasan: bad access detected
 CPU: 1 PID: 3894 Comm: a.out Not tainted 4.11.0-rc7+ #251
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 Call Trace:
  __dump_stack lib/dump_stack.c:16
  dump_stack+0x292/0x398 lib/dump_stack.c:52
  kasan_report_error mm/kasan/report.c:212
  kasan_report+0x4d8/0x510 mm/kasan/report.c:347
  check_memory_region_inline mm/kasan/kasan.c:326
  check_memory_region+0x139/0x190 mm/kasan/kasan.c:333
  memcpy+0x37/0x50 mm/kasan/kasan.c:369
  bond_enslave+0xe0a/0x4ef0 drivers/net/bonding/bond_main.c:1491
  bond_do_ioctl+0xb5d/0xec0 drivers/net/bonding/bond_main.c:3449
  dev_ifsioc+0x53f/0x9f0 net/core/dev_ioctl.c:338
  dev_ioctl+0x249/0x1160 net/core/dev_ioctl.c:532
  sock_do_ioctl+0x94/0xb0 net/socket.c:913
  sock_ioctl+0x28f/0x440 net/socket.c:1004
  vfs_ioctl fs/ioctl.c:45
  do_vfs_ioctl+0x1bf/0x1780 fs/ioctl.c:685
  SYSC_ioctl fs/ioctl.c:700
  SyS_ioctl+0x8f/0xc0 fs/ioctl.c:691
  entry_SYSCALL_64_fastpath+0x1f/0xc2 arch/x86/entry/entry_64.S:204
 RIP: 0033:0x7fc764b31b79
 RSP: 002b:00007ffc1d73aa58 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
 RAX: ffffffffffffffda RBX: 00007ffc1d73abb0 RCX: 00007fc764b31b79
 RDX: 00000000209eafd8 RSI: 0000008000008990 RDI: 0000000000000003
 RBP: 00000000004004e0 R08: 0000000000000000 R09: 0000000000000000
 R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000000
 R13: 00007ffc1d73abb0 R14: 0000000000000000 R15: 0000000000000000

Please see each patch for details.

Cong Wang (3):
  net: check mac address length for dev_set_mac_address()
  bonding: use a larger struct for mac address
  team: use a larger struct for mac address

 drivers/net/bonding/bond_alb.c     |  8 ++++----
 drivers/net/bonding/bond_main.c    | 17 +++++++++--------
 drivers/net/bonding/bonding_priv.h |  6 ++++++
 drivers/net/team/team.c            |  9 ++++++---
 include/linux/netdevice.h          |  2 +-
 net/core/dev.c                     | 10 +++++++---
 net/core/dev_ioctl.c               |  2 ++
 7 files changed, 35 insertions(+), 19 deletions(-)

-- 
2.5.5

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Patch net 1/3] net: check mac address length for dev_set_mac_address()
  2017-04-26  5:03 [Patch net 0/3] net: fix a stack out-of-bound access Cong Wang
@ 2017-04-26  5:03 ` Cong Wang
  2017-04-26  5:03 ` [Patch net 2/3] bonding: use a larger struct for mac address Cong Wang
  2017-04-26  5:03 ` [Patch net 3/3] team: " Cong Wang
  2 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2017-04-26  5:03 UTC (permalink / raw)
  To: netdev; +Cc: andreyknvl, Cong Wang

dev_set_mac_address() accepts a struct sockaddr pointer as
input but we have various types of mac addresse whose lengths
are up to MAX_ADDR_LEN, this is confusing.

Make it void like ->ndo_set_mac_address() and let callers check
its length before calling it. It is too late to fix dev_ifsioc()
due to API compatiblity, so just reject those larger than
sizeof(struct sockaddr).

This is also a preparation for the following patches.

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/linux/netdevice.h |  2 +-
 net/core/dev.c            | 10 +++++++---
 net/core/dev_ioctl.c      |  2 ++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 97456b25..40e674c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3271,7 +3271,7 @@ int dev_set_alias(struct net_device *, const char *, size_t);
 int dev_change_net_namespace(struct net_device *, struct net *, const char *);
 int dev_set_mtu(struct net_device *, int);
 void dev_set_group(struct net_device *, int);
-int dev_set_mac_address(struct net_device *, struct sockaddr *);
+int dev_set_mac_address(struct net_device *, void *);
 int dev_change_carrier(struct net_device *, bool new_carrier);
 int dev_get_phys_port_id(struct net_device *dev,
 			 struct netdev_phys_item_id *ppid);
diff --git a/net/core/dev.c b/net/core/dev.c
index 533a6d6..cc670e1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6610,13 +6610,17 @@ EXPORT_SYMBOL(dev_set_group);
 /**
  *	dev_set_mac_address - Change Media Access Control Address
  *	@dev: device
- *	@sa: new address
+ *	@addr: new address, whose type could be either struct sockaddr or
+ *	any other compatible type whose length is up to MAX_ADDR_LEN depending
+ *	on the dev->addr_len. Callers should check if its length is smaller than
+ *	dev->addr_len!!
  *
  *	Change the hardware (MAC) address of the device
  */
-int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
+int dev_set_mac_address(struct net_device *dev, void *addr)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
+	struct sockaddr *sa = addr;
 	int err;
 
 	if (!ops->ndo_set_mac_address)
@@ -6625,7 +6629,7 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
-	err = ops->ndo_set_mac_address(dev, sa);
+	err = ops->ndo_set_mac_address(dev, addr);
 	if (err)
 		return err;
 	dev->addr_assign_type = NET_ADDR_SET;
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b94b1d2..11f262e 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -261,6 +261,8 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 		return dev_set_mtu(dev, ifr->ifr_mtu);
 
 	case SIOCSIFHWADDR:
+		if (dev->addr_len > sizeof(struct sockaddr))
+			return -EINVAL;
 		return dev_set_mac_address(dev, &ifr->ifr_hwaddr);
 
 	case SIOCSIFHWBROADCAST:
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch net 2/3] bonding: use a larger struct for mac address
  2017-04-26  5:03 [Patch net 0/3] net: fix a stack out-of-bound access Cong Wang
  2017-04-26  5:03 ` [Patch net 1/3] net: check mac address length for dev_set_mac_address() Cong Wang
@ 2017-04-26  5:03 ` Cong Wang
  2017-04-26  5:03 ` [Patch net 3/3] team: " Cong Wang
  2 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2017-04-26  5:03 UTC (permalink / raw)
  To: netdev; +Cc: andreyknvl, Cong Wang, Jay Vosburgh

IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len,
but in many places especially bonding, we use struct sockaddr
to copy and set mac addr, this could lead to stack out-of-bounds
access.

Fix it by using a larger address storage.

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/net/bonding/bond_alb.c     |  8 ++++----
 drivers/net/bonding/bond_main.c    | 17 +++++++++--------
 drivers/net/bonding/bonding_priv.h |  6 ++++++
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index c80b023..0038266 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -39,7 +39,7 @@
 #include <asm/byteorder.h>
 #include <net/bonding.h>
 #include <net/bond_alb.h>
-
+#include "bonding_priv.h"
 
 
 static const u8 mac_bcast[ETH_ALEN + 2] __long_aligned = {
@@ -1234,7 +1234,7 @@ static int alb_set_mac_address(struct bonding *bond, void *addr)
 {
 	struct slave *slave, *rollback_slave;
 	struct list_head *iter;
-	struct sockaddr sa;
+	struct bond_mac_addr sa;
 	char tmp_addr[ETH_ALEN];
 	int res;
 
@@ -1257,8 +1257,8 @@ static int alb_set_mac_address(struct bonding *bond, void *addr)
 	return 0;
 
 unwind:
-	memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len);
-	sa.sa_family = bond->dev->type;
+	memcpy(sa.addr, bond->dev->dev_addr, bond->dev->addr_len);
+	sa.type = bond->dev->type;
 
 	/* unwind from head to the slave that failed */
 	bond_for_each_slave(bond, rollback_slave, iter) {
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8a4ba8b..7a9bd30 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1330,7 +1330,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	struct bonding *bond = netdev_priv(bond_dev);
 	const struct net_device_ops *slave_ops = slave_dev->netdev_ops;
 	struct slave *new_slave = NULL, *prev_slave;
-	struct sockaddr addr;
+	struct bond_mac_addr addr;
 	int link_reporting;
 	int res = 0, i;
 
@@ -1488,8 +1488,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		/* Set slave to master's mac address.  The application already
 		 * set the master's mac address to that of the first slave
 		 */
-		memcpy(addr.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
-		addr.sa_family = slave_dev->type;
+		memcpy(addr.addr, bond_dev->dev_addr, bond_dev->addr_len);
+		addr.type = slave_dev->type;
 		res = dev_set_mac_address(slave_dev, &addr);
 		if (res) {
 			netdev_dbg(bond_dev, "Error %d calling set_mac_address\n", res);
@@ -1773,8 +1773,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		 * MAC if this slave's MAC is in use by the bond, or at
 		 * least print a warning.
 		 */
-		ether_addr_copy(addr.sa_data, new_slave->perm_hwaddr);
-		addr.sa_family = slave_dev->type;
+		ether_addr_copy(addr.addr, new_slave->perm_hwaddr);
+		addr.type = slave_dev->type;
 		dev_set_mac_address(slave_dev, &addr);
 	}
 
@@ -3619,7 +3619,8 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave, *rollback_slave;
-	struct sockaddr *sa = addr, tmp_sa;
+	struct sockaddr *sa = addr;
+	struct bond_mac_addr tmp_sa;
 	struct list_head *iter;
 	int res = 0;
 
@@ -3659,8 +3660,8 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
 	return 0;
 
 unwind:
-	memcpy(tmp_sa.sa_data, bond_dev->dev_addr, bond_dev->addr_len);
-	tmp_sa.sa_family = bond_dev->type;
+	memcpy(tmp_sa.addr, bond_dev->dev_addr, bond_dev->addr_len);
+	tmp_sa.type = bond_dev->type;
 
 	/* unwind from head to the slave that failed */
 	bond_for_each_slave(bond, rollback_slave, iter) {
diff --git a/drivers/net/bonding/bonding_priv.h b/drivers/net/bonding/bonding_priv.h
index 5a4d81a..8b3a7e3 100644
--- a/drivers/net/bonding/bonding_priv.h
+++ b/drivers/net/bonding/bonding_priv.h
@@ -22,4 +22,10 @@
 
 #define bond_version DRV_DESCRIPTION ": v" DRV_VERSION " (" DRV_RELDATE ")\n"
 
+/* Must be compatible with struct sockaddr */
+struct bond_mac_addr {
+	unsigned short type;
+	unsigned char addr[MAX_ADDR_LEN];
+};
+
 #endif
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26  5:03 [Patch net 0/3] net: fix a stack out-of-bound access Cong Wang
  2017-04-26  5:03 ` [Patch net 1/3] net: check mac address length for dev_set_mac_address() Cong Wang
  2017-04-26  5:03 ` [Patch net 2/3] bonding: use a larger struct for mac address Cong Wang
@ 2017-04-26  5:03 ` Cong Wang
  2017-04-26  5:40   ` Jiri Pirko
  2 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-04-26  5:03 UTC (permalink / raw)
  To: netdev; +Cc: andreyknvl, Cong Wang, Jiri Pirko

IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len,
but in many places especially bonding, we use struct sockaddr
to copy and set mac addr, this could lead to stack out-of-bounds
access.

Fix it by using a larger address storage.

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 drivers/net/team/team.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 85c0124..88878f1 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev)
 static int __set_port_dev_addr(struct net_device *port_dev,
 			       const unsigned char *dev_addr)
 {
-	struct sockaddr addr;
+	struct {
+		unsigned short type;
+		unsigned char addr[MAX_ADDR_LEN];
+	} addr;
 
-	memcpy(addr.sa_data, dev_addr, port_dev->addr_len);
-	addr.sa_family = port_dev->type;
+	memcpy(addr.addr, dev_addr, port_dev->addr_len);
+	addr.type = port_dev->type;
 	return dev_set_mac_address(port_dev, &addr);
 }
 
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26  5:03 ` [Patch net 3/3] team: " Cong Wang
@ 2017-04-26  5:40   ` Jiri Pirko
  2017-04-26 15:55     ` Jarod Wilson
  2017-04-26 16:10     ` Cong Wang
  0 siblings, 2 replies; 11+ messages in thread
From: Jiri Pirko @ 2017-04-26  5:40 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, andreyknvl

Wed, Apr 26, 2017 at 07:03:23AM CEST, xiyou.wangcong@gmail.com wrote:
>IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len,
>but in many places especially bonding, we use struct sockaddr
>to copy and set mac addr, this could lead to stack out-of-bounds
>access.
>
>Fix it by using a larger address storage.
>
>Reported-by: Andrey Konovalov <andreyknvl@google.com>
>Cc: Jiri Pirko <jiri@resnulli.us>
>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>---
> drivers/net/team/team.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index 85c0124..88878f1 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev)
> static int __set_port_dev_addr(struct net_device *port_dev,
> 			       const unsigned char *dev_addr)
> {
>-	struct sockaddr addr;
>+	struct {
>+		unsigned short type;
>+		unsigned char addr[MAX_ADDR_LEN];
>+	} addr;

Wouldn't it make sense to define this struct somewhere in the core
headers?


> 
>-	memcpy(addr.sa_data, dev_addr, port_dev->addr_len);
>-	addr.sa_family = port_dev->type;
>+	memcpy(addr.addr, dev_addr, port_dev->addr_len);
>+	addr.type = port_dev->type;
> 	return dev_set_mac_address(port_dev, &addr);
> }
> 
>-- 
>2.5.5
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26  5:40   ` Jiri Pirko
@ 2017-04-26 15:55     ` Jarod Wilson
  2017-04-26 16:11       ` Cong Wang
  2017-04-26 16:10     ` Cong Wang
  1 sibling, 1 reply; 11+ messages in thread
From: Jarod Wilson @ 2017-04-26 15:55 UTC (permalink / raw)
  To: Jiri Pirko, Cong Wang; +Cc: netdev, andreyknvl

On 2017-04-26 1:40 AM, Jiri Pirko wrote:
> Wed, Apr 26, 2017 at 07:03:23AM CEST, xiyou.wangcong@gmail.com wrote:
>> IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len,
>> but in many places especially bonding, we use struct sockaddr
>> to copy and set mac addr, this could lead to stack out-of-bounds
>> access.
>>
>> Fix it by using a larger address storage.
>>
>> Reported-by: Andrey Konovalov <andreyknvl@google.com>
>> Cc: Jiri Pirko <jiri@resnulli.us>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>> drivers/net/team/team.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>> index 85c0124..88878f1 100644
>> --- a/drivers/net/team/team.c
>> +++ b/drivers/net/team/team.c
>> @@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev)
>> static int __set_port_dev_addr(struct net_device *port_dev,
>> 			       const unsigned char *dev_addr)
>> {
>> -	struct sockaddr addr;
>> +	struct {
>> +		unsigned short type;
>> +		unsigned char addr[MAX_ADDR_LEN];
>> +	} addr;
> 
> Wouldn't it make sense to define this struct somewhere in the core
> headers?

We already have struct sockaddr_storage that could be used throughout 
this set as well. We just converted a few pieces of the bonding driver 
over to using it for better support of ipoib bonds, via commit 
faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use 
that in both bonding and team, rather than having different per-driver 
structs, or Yet Another Address Storage implementation.

-- 
Jarod Wilson
jarod@redhat.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26  5:40   ` Jiri Pirko
  2017-04-26 15:55     ` Jarod Wilson
@ 2017-04-26 16:10     ` Cong Wang
  1 sibling, 0 replies; 11+ messages in thread
From: Cong Wang @ 2017-04-26 16:10 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Andrey Konovalov

On Tue, Apr 25, 2017 at 10:40 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Apr 26, 2017 at 07:03:23AM CEST, xiyou.wangcong@gmail.com wrote:
>>IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len,
>>but in many places especially bonding, we use struct sockaddr
>>to copy and set mac addr, this could lead to stack out-of-bounds
>>access.
>>
>>Fix it by using a larger address storage.
>>
>>Reported-by: Andrey Konovalov <andreyknvl@google.com>
>>Cc: Jiri Pirko <jiri@resnulli.us>
>>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>---
>> drivers/net/team/team.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>>index 85c0124..88878f1 100644
>>--- a/drivers/net/team/team.c
>>+++ b/drivers/net/team/team.c
>>@@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev)
>> static int __set_port_dev_addr(struct net_device *port_dev,
>>                              const unsigned char *dev_addr)
>> {
>>-      struct sockaddr addr;
>>+      struct {
>>+              unsigned short type;
>>+              unsigned char addr[MAX_ADDR_LEN];
>>+      } addr;
>
> Wouldn't it make sense to define this struct somewhere in the core
> headers?

I _did_ use a struct mac_addr until I found there are multiple places
in the tree already defining it... We are in a similar situation to the union
of struct in_addr and struct in6_addr, unfortunately.

We can always clean up these for net-next.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26 15:55     ` Jarod Wilson
@ 2017-04-26 16:11       ` Cong Wang
  2017-04-26 16:46         ` Jarod Wilson
  0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-04-26 16:11 UTC (permalink / raw)
  To: Jarod Wilson
  Cc: Jiri Pirko, Linux Kernel Network Developers, Andrey Konovalov

On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote:
>
> We already have struct sockaddr_storage that could be used throughout this
> set as well. We just converted a few pieces of the bonding driver over to
> using it for better support of ipoib bonds, via commit
> faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use that
> in both bonding and team, rather than having different per-driver structs,
> or Yet Another Address Storage implementation.

Technically, struct sockaddr_storage is not enough either, given the
max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26 16:11       ` Cong Wang
@ 2017-04-26 16:46         ` Jarod Wilson
  2017-04-26 17:28           ` Cong Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Jarod Wilson @ 2017-04-26 16:46 UTC (permalink / raw)
  To: Cong Wang; +Cc: Jiri Pirko, Linux Kernel Network Developers, Andrey Konovalov

On 2017-04-26 12:11 PM, Cong Wang wrote:
> On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote:
>>
>> We already have struct sockaddr_storage that could be used throughout this
>> set as well. We just converted a few pieces of the bonding driver over to
>> using it for better support of ipoib bonds, via commit
>> faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use that
>> in both bonding and team, rather than having different per-driver structs,
>> or Yet Another Address Storage implementation.
> 
> Technically, struct sockaddr_storage is not enough either, given the
> max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage.

Wait, what? Am I missing something? MAX_ADDR_LEN is 32, and 
sockaddr_storage is a #define for __kernel_sockaddr_storage, which has 
it's __data member defined as being of size 128 - sizeof(unsigned short).

-- 
Jarod Wilson
jarod@redhat.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26 16:46         ` Jarod Wilson
@ 2017-04-26 17:28           ` Cong Wang
  2017-04-26 17:59             ` Jarod Wilson
  0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-04-26 17:28 UTC (permalink / raw)
  To: Jarod Wilson
  Cc: Jiri Pirko, Linux Kernel Network Developers, Andrey Konovalov

On Wed, Apr 26, 2017 at 9:46 AM, Jarod Wilson <jarod@redhat.com> wrote:
> On 2017-04-26 12:11 PM, Cong Wang wrote:
>>
>> On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote:
>>>
>>>
>>> We already have struct sockaddr_storage that could be used throughout
>>> this
>>> set as well. We just converted a few pieces of the bonding driver over to
>>> using it for better support of ipoib bonds, via commit
>>> faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use
>>> that
>>> in both bonding and team, rather than having different per-driver
>>> structs,
>>> or Yet Another Address Storage implementation.
>>
>>
>> Technically, struct sockaddr_storage is not enough either, given the
>> max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage.
>
>
> Wait, what? Am I missing something? MAX_ADDR_LEN is 32, and sockaddr_storage
> is a #define for __kernel_sockaddr_storage, which has it's __data member
> defined as being of size 128 - sizeof(unsigned short).

My bad, I thought it is same with sizeof(in6addr) without looking into it.
The question is, why do we waste 126 - 32 = 94 bytes on stack to just
use struct sockaddr_storage?

I totally understand we want a unified struct, but we already redefine
it in multiple places in tree...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net 3/3] team: use a larger struct for mac address
  2017-04-26 17:28           ` Cong Wang
@ 2017-04-26 17:59             ` Jarod Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Jarod Wilson @ 2017-04-26 17:59 UTC (permalink / raw)
  To: Cong Wang; +Cc: Jiri Pirko, Linux Kernel Network Developers, Andrey Konovalov

On 2017-04-26 1:28 PM, Cong Wang wrote:
> On Wed, Apr 26, 2017 at 9:46 AM, Jarod Wilson <jarod@redhat.com> wrote:
>> On 2017-04-26 12:11 PM, Cong Wang wrote:
>>>
>>> On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote:
>>>>
>>>>
>>>> We already have struct sockaddr_storage that could be used throughout
>>>> this
>>>> set as well. We just converted a few pieces of the bonding driver over to
>>>> using it for better support of ipoib bonds, via commit
>>>> faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use
>>>> that
>>>> in both bonding and team, rather than having different per-driver
>>>> structs,
>>>> or Yet Another Address Storage implementation.
>>>
>>>
>>> Technically, struct sockaddr_storage is not enough either, given the
>>> max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage.
>>
>>
>> Wait, what? Am I missing something? MAX_ADDR_LEN is 32, and sockaddr_storage
>> is a #define for __kernel_sockaddr_storage, which has it's __data member
>> defined as being of size 128 - sizeof(unsigned short).
> 
> My bad, I thought it is same with sizeof(in6addr) without looking into it.
> The question is, why do we waste 126 - 32 = 94 bytes on stack to just
> use struct sockaddr_storage?

That's a fair point.

> I totally understand we want a unified struct, but we already redefine
> it in multiple places in tree...

Something unified and centralized with a data storage of MAX_ADDR_LEN 
does seem reasonable to get both consistency and minimized waste, and I 
could certainly do a follow-up patch for the bonding driver to switch 
the bits now using sockaddr_storage over to whatever new struct gets added.

-- 
Jarod Wilson
jarod@redhat.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-04-26 17:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-26  5:03 [Patch net 0/3] net: fix a stack out-of-bound access Cong Wang
2017-04-26  5:03 ` [Patch net 1/3] net: check mac address length for dev_set_mac_address() Cong Wang
2017-04-26  5:03 ` [Patch net 2/3] bonding: use a larger struct for mac address Cong Wang
2017-04-26  5:03 ` [Patch net 3/3] team: " Cong Wang
2017-04-26  5:40   ` Jiri Pirko
2017-04-26 15:55     ` Jarod Wilson
2017-04-26 16:11       ` Cong Wang
2017-04-26 16:46         ` Jarod Wilson
2017-04-26 17:28           ` Cong Wang
2017-04-26 17:59             ` Jarod Wilson
2017-04-26 16:10     ` Cong Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.