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

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

[Tariq Toukan]

From: Jianbo Liu <jianbol@nvidia.com>

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 a redundant hardware update
when nothing has changed, while still allowing the rule to be restored
when the neighbor becomes reachable again after a drop.

Fixes: 4c24272b4e2b ("net/mlx5e: Listen to ARP events to update IPsec L2 headers in tunnel mode")
Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c  | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

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 a52e12c3c95a..f567cd801adb 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;
+	u8 *mac;
 
 	attrs = &sa_entry->attrs;
 
 	switch (attrs->dir) {
 	case XFRM_DEV_OFFLOAD_IN:
-		ether_addr_copy(attrs->smac, data->addr);
+		mac = attrs->smac;
 		break;
 	case XFRM_DEV_OFFLOAD_OUT:
-		ether_addr_copy(attrs->dmac, data->addr);
+		mac = attrs->dmac;
 		break;
 	default:
 		WARN_ON_ONCE(true);
+		return;
 	}
+
+	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);
 }

base-commit: f5b2772d14884f4be9e718644f1203d4d0e6f0d6
-- 
2.44.0

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

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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.

>  }

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

[Paolo Abeni]

On 5/18/26 1:27 PM, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> 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.

@Tariq, @Jianbo: I tend to think that the above qualifies as
pre-existing issue: when mlx5e_accel_ipsec_fs_modify() failed the S/W
and the H/W ended-up in out-of-sync state for a potentially unlimited
time even before this patch. WDYT?

/P