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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Nov 22, 2023 at 12:28:32PM CET, leon@kernel.org wrote:
>On Wed, Nov 22, 2023 at 10:50:37AM +0100, Jiri Pirko wrote:
>> Wed, Nov 22, 2023 at 10:35:46AM CET, leon@kernel.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@kernel.org wrote:
>> >> >From: Jianbo Liu <jianbol@nvidia.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@nvidia.com>
>> >> >Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>> >> >Signed-off-by: Saeed Mahameed <saeedm@nvidia.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:

That is clearly a bug that should be fixed. That will solve the issue.


>
>  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