Re: [PATCH net] net/mlx5e: Skip IPsec flow modify when MAC address is unchanged

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net/mlx5e: Skip IPsec flow modify when MAC address is unchanged

When a netdev event fires for an IPsec SA, skip calling
mlx5e_accel_ipsec_fs_modify() if the MAC address has not changed and
the rule is not in drop mode. This avoids redundant hardware updates.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index a52e12c3c95a2..f567cd801adb3 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -674,19 +674,26 @@ static void mlx5e_ipsec_handle_netdev_event(struct work_struct *_work)
>  	struct mlx5e_ipsec_sa_entry *sa_entry = work->sa_entry;
>  	struct mlx5e_ipsec_netevent_data *data = work->data;
>  	struct mlx5_accel_esp_xfrm_attrs *attrs;
[ ... ]
> +
> +	if (ether_addr_equal(mac, data->addr) && !attrs->drop)
> +		return;
> +
> +	ether_addr_copy(mac, data->addr);
>  	attrs->drop = false;
>  	mlx5e_accel_ipsec_fs_modify(sa_entry);

If mlx5e_accel_ipsec_fs_modify() fails (for example, due to a memory
allocation failure), does this early return prevent the hardware state
from recovering?

Since attrs->drop and the mac address are updated before calling
mlx5e_accel_ipsec_fs_modify(), a failure in the hardware update leaves the
software state out of sync with the hardware.

Prior to this patch, a subsequent netdev event for the same neighbor would
unconditionally retry the hardware update. With this patch, the early return
sees the optimistically updated software state, matches it, and skips the
retry, potentially leaving the hardware permanently stuck in a dropped or
failed state.

Also, can sa_entry->work and sa_entry->dwork execute concurrently?

Both sa_entry->work (which executes mlx5e_ipsec_handle_netdev_event()) and
sa_entry->dwork (which executes mlx5e_ipsec_handle_sw_limits()) are queued
to ipsec->wq. Since this is an unbound workqueue with max_active=0, works
can execute concurrently.

Neither handler appears to hold a lock when calling
mlx5e_accel_ipsec_fs_modify(sa_entry). If both handlers run concurrently,
could this happen:

Thread 1:
mlx5e_accel_ipsec_fs_modify(sa_entry)
  memcpy(&sa_entry_shadow, sa_entry, sizeof(*sa_entry));
  mlx5e_accel_ipsec_fs_add_rule(&sa_entry_shadow);

Thread 2:
mlx5e_accel_ipsec_fs_modify(sa_entry)
  memcpy(&sa_entry_shadow, sa_entry, sizeof(*sa_entry));
  mlx5e_accel_ipsec_fs_add_rule(&sa_entry_shadow);

Both threads would then call mlx5e_accel_ipsec_fs_del_rule(sa_entry),
potentially causing a double-free of the exact same ipsec_rule->rule and
ipsec_rule->fc. The subsequent memcpy(sa_entry, &sa_entry_shadow, ...)
would then cause one thread to overwrite the other's newly created rule
pointers, leaking the flow rules.

>  }