netdev - Re: [net 09/15] net/mlx5e: Forbid devlink reload if IPSec rules are offloaded

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231122112832.GB4760@unreal>
Date: Wed, 22 Nov 2023 13:28:32 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Saeed Mahameed <saeed@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Saeed Mahameed <saeedm@...dia.com>, netdev@...r.kernel.org,
	Tariq Toukan <tariqt@...dia.com>, Jianbo Liu <jianbol@...dia.com>
Subject: Re: [net 09/15] net/mlx5e: Forbid devlink reload if IPSec rules are
 offloaded

On Wed, Nov 22, 2023 at 10:50:37AM +0100, Jiri Pirko wrote:
> Wed, Nov 22, 2023 at 10:35:46AM CET, leon@...nel.org wrote:
> >On Wed, Nov 22, 2023 at 10:13:45AM +0100, Jiri Pirko wrote:
> >> Wed, Nov 22, 2023 at 02:47:58AM CET, saeed@...nel.org wrote:
> >> >From: Jianbo Liu <jianbol@...dia.com>
> >> >
> >> >When devlink reload, mlx5 IPSec module can't be safely cleaned up if
> >> >there is any IPSec rule offloaded, so forbid it in this condition.
> >> >
> >> >Fixes: edd8b295f9e2 ("Merge branch 'mlx5-ipsec-packet-offload-support-in-eswitch-mode'")
> >> >Signed-off-by: Jianbo Liu <jianbol@...dia.com>
> >> >Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> >> >Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> >> >---
> >> > drivers/net/ethernet/mellanox/mlx5/core/devlink.c |  5 +++++
> >> > drivers/net/ethernet/mellanox/mlx5/core/eswitch.h |  2 ++
> >> > .../mellanox/mlx5/core/eswitch_offloads.c         | 15 +++++++++++++++
> >> > 3 files changed, 22 insertions(+)
> >> >
> >> >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> >> >index 3e064234f6fe..8925e87a3ed5 100644
> >> >--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> >> >+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> >> >@@ -157,6 +157,11 @@ static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change,
> >> > 		return -EOPNOTSUPP;
> >> > 	}
> >> > 
> >> >+	if (mlx5_eswitch_mode_is_blocked(dev)) {
> >> >+		NL_SET_ERR_MSG_MOD(extack, "reload is unsupported if IPSec rules are configured");
> >> 
> >> That sounds a bit odd to me to be honest. Is pci device unbind forbidden
> >> if ipsec rules are present too? This should be gracefully handled
> >> instead of forbid.
> >
> >unbind is handled differently because that operation will call to
> >unregister netdevice event which will clean everything.
> 
> But in reload, the netdevice is also unregistered. Same flow, isn't it?

Unfortunately not, we (mlx5) were forced by employer of one of
the netdev maintainers to keep uplink netdev in devlink reload
while we are in eswitch. It is skipped in lines 1556-1558:

  1548 static void
  1549 mlx5e_vport_rep_unload(struct mlx5_eswitch_rep *rep)
  1550 {
  1551         struct mlx5e_rep_priv *rpriv = mlx5e_rep_to_rep_priv(rep);
  1552         struct net_device *netdev = rpriv->netdev;
  1553         struct mlx5e_priv *priv = netdev_priv(netdev);
  1554         void *ppriv = priv->ppriv;
  1555
  1556         if (rep->vport == MLX5_VPORT_UPLINK) {
  1557                 mlx5e_vport_uplink_rep_unload(rpriv);
  1558                 goto free_ppriv;
  1559         }
  1560
  1561         unregister_netdev(netdev);
  1562         mlx5e_rep_vnic_reporter_destroy(priv);
  1563         mlx5e_detach_netdev(priv);
  1564         priv->profile->cleanup(priv);
  1565         mlx5e_destroy_netdev(priv);
  1566 free_ppriv:
  1567         kvfree(ppriv); /* mlx5e_rep_priv */
  1568 }

> 
> >
> >devlink reload behaves differently from unbind.
> 
> I don't see why. Forget about the driver implementation for now. From
> the perspective of the user, what's the difference between these flows:
> 1) unbind->netdevremoval

netdevice can be removed and there is no way to inform users about errors.

> 2) reload->netdevremoval

According to that employer, netdevice should stay.

> 
> Both should be working and do necessary cleanups.

I would be more than happy to see same flow, but this is above my
pay grade and I have little desire to be in the middle between
that netdev maintainer and his management.

Feel free to approach me offline, and I will give you the names.

Thanks

> 
> 
> >
> >Thanks