[PATCH net] net: marvell: prestera: fix brige port operation

[PATCH net] net: marvell: prestera: fix brige port operation

[Volodymyr Mytnyk]

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

Re: [PATCH net] net: marvell: prestera: fix brige port operation

[Vladimir Oltean]

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
>

RE: [PATCH net] net: marvell: prestera: fix brige port operation

[Volodymyr Mytnyk]

> 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