* [PATCH 1/2] bonding: fix netdev event NULL pointer dereference
@ 2013-04-09 20:17 Nikolay Aleksandrov
2013-04-09 20:17 ` [PATCH 2/2] bonding: IFF_BONDING is not stripped on enslave failure Nikolay Aleksandrov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2013-04-09 20:17 UTC (permalink / raw)
To: netdev; +Cc: andy, fubar, davem
In commit 471cb5a33dcbd7c529684a2ac7ba4451414ee4a7 ("bonding: remove
usage of dev->master") a bug was introduced which causes a NULL pointer
dereference. If a bond device is in mode 6 (ALB) and a slave is added
it will dereference a NULL pointer in bond_slave_netdev_event().
This is because in bond_enslave we have bond_alb_init_slave() which
changes the MAC address of the slave and causes a NETDEV_CHANGEADDR.
Then we have in bond_slave_netdev_event():
struct slave *slave = bond_slave_get_rtnl(slave_dev);
struct bonding *bond = slave->bond;
bond_slave_get_rtnl() dereferences slave_dev->rx_handler_data which at
that time is NULL since netdev_rx_handler_register() is called later.
This is fixed by checking if slave is NULL before dereferencing it.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_main.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 171b10f..9995ddc 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3168,11 +3168,21 @@ static int bond_slave_netdev_event(unsigned long event,
struct net_device *slave_dev)
{
struct slave *slave = bond_slave_get_rtnl(slave_dev);
- struct bonding *bond = slave->bond;
- struct net_device *bond_dev = slave->bond->dev;
+ struct bonding *bond;
+ struct net_device *bond_dev;
u32 old_speed;
u8 old_duplex;
+ /*
+ * A netdev event can be generated while enslaving a device
+ * before netdev_rx_handler_register is called in which case
+ * slave will be NULL
+ */
+ if (!slave)
+ return NOTIFY_DONE;
+ bond_dev = slave->bond->dev;
+ bond = slave->bond;
+
switch (event) {
case NETDEV_UNREGISTER:
if (bond->setup_by_slave)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] bonding: IFF_BONDING is not stripped on enslave failure
2013-04-09 20:17 [PATCH 1/2] bonding: fix netdev event NULL pointer dereference Nikolay Aleksandrov
@ 2013-04-09 20:17 ` Nikolay Aleksandrov
2013-04-11 13:40 ` [PATCH 1/2] bonding: fix netdev event NULL pointer dereference Sergei Shtylyov
2013-04-11 13:45 ` [PATCHv2 " Nikolay Aleksandrov
2 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2013-04-09 20:17 UTC (permalink / raw)
To: netdev; +Cc: andy, fubar, davem
While enslaving a new device and after IFF_BONDING flag is set, in case
of failure it is not stripped from the device's priv_flags while
cleaning up, which could lead to other problems.
Cleaning at err_close because the flag is set after dev_open().
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9995ddc..0d9ee48 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1906,6 +1906,7 @@ err_detach:
write_unlock_bh(&bond->lock);
err_close:
+ slave_dev->priv_flags &= ~IFF_BONDING;
dev_close(slave_dev);
err_unset_master:
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] bonding: fix netdev event NULL pointer dereference
2013-04-09 20:17 [PATCH 1/2] bonding: fix netdev event NULL pointer dereference Nikolay Aleksandrov
2013-04-09 20:17 ` [PATCH 2/2] bonding: IFF_BONDING is not stripped on enslave failure Nikolay Aleksandrov
@ 2013-04-11 13:40 ` Sergei Shtylyov
2013-04-11 13:45 ` [PATCHv2 " Nikolay Aleksandrov
2 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2013-04-11 13:40 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, andy, fubar, davem
Hello.
On 10-04-2013 0:17, Nikolay Aleksandrov wrote:
> In commit 471cb5a33dcbd7c529684a2ac7ba4451414ee4a7 ("bonding: remove
> usage of dev->master") a bug was introduced which causes a NULL pointer
> dereference. If a bond device is in mode 6 (ALB) and a slave is added
> it will dereference a NULL pointer in bond_slave_netdev_event().
> This is because in bond_enslave we have bond_alb_init_slave() which
> changes the MAC address of the slave and causes a NETDEV_CHANGEADDR.
> Then we have in bond_slave_netdev_event():
> struct slave *slave = bond_slave_get_rtnl(slave_dev);
> struct bonding *bond = slave->bond;
> bond_slave_get_rtnl() dereferences slave_dev->rx_handler_data which at
> that time is NULL since netdev_rx_handler_register() is called later.
> This is fixed by checking if slave is NULL before dereferencing it.
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
[...]
Minor formatting nit.
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 171b10f..9995ddc 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3168,11 +3168,21 @@ static int bond_slave_netdev_event(unsigned long event,
> struct net_device *slave_dev)
> {
> struct slave *slave = bond_slave_get_rtnl(slave_dev);
> - struct bonding *bond = slave->bond;
> - struct net_device *bond_dev = slave->bond->dev;
> + struct bonding *bond;
> + struct net_device *bond_dev;
> u32 old_speed;
> u8 old_duplex;
>
> + /*
> + * A netdev event can be generated while enslaving a device
> + * before netdev_rx_handler_register is called in which case
> + * slave will be NULL
> + */
The preferred multi-line comment style in the networking code is:
/* bla
* bla
*/
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2 1/2] bonding: fix netdev event NULL pointer dereference
2013-04-09 20:17 [PATCH 1/2] bonding: fix netdev event NULL pointer dereference Nikolay Aleksandrov
2013-04-09 20:17 ` [PATCH 2/2] bonding: IFF_BONDING is not stripped on enslave failure Nikolay Aleksandrov
2013-04-11 13:40 ` [PATCH 1/2] bonding: fix netdev event NULL pointer dereference Sergei Shtylyov
@ 2013-04-11 13:45 ` Nikolay Aleksandrov
2013-04-11 17:28 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2013-04-11 13:45 UTC (permalink / raw)
To: netdev; +Cc: andy, fubar, davem
In commit 471cb5a33dcbd7c529684a2ac7ba4451414ee4a7 ("bonding: remove
usage of dev->master") a bug was introduced which causes a NULL pointer
dereference. If a bond device is in mode 6 (ALB) and a slave is added
it will dereference a NULL pointer in bond_slave_netdev_event().
This is because in bond_enslave we have bond_alb_init_slave() which
changes the MAC address of the slave and causes a NETDEV_CHANGEADDR.
Then we have in bond_slave_netdev_event():
struct slave *slave = bond_slave_get_rtnl(slave_dev);
struct bonding *bond = slave->bond;
bond_slave_get_rtnl() dereferences slave_dev->rx_handler_data which at
that time is NULL since netdev_rx_handler_register() is called later.
This is fixed by checking if slave is NULL before dereferencing it.
v2: Fix the comment styling
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_main.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 171b10f..15c675c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3168,11 +3168,20 @@ static int bond_slave_netdev_event(unsigned long event,
struct net_device *slave_dev)
{
struct slave *slave = bond_slave_get_rtnl(slave_dev);
- struct bonding *bond = slave->bond;
- struct net_device *bond_dev = slave->bond->dev;
+ struct bonding *bond;
+ struct net_device *bond_dev;
u32 old_speed;
u8 old_duplex;
+ /* A netdev event can be generated while enslaving a device
+ * before netdev_rx_handler_register is called in which case
+ * slave will be NULL
+ */
+ if (!slave)
+ return NOTIFY_DONE;
+ bond_dev = slave->bond->dev;
+ bond = slave->bond;
+
switch (event) {
case NETDEV_UNREGISTER:
if (bond->setup_by_slave)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2 1/2] bonding: fix netdev event NULL pointer dereference
2013-04-11 13:45 ` [PATCHv2 " Nikolay Aleksandrov
@ 2013-04-11 17:28 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2013-04-11 17:28 UTC (permalink / raw)
To: nikolay; +Cc: netdev, andy, fubar
When you repost a patch that's part of a series, you must resubmit all
patches in that series, not just the one that changed.
You should also provide a header posting "[PATCH 0/N] ..." that explains
the high level purpose and intent of the patch set.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2 1/2] bonding: fix netdev event NULL pointer dereference
2013-04-11 19:18 [PATCHv2 0/2] bonding: NULL pointer dereference and flag strip fixes Nikolay Aleksandrov
@ 2013-04-11 19:18 ` Nikolay Aleksandrov
0 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2013-04-11 19:18 UTC (permalink / raw)
To: netdev; +Cc: andy, fubar, davem
In commit 471cb5a33dcbd7c529684a2ac7ba4451414ee4a7 ("bonding: remove
usage of dev->master") a bug was introduced which causes a NULL pointer
dereference. If a bond device is in mode 6 (ALB) and a slave is added
it will dereference a NULL pointer in bond_slave_netdev_event().
This is because in bond_enslave we have bond_alb_init_slave() which
changes the MAC address of the slave and causes a NETDEV_CHANGEADDR.
Then we have in bond_slave_netdev_event():
struct slave *slave = bond_slave_get_rtnl(slave_dev);
struct bonding *bond = slave->bond;
bond_slave_get_rtnl() dereferences slave_dev->rx_handler_data which at
that time is NULL since netdev_rx_handler_register() is called later.
This is fixed by checking if slave is NULL before dereferencing it.
v2: Comment style changed.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_main.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 171b10f..15c675c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3168,11 +3168,20 @@ static int bond_slave_netdev_event(unsigned long event,
struct net_device *slave_dev)
{
struct slave *slave = bond_slave_get_rtnl(slave_dev);
- struct bonding *bond = slave->bond;
- struct net_device *bond_dev = slave->bond->dev;
+ struct bonding *bond;
+ struct net_device *bond_dev;
u32 old_speed;
u8 old_duplex;
+ /* A netdev event can be generated while enslaving a device
+ * before netdev_rx_handler_register is called in which case
+ * slave will be NULL
+ */
+ if (!slave)
+ return NOTIFY_DONE;
+ bond_dev = slave->bond->dev;
+ bond = slave->bond;
+
switch (event) {
case NETDEV_UNREGISTER:
if (bond->setup_by_slave)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-04-11 19:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-09 20:17 [PATCH 1/2] bonding: fix netdev event NULL pointer dereference Nikolay Aleksandrov
2013-04-09 20:17 ` [PATCH 2/2] bonding: IFF_BONDING is not stripped on enslave failure Nikolay Aleksandrov
2013-04-11 13:40 ` [PATCH 1/2] bonding: fix netdev event NULL pointer dereference Sergei Shtylyov
2013-04-11 13:45 ` [PATCHv2 " Nikolay Aleksandrov
2013-04-11 17:28 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2013-04-11 19:18 [PATCHv2 0/2] bonding: NULL pointer dereference and flag strip fixes Nikolay Aleksandrov
2013-04-11 19:18 ` [PATCHv2 1/2] bonding: fix netdev event NULL pointer dereference Nikolay Aleksandrov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).