* [patch net 1/4] mlxsw: spectrum_router: Add missing rollback
2017-07-12 7:12 [patch net 0/4] mlxsw: spectrum: Various fixes Jiri Pirko
@ 2017-07-12 7:12 ` Jiri Pirko
2017-07-12 7:12 ` [patch net 2/4] mlxsw: spectrum_router: Fix use-after-free in route replace Jiri Pirko
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jiri Pirko @ 2017-07-12 7:12 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
From: Ido Schimmel <idosch@mellanox.com>
With this patch the error path of mlxsw_sp_nexthop_init() is symmetric
with mlxsw_sp_nexthop_fini(). Noticed during code review.
Fixes: a8c970142798 ("mlxsw: spectrum_router: Refactor nexthop init routine")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 192cb93..129afc1 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -1790,6 +1790,7 @@ static int mlxsw_sp_nexthop_init(struct mlxsw_sp *mlxsw_sp,
return 0;
err_nexthop_neigh_init:
+ mlxsw_sp_nexthop_rif_fini(nh);
mlxsw_sp_nexthop_remove(mlxsw_sp, nh);
return err;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [patch net 2/4] mlxsw: spectrum_router: Fix use-after-free in route replace
2017-07-12 7:12 [patch net 0/4] mlxsw: spectrum: Various fixes Jiri Pirko
2017-07-12 7:12 ` [patch net 1/4] mlxsw: spectrum_router: Add missing rollback Jiri Pirko
@ 2017-07-12 7:12 ` Jiri Pirko
2017-07-12 7:12 ` [patch net 3/4] mlxsw: spectrum_switchdev: Remove unused variable Jiri Pirko
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jiri Pirko @ 2017-07-12 7:12 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
From: Ido Schimmel <idosch@mellanox.com>
While working on IPv6 route replace I realized we can have a
use-after-free in IPv4 in case the replaced route is offloaded and the
only one using its FIB info.
The problem is that fib_table_insert() drops the reference on the FIB
info of the replaced routes which is eventually freed via call_rcu().
Since the driver doesn't hold a reference on this FIB info it can cause
a use-after-free when it tries to clear the RTNH_F_OFFLOAD flag stored
in fi->fib_flags.
After running the following commands in a loop for enough time with a
KASAN enabled kernel I finally got the below trace.
$ ip route add 192.168.50.0/24 via 192.168.200.1 dev enp3s0np3
$ ip route replace 192.168.50.0/24 dev enp3s0np5
$ ip route del 192.168.50.0/24 dev enp3s0np5
BUG: KASAN: use-after-free in mlxsw_sp_fib_entry_offload_unset+0xa7/0x120 [mlxsw_spectrum]
Read of size 4 at addr ffff8803717d9820 by task kworker/u4:2/55
[...]
? mlxsw_sp_fib_entry_offload_unset+0xa7/0x120 [mlxsw_spectrum]
? mlxsw_sp_fib_entry_offload_unset+0xa7/0x120 [mlxsw_spectrum]
? mlxsw_sp_router_neighs_update_work+0x1cd0/0x1ce0 [mlxsw_spectrum]
? mlxsw_sp_fib_entry_offload_unset+0xa7/0x120 [mlxsw_spectrum]
__asan_load4+0x61/0x80
mlxsw_sp_fib_entry_offload_unset+0xa7/0x120 [mlxsw_spectrum]
mlxsw_sp_fib_entry_offload_refresh+0xb6/0x370 [mlxsw_spectrum]
mlxsw_sp_router_fib_event_work+0xd1c/0x2780 [mlxsw_spectrum]
[...]
Freed by task 5131:
save_stack_trace+0x16/0x20
save_stack+0x46/0xd0
kasan_slab_free+0x70/0xc0
kfree+0x144/0x570
free_fib_info_rcu+0x2e7/0x410
rcu_process_callbacks+0x4f8/0xe30
__do_softirq+0x1d3/0x9e2
Fix this by taking a reference on the FIB info when creating the nexthop
group it represents and drop it when the group is destroyed.
Fixes: 599cf8f95f22 ("mlxsw: spectrum_router: Add support for route replace")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 129afc1..383fef5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -1867,6 +1867,7 @@ mlxsw_sp_nexthop_group_create(struct mlxsw_sp *mlxsw_sp, struct fib_info *fi)
nh_grp->gateway = fi->fib_nh->nh_scope == RT_SCOPE_LINK;
nh_grp->count = fi->fib_nhs;
nh_grp->key.fi = fi;
+ fib_info_hold(fi);
for (i = 0; i < nh_grp->count; i++) {
nh = &nh_grp->nexthops[i];
fib_nh = &fi->fib_nh[i];
@@ -1886,6 +1887,7 @@ mlxsw_sp_nexthop_group_create(struct mlxsw_sp *mlxsw_sp, struct fib_info *fi)
nh = &nh_grp->nexthops[i];
mlxsw_sp_nexthop_fini(mlxsw_sp, nh);
}
+ fib_info_put(nh_grp->key.fi);
kfree(nh_grp);
return ERR_PTR(err);
}
@@ -1904,6 +1906,7 @@ mlxsw_sp_nexthop_group_destroy(struct mlxsw_sp *mlxsw_sp,
}
mlxsw_sp_nexthop_group_refresh(mlxsw_sp, nh_grp);
WARN_ON_ONCE(nh_grp->adj_index_valid);
+ fib_info_put(nh_grp->key.fi);
kfree(nh_grp);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [patch net 3/4] mlxsw: spectrum_switchdev: Remove unused variable
2017-07-12 7:12 [patch net 0/4] mlxsw: spectrum: Various fixes Jiri Pirko
2017-07-12 7:12 ` [patch net 1/4] mlxsw: spectrum_router: Add missing rollback Jiri Pirko
2017-07-12 7:12 ` [patch net 2/4] mlxsw: spectrum_router: Fix use-after-free in route replace Jiri Pirko
@ 2017-07-12 7:12 ` Jiri Pirko
2017-07-12 7:12 ` [patch net 4/4] mlxsw: spectrum_switchdev: Check status of memory allocation Jiri Pirko
2017-07-12 15:16 ` [patch net 0/4] mlxsw: spectrum: Various fixes David Miller
4 siblings, 0 replies; 6+ messages in thread
From: Jiri Pirko @ 2017-07-12 7:12 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
From: Ido Schimmel <idosch@mellanox.com>
Commit 10e23eb299fa ("mlxsw: spectrum: Remove support for bypass bridge
port attributes/vlan set") removed statements that used 'bridge_vlan',
but didn't remove the variable itself resulting in the following warning
with W=1:
warning: variable ‘bridge_vlan’ set but not used
[-Wunused-but-set-variable]
Remove the variable and suppress the warning.
Fixes: 10e23eb299fa ("mlxsw: spectrum: Remove support for bypass bridge port attributes/vlan set")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index cd89a3e..4733fe9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -979,7 +979,6 @@ mlxsw_sp_bridge_port_vlan_add(struct mlxsw_sp_port *mlxsw_sp_port,
{
u16 pvid = mlxsw_sp_port_pvid_determine(mlxsw_sp_port, vid, is_pvid);
struct mlxsw_sp_port_vlan *mlxsw_sp_port_vlan;
- struct mlxsw_sp_bridge_vlan *bridge_vlan;
u16 old_pvid = mlxsw_sp_port->pvid;
int err;
@@ -1000,8 +999,6 @@ mlxsw_sp_bridge_port_vlan_add(struct mlxsw_sp_port *mlxsw_sp_port,
if (err)
goto err_port_vlan_bridge_join;
- bridge_vlan = mlxsw_sp_bridge_vlan_find(bridge_port, vid);
-
return 0;
err_port_vlan_bridge_join:
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [patch net 4/4] mlxsw: spectrum_switchdev: Check status of memory allocation
2017-07-12 7:12 [patch net 0/4] mlxsw: spectrum: Various fixes Jiri Pirko
` (2 preceding siblings ...)
2017-07-12 7:12 ` [patch net 3/4] mlxsw: spectrum_switchdev: Remove unused variable Jiri Pirko
@ 2017-07-12 7:12 ` Jiri Pirko
2017-07-12 15:16 ` [patch net 0/4] mlxsw: spectrum: Various fixes David Miller
4 siblings, 0 replies; 6+ messages in thread
From: Jiri Pirko @ 2017-07-12 7:12 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
From: Ido Schimmel <idosch@mellanox.com>
We can't rely on kzalloc() always succeeding, so check its return value.
Suppresses the following smatch error:
mlxsw_sp_switchdev_event() error: potential null dereference
'switchdev_work->fdb_info.addr'. (kzalloc returns
null)
Fixes: af061378924f ("mlxsw: spectrum_switchdev: Add support for learning FDB through notification")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 4733fe9..656b2d3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -1916,6 +1916,8 @@ static int mlxsw_sp_switchdev_event(struct notifier_block *unused,
memcpy(&switchdev_work->fdb_info, ptr,
sizeof(switchdev_work->fdb_info));
switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
+ if (!switchdev_work->fdb_info.addr)
+ goto err_addr_alloc;
ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
fdb_info->addr);
/* Take a reference on the device. This can be either
@@ -1932,6 +1934,10 @@ static int mlxsw_sp_switchdev_event(struct notifier_block *unused,
mlxsw_core_schedule_work(&switchdev_work->work);
return NOTIFY_DONE;
+
+err_addr_alloc:
+ kfree(switchdev_work);
+ return NOTIFY_BAD;
}
static struct notifier_block mlxsw_sp_switchdev_notifier = {
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch net 0/4] mlxsw: spectrum: Various fixes
2017-07-12 7:12 [patch net 0/4] mlxsw: spectrum: Various fixes Jiri Pirko
` (3 preceding siblings ...)
2017-07-12 7:12 ` [patch net 4/4] mlxsw: spectrum_switchdev: Check status of memory allocation Jiri Pirko
@ 2017-07-12 15:16 ` David Miller
4 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2017-07-12 15:16 UTC (permalink / raw)
To: jiri; +Cc: netdev, idosch, mlxsw
From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 12 Jul 2017 09:12:51 +0200
> First patch adds a missing rollback in error path. Second patch prevents
> a use-after-free during IPv4 route replace. Last two patches fix warnings
> from static checkers.
Series applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread