Re: [PATCH net-next V2 2/4] net/mlx5: Implement devlink port function cmds to control ipsec_crypto

[Simon Horman <simon.horman@corigine.com>]

On Fri, Apr 21, 2023 at 01:48:59PM +0300, Dima Chumak wrote:
> Implement devlink port function commands to enable / disable IPsec
> crypto offloads.  This is used to control the IPsec capability of the
> device.
> 
> When ipsec_crypto is enabled for a VF, it prevents adding IPsec crypto
> offloads on the PF, because the two cannot be active simultaneously due
> to HW constraints. Conversely, if there are any active IPsec crypto
> offloads on the PF, it's not allowed to enable ipsec_crypto on a VF,
> until PF IPsec offloads are cleared.
> 
> Signed-off-by: Dima Chumak <dchumak@nvidia.com>

Hi Dima,

I noticed a few issues in error handling, mostly flagged by smatch.

> @@ -622,6 +624,7 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x,
>  	struct mlx5e_ipsec_sa_entry *sa_entry = NULL;
>  	struct net_device *netdev = x->xso.real_dev;
>  	struct mlx5e_ipsec *ipsec;
> +	struct mlx5_eswitch *esw;
>  	struct mlx5e_priv *priv;
>  	gfp_t gfp;
>  	int err;
> @@ -646,6 +649,11 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x,
>  	if (err)
>  		goto err_xfrm;

goto err_xfrm will now result in a call to
mlx5_eswitch_ipsec_offloads_count_dec().
But mlx5_eswitch_ipsec_offloads_count_inc is not called
until a few lines below.
This seems inconsistent to me.


>  
> +	esw = priv->mdev->priv.eswitch;
> +	if (esw && mlx5_esw_vport_ipsec_offload_enabled(esw))
> +		return -EBUSY;

I think a goto is needed here in order to unwind correctly.

> +	mlx5_eswitch_ipsec_offloads_count_inc(priv->mdev);
> +
>  	/* check esn */
>  	if (x->props.flags & XFRM_STATE_ESN)
>  		mlx5e_ipsec_update_esn_state(sa_entry);
> @@ -711,6 +719,7 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x,
>  	kfree(sa_entry->work->data);
>  	kfree(sa_entry->work);
>  err_xfrm:
> +	mlx5_eswitch_ipsec_offloads_count_dec(priv->mdev);
>  	kfree(sa_entry);
>  	NL_SET_ERR_MSG_MOD(extack, "Device failed to offload this policy");
>  	return err;
> @@ -734,6 +743,7 @@ static void mlx5e_xfrm_del_state(struct xfrm_state *x)
>  		/* Make sure that no ARP requests are running in parallel */
>  		flush_workqueue(ipsec->wq);
>  
> +	mlx5_eswitch_ipsec_offloads_count_dec(ipsec->mdev);
>  }
>  
>  static void mlx5e_xfrm_free_state(struct xfrm_state *x)
> @@ -1007,6 +1017,7 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  {
>  	struct net_device *netdev = x->xdo.real_dev;
>  	struct mlx5e_ipsec_pol_entry *pol_entry;
> +	struct mlx5_eswitch *esw;
>  	struct mlx5e_priv *priv;
>  	int err;
>  
> @@ -1027,6 +1038,11 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  	pol_entry->x = x;
>  	pol_entry->ipsec = priv->ipsec;
>  
> +	esw = priv->mdev->priv.eswitch;
> +	if (esw && mlx5_esw_vport_ipsec_offload_enabled(esw))
> +		return -EBUSY;

I think this leaks pol_entry.

> +	mlx5_eswitch_ipsec_offloads_count_inc(priv->mdev);
> +
>  	mlx5e_ipsec_build_accel_pol_attrs(pol_entry, &pol_entry->attrs);
>  	err = mlx5e_accel_ipsec_fs_add_pol(pol_entry);
>  	if (err)

...