| Message ID | 20180527064841.32199-1-idosch@mellanox.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] mlxsw: spectrum: Forbid creation of VLAN 1 over port/LAG | expand |
Sun, May 27, 2018 at 08:48:41AM CEST, idosch@mellanox.com wrote: >From: Petr Machata <petrm@mellanox.com> > >VLAN 1 is internally used for untagged traffic. Prevent creation of >explicit netdevice for that VLAN, because that currently isn't supported >and leads to the NULL pointer dereference cited below. > >Fix by preventing creation of VLAN devices with VID of 1 over mlxsw >devices or LAG devices that involve mlxsw devices. > >[ 327.175816] ================================================================================ >[ 327.184544] UBSAN: Undefined behaviour in drivers/net/ethernet/mellanox/mlxsw/spectrum_fid.c:200:12 >[ 327.193667] member access within null pointer of type 'const struct mlxsw_sp_fid' >[ 327.201226] CPU: 0 PID: 8983 Comm: ip Not tainted 4.17.0-rc4-petrm_net_ip6gre_headroom-custom-140 #11 >[ 327.210496] Hardware name: Mellanox Technologies Ltd. "MSN2410-CB2F"/"SA000874", BIOS 4.6.5 03/08/2016 >[ 327.219872] Call Trace: >[ 327.222384] dump_stack+0xc3/0x12b >[ 327.234007] ubsan_epilogue+0x9/0x49 >[ 327.237638] ubsan_type_mismatch_common+0x1f9/0x2d0 >[ 327.255769] __ubsan_handle_type_mismatch+0x90/0xa7 >[ 327.264716] mlxsw_sp_fid_type+0x35/0x50 [mlxsw_spectrum] >[ 327.270255] mlxsw_sp_port_vlan_router_leave+0x46/0xc0 [mlxsw_spectrum] >[ 327.277019] mlxsw_sp_inetaddr_port_vlan_event+0xe1/0x340 [mlxsw_spectrum] >[ 327.315031] mlxsw_sp_netdevice_vrf_event+0xa8/0x100 [mlxsw_spectrum] >[ 327.321626] mlxsw_sp_netdevice_event+0x276/0x430 [mlxsw_spectrum] >[ 327.367863] notifier_call_chain+0x4c/0x150 >[ 327.372128] __netdev_upper_dev_link+0x1b3/0x260 >[ 327.399450] vrf_add_slave+0xce/0x170 [vrf] >[ 327.403703] do_setlink+0x658/0x1d70 >[ 327.508998] rtnl_newlink+0x908/0xf20 >[ 327.559128] rtnetlink_rcv_msg+0x50c/0x720 >[ 327.571720] netlink_rcv_skb+0x16a/0x1f0 >[ 327.583450] netlink_unicast+0x2ca/0x3e0 >[ 327.599305] netlink_sendmsg+0x3e2/0x7f0 >[ 327.616655] sock_sendmsg+0x76/0xc0 >[ 327.620207] ___sys_sendmsg+0x494/0x5d0 >[ 327.666117] __sys_sendmsg+0xc2/0x130 >[ 327.690953] do_syscall_64+0x66/0x370 >[ 327.694677] entry_SYSCALL_64_after_hwframe+0x49/0xbe >[ 327.699782] RIP: 0033:0x7f4c2f3f8037 >[ 327.703393] RSP: 002b:00007ffe8c389708 EFLAGS: 00000246 ORIG_RAX: 000000000000002e >[ 327.711035] RAX: ffffffffffffffda RBX: 000000005b03f53e RCX: 00007f4c2f3f8037 >[ 327.718229] RDX: 0000000000000000 RSI: 00007ffe8c389760 RDI: 0000000000000003 >[ 327.725431] RBP: 00007ffe8c389760 R08: 0000000000000000 R09: 00007f4c2f443630 >[ 327.732632] R10: 00000000000005eb R11: 0000000000000246 R12: 0000000000000000 >[ 327.739833] R13: 00000000006774e0 R14: 00007ffe8c3897e8 R15: 0000000000000000 >[ 327.747096] ================================================================================ > >Fixes: 9589a7b5d7d9 ("mlxsw: spectrum: Handle VLAN devices linking / unlinking") >Suggested-by: Ido Schimmel <idosch@mellanox.com> >Signed-off-by: Petr Machata <petrm@mellanox.com> >Signed-off-by: Ido Schimmel <idosch@mellanox.com> Acked-by: Jiri Pirko <jiri@mellanox.com>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > index ca38a30fbe91..adc6ab2cf429 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > @@ -4433,6 +4433,11 @@ static int mlxsw_sp_netdevice_port_upper_event(struct net_device *lower_dev, > NL_SET_ERR_MSG_MOD(extack, "Can not put a VLAN on an OVS port"); > return -EINVAL; > } > + if (is_vlan_dev(upper_dev) && > + vlan_dev_vlan_id(upper_dev) == 1) { > + NL_SET_ERR_MSG_MOD(extack, "Creating a VLAN device with VID 1 is unsupported: VLAN 1 carries untagged traffic"); > + return -EINVAL; > + } Hi Ido Would ENOTSUPP be a better return code. VLAN 1 is valid, you just don't support it. Andrew
On Mon, May 28, 2018 at 05:55:58AM +0200, Andrew Lunn wrote: > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > > index ca38a30fbe91..adc6ab2cf429 100644 > > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > > @@ -4433,6 +4433,11 @@ static int mlxsw_sp_netdevice_port_upper_event(struct net_device *lower_dev, > > NL_SET_ERR_MSG_MOD(extack, "Can not put a VLAN on an OVS port"); > > return -EINVAL; > > } > > + if (is_vlan_dev(upper_dev) && > > + vlan_dev_vlan_id(upper_dev) == 1) { > > + NL_SET_ERR_MSG_MOD(extack, "Creating a VLAN device with VID 1 is unsupported: VLAN 1 carries untagged traffic"); > > + return -EINVAL; > > + } > > Hi Ido > > Would ENOTSUPP be a better return code. VLAN 1 is valid, you just > don't support it. OK, makes sense. We currently use EINVAL for such errors, but we can convert to EOPNOTSUPP in net-next. Thanks
Ido Schimmel <idosch@idosch.org> writes: > On Mon, May 28, 2018 at 05:55:58AM +0200, Andrew Lunn wrote: >> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c >> > index ca38a30fbe91..adc6ab2cf429 100644 >> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c >> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c >> > @@ -4433,6 +4433,11 @@ static int mlxsw_sp_netdevice_port_upper_event(struct net_device *lower_dev, >> > NL_SET_ERR_MSG_MOD(extack, "Can not put a VLAN on an OVS port"); >> > return -EINVAL; >> > } >> > + if (is_vlan_dev(upper_dev) && >> > + vlan_dev_vlan_id(upper_dev) == 1) { >> > + NL_SET_ERR_MSG_MOD(extack, "Creating a VLAN device with VID 1 is unsupported: VLAN 1 carries untagged traffic"); >> > + return -EINVAL; >> > + } >> >> Would ENOTSUPP be a better return code. VLAN 1 is valid, you just >> don't support it. > > OK, makes sense. We currently use EINVAL for such errors, but we can > convert to EOPNOTSUPP in net-next. Yep, agreed. Thanks, Petr
From: Ido Schimmel <idosch@mellanox.com> Date: Sun, 27 May 2018 09:48:41 +0300 > From: Petr Machata <petrm@mellanox.com> > > VLAN 1 is internally used for untagged traffic. Prevent creation of > explicit netdevice for that VLAN, because that currently isn't supported > and leads to the NULL pointer dereference cited below. > > Fix by preventing creation of VLAN devices with VID of 1 over mlxsw > devices or LAG devices that involve mlxsw devices. ... > Fixes: 9589a7b5d7d9 ("mlxsw: spectrum: Handle VLAN devices linking / unlinking") > Suggested-by: Ido Schimmel <idosch@mellanox.com> > Signed-off-by: Petr Machata <petrm@mellanox.com> > Signed-off-by: Ido Schimmel <idosch@mellanox.com> Applied and queued up for -stable.
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c index ca38a30fbe91..adc6ab2cf429 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c @@ -4433,6 +4433,11 @@ static int mlxsw_sp_netdevice_port_upper_event(struct net_device *lower_dev, NL_SET_ERR_MSG_MOD(extack, "Can not put a VLAN on an OVS port"); return -EINVAL; } + if (is_vlan_dev(upper_dev) && + vlan_dev_vlan_id(upper_dev) == 1) { + NL_SET_ERR_MSG_MOD(extack, "Creating a VLAN device with VID 1 is unsupported: VLAN 1 carries untagged traffic"); + return -EINVAL; + } break; case NETDEV_CHANGEUPPER: upper_dev = info->upper_dev;