| Message ID | 1637142232-19344-1-git-send-email-volodymyr.mytnyk@plvision.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: marvell: prestera: fix brige port operation | expand |
On Wed, Nov 17, 2021 at 11:43:51AM +0200, Volodymyr Mytnyk wrote: > From: Volodymyr Mytnyk <vmytnyk@marvell.com> > > - handle SWITCHDEV_BRPORT_[UN]OFFLOADED events for > switchdev_bridge_port_offload to avoid fail return. > - fix error path handling in prestera_bridge_port_join to > fix double free issue (see below). > > Trace: > Internal error: Oops: 96000044 [#1] SMP > Modules linked in: prestera_pci prestera uio_pdrv_genirq > CPU: 1 PID: 881 Comm: ip Not tainted 5.15.0 #1 > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : prestera_bridge_destroy+0x2c/0xb0 [prestera] > lr : prestera_bridge_port_join+0x2cc/0x350 [prestera] > sp : ffff800011a1b0f0 > ... > x2 : ffff000109ca6c80 x1 : dead000000000100 x0 : dead000000000122 > Call trace: > prestera_bridge_destroy+0x2c/0xb0 [prestera] > prestera_bridge_port_join+0x2cc/0x350 [prestera] > prestera_netdev_port_event.constprop.0+0x3c4/0x450 [prestera] > prestera_netdev_event_handler+0xf4/0x110 [prestera] > raw_notifier_call_chain+0x54/0x80 > call_netdevice_notifiers_info+0x54/0xa0 > __netdev_upper_dev_link+0x19c/0x380 > > Fixes: 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which bridge ports are offloaded") > Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com> > --- > drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > index 3ce6ccd0f539..f1bc6699ec8b 100644 > --- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > +++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > @@ -497,8 +497,8 @@ int prestera_bridge_port_join(struct net_device *br_dev, > > br_port = prestera_bridge_port_add(bridge, port->dev); > if (IS_ERR(br_port)) { > - err = PTR_ERR(br_port); > - goto err_brport_create; > + prestera_bridge_put(bridge); > + return PTR_ERR(br_port); > } Here is how the function looked _before_ my patch: int prestera_bridge_port_join(struct net_device *br_dev, struct prestera_port *port) { struct prestera_switchdev *swdev = port->sw->swdev; struct prestera_bridge_port *br_port; struct prestera_bridge *bridge; int err; bridge = prestera_bridge_by_dev(swdev, br_dev); if (!bridge) { bridge = prestera_bridge_create(swdev, br_dev); if (IS_ERR(bridge)) return PTR_ERR(bridge); } br_port = prestera_bridge_port_add(bridge, port->dev); if (IS_ERR(br_port)) { err = PTR_ERR(br_port); goto err_brport_create; } if (bridge->vlan_enabled) return 0; err = prestera_bridge_1d_port_join(br_port); if (err) goto err_port_join; return 0; err_port_join: prestera_bridge_port_put(br_port); err_brport_create: prestera_bridge_put(bridge); return err; } The double free is due to the fact that prestera_bridge_port_put() calls prestera_bridge_put() by itself too. But the code was already buggy, for example the error path of prestera_bridge_1d_port_join() would trigger this double free as well. The change itself is ok (but is very poorly explained), if prestera_bridge_port_add() fails, you want to undo just prestera_bridge_create(), otherwise, prestera_bridge_port_put() will undo both prestera_bridge_port_add() and prestera_bridge_create(). So the honest Fixes: tag should be: Fixes: e1189d9a5fbe ("net: marvell: prestera: Add Switchdev driver implementation") because you want this change to be backported even to stable kernels where commit 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which bridge ports are offloaded") is not present. > > err = switchdev_bridge_port_offload(br_port->dev, port->dev, NULL, > @@ -519,8 +519,6 @@ int prestera_bridge_port_join(struct net_device *br_dev, > switchdev_bridge_port_unoffload(br_port->dev, NULL, NULL, NULL); > err_switchdev_offload: > prestera_bridge_port_put(br_port); > -err_brport_create: > - prestera_bridge_put(bridge); > return err; > } > > @@ -1123,6 +1121,9 @@ static int prestera_switchdev_blk_event(struct notifier_block *unused, > prestera_netdev_check, > prestera_port_obj_attr_set); > break; > + case SWITCHDEV_BRPORT_OFFLOADED: > + case SWITCHDEV_BRPORT_UNOFFLOADED: > + return NOTIFY_DONE; > default: > err = -EOPNOTSUPP; May I suggest that the root cause of the problem is that you're returning -EOPNOTSUPP here? The switchdev events may just as well not be destined for your prestera switch. You should return NOTIFY_DONE ("don't care") for event types you don't know how to handle. And technically, this part of the patch should have: Fixes: 957e2235e526 ("net: make switchdev_bridge_port_{,unoffload} loosely coupled with the bridge") > } > -- > 2.7.4 >
> On Wed, Nov 17, 2021 at 11:43:51AM +0200, Volodymyr Mytnyk wrote: > > From: Volodymyr Mytnyk <vmytnyk@marvell.com> > > > > - handle SWITCHDEV_BRPORT_[UN]OFFLOADED events for > > switchdev_bridge_port_offload to avoid fail return. > > - fix error path handling in prestera_bridge_port_join to > > fix double free issue (see below). > > > > Trace: > > Internal error: Oops: 96000044 [#1] SMP > > Modules linked in: prestera_pci prestera uio_pdrv_genirq > > CPU: 1 PID: 881 Comm: ip Not tainted 5.15.0 #1 > > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > pc : prestera_bridge_destroy+0x2c/0xb0 [prestera] > > lr : prestera_bridge_port_join+0x2cc/0x350 [prestera] > > sp : ffff800011a1b0f0 > > ... > > x2 : ffff000109ca6c80 x1 : dead000000000100 x0 : dead000000000122 > > Call trace: > > prestera_bridge_destroy+0x2c/0xb0 [prestera] > > prestera_bridge_port_join+0x2cc/0x350 [prestera] > > prestera_netdev_port_event.constprop.0+0x3c4/0x450 [prestera] > > prestera_netdev_event_handler+0xf4/0x110 [prestera] > > raw_notifier_call_chain+0x54/0x80 > > call_netdevice_notifiers_info+0x54/0xa0 > > __netdev_upper_dev_link+0x19c/0x380 > > > > Fixes: 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which > > bridge ports are offloaded") > > Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com> > > --- > > drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 9 > > +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git > > a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > > b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > > index 3ce6ccd0f539..f1bc6699ec8b 100644 > > --- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > > +++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c > > @@ -497,8 +497,8 @@ int prestera_bridge_port_join(struct net_device > > *br_dev, > > > > br_port = prestera_bridge_port_add(bridge, port->dev); > > if (IS_ERR(br_port)) { > > - err = PTR_ERR(br_port); > > - goto err_brport_create; > > + prestera_bridge_put(bridge); > > + return PTR_ERR(br_port); > > } > > Here is how the function looked _before_ my patch: > > int prestera_bridge_port_join(struct net_device *br_dev, > struct prestera_port *port) > { > struct prestera_switchdev *swdev = port->sw->swdev; > struct prestera_bridge_port *br_port; > struct prestera_bridge *bridge; > int err; > > bridge = prestera_bridge_by_dev(swdev, br_dev); > if (!bridge) { > bridge = prestera_bridge_create(swdev, br_dev); > if (IS_ERR(bridge)) > return PTR_ERR(bridge); > } > > br_port = prestera_bridge_port_add(bridge, port->dev); > if (IS_ERR(br_port)) { > err = PTR_ERR(br_port); > goto err_brport_create; > } > > if (bridge->vlan_enabled) > return 0; > > err = prestera_bridge_1d_port_join(br_port); > if (err) > goto err_port_join; > > return 0; > > err_port_join: > prestera_bridge_port_put(br_port); > err_brport_create: > prestera_bridge_put(bridge); > return err; > } > > The double free is due to the fact that prestera_bridge_port_put() calls > prestera_bridge_put() by itself too. > > But the code was already buggy, for example the error path of > prestera_bridge_1d_port_join() would trigger this double free as well. > The change itself is ok (but is very poorly explained), if > prestera_bridge_port_add() fails, you want to undo just prestera_bridge_create(), otherwise, prestera_bridge_port_put() will undo both prestera_bridge_port_add() and prestera_bridge_create(). > > So the honest Fixes: tag should be: > > Fixes: e1189d9a5fbe ("net: marvell: prestera: Add Switchdev driver implementation") Right, the double free was before. Will change this the commit tag with e1189d9a5fbe. Thx. > > because you want this change to be backported even to stable kernels where commit 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which bridge ports are offloaded") is not present. > > > > > err = switchdev_bridge_port_offload(br_port->dev, port->dev, NULL, > > @@ -519,8 +519,6 @@ int prestera_bridge_port_join(struct net_device *br_dev, > > switchdev_bridge_port_unoffload(br_port->dev, NULL, NULL, NULL); > > err_switchdev_offload: > > prestera_bridge_port_put(br_port); > > -err_brport_create: > > - prestera_bridge_put(bridge); > > return err; > > } > > > > @@ -1123,6 +1121,9 @@ static int prestera_switchdev_blk_event(struct notifier_block *unused, > > prestera_netdev_check, > > prestera_port_obj_attr_set); > > break; > > + case SWITCHDEV_BRPORT_OFFLOADED: > > + case SWITCHDEV_BRPORT_UNOFFLOADED: > > + return NOTIFY_DONE; > > default: > > err = -EOPNOTSUPP; > > May I suggest that the root cause of the problem is that you're returning -EOPNOTSUPP here? The switchdev events may just as well not be destined for your prestera switch. You should return NOTIFY_DONE ("don't > care") for event types you don't know how to handle. Yes, I think NOTIFY_DONE can be returned by default. It makes sense to split this change into two commits with their fix tag set respectively. > > And technically, this part of the patch should have: > Fixes: 957e2235e526 ("net: make switchdev_bridge_port_{,unoffload} loosely coupled with the bridge") > Regards, Volodymyr
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c index 3ce6ccd0f539..f1bc6699ec8b 100644 --- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c +++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c @@ -497,8 +497,8 @@ int prestera_bridge_port_join(struct net_device *br_dev, br_port = prestera_bridge_port_add(bridge, port->dev); if (IS_ERR(br_port)) { - err = PTR_ERR(br_port); - goto err_brport_create; + prestera_bridge_put(bridge); + return PTR_ERR(br_port); } err = switchdev_bridge_port_offload(br_port->dev, port->dev, NULL, @@ -519,8 +519,6 @@ int prestera_bridge_port_join(struct net_device *br_dev, switchdev_bridge_port_unoffload(br_port->dev, NULL, NULL, NULL); err_switchdev_offload: prestera_bridge_port_put(br_port); -err_brport_create: - prestera_bridge_put(bridge); return err; } @@ -1123,6 +1121,9 @@ static int prestera_switchdev_blk_event(struct notifier_block *unused, prestera_netdev_check, prestera_port_obj_attr_set); break; + case SWITCHDEV_BRPORT_OFFLOADED: + case SWITCHDEV_BRPORT_UNOFFLOADED: + return NOTIFY_DONE; default: err = -EOPNOTSUPP; }