* [PATCH net] net: marvell: prestera: fix brige port operation
@ 2021-11-17 9:43 Volodymyr Mytnyk
2021-11-17 17:10 ` Vladimir Oltean
0 siblings, 1 reply; 3+ messages in thread
From: Volodymyr Mytnyk @ 2021-11-17 9:43 UTC (permalink / raw)
To: netdev
Cc: mickeyr, serhiy.pshyk, taras.chornyi, Volodymyr Mytnyk,
Taras Chornyi, David S. Miller, Jakub Kicinski, Vladimir Oltean,
Ioana Ciornei, linux-kernel
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);
}
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;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] net: marvell: prestera: fix brige port operation
2021-11-17 9:43 [PATCH net] net: marvell: prestera: fix brige port operation Volodymyr Mytnyk
@ 2021-11-17 17:10 ` Vladimir Oltean
2021-11-18 14:28 ` Volodymyr Mytnyk
0 siblings, 1 reply; 3+ messages in thread
From: Vladimir Oltean @ 2021-11-17 17:10 UTC (permalink / raw)
To: Volodymyr Mytnyk
Cc: netdev, mickeyr, serhiy.pshyk, taras.chornyi, Volodymyr Mytnyk,
Taras Chornyi, David S. Miller, Jakub Kicinski, Ioana Ciornei,
linux-kernel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH net] net: marvell: prestera: fix brige port operation
2021-11-17 17:10 ` Vladimir Oltean
@ 2021-11-18 14:28 ` Volodymyr Mytnyk
0 siblings, 0 replies; 3+ messages in thread
From: Volodymyr Mytnyk @ 2021-11-18 14:28 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, mickeyr, Serhiy Pshyk, Taras Chornyi, Volodymyr Mytnyk,
Taras Chornyi, David S. Miller, Jakub Kicinski, Ioana Ciornei,
linux-kernel
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-18 14:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 9:43 [PATCH net] net: marvell: prestera: fix brige port operation Volodymyr Mytnyk
2021-11-17 17:10 ` Vladimir Oltean
2021-11-18 14:28 ` Volodymyr Mytnyk
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).