[PATCH net] net/mlx5: HV VHCA: Fix the agent registration race

[PATCH net] net/mlx5: HV VHCA: Fix the agent registration race

[Ginger Li]

mlx5_hv_vhca_agent_create() drops agents_lock between checking whether an
agent is already registered for the type and publishing the new agent, so two
concurrent create calls for the same type can both pass the check.  The second
one then silently replaces the first one in hv_vhca->agents[], and the agent
allocated by the first call is leaked.

Hold agents_lock for the whole registration.  While at it, read
hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL] under the lock in
mlx5_hv_vhca_cleanup() as well, since that slot is written under the lock.

Fixes: 87175120defd ("net/mlx5: Add HV VHCA infrastructure")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
@@ -228,7 +228,10 @@ void mlx5_hv_vhca_cleanup(struct mlx5_hv_vhca *hv_vhca
 	if (!hv_vhca)
 		return;
 
+	mutex_lock(&hv_vhca->agents_lock);
 	agent = hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL];
+	mutex_unlock(&hv_vhca->agents_lock);
+
 	if (agent)
 		mlx5_hv_vhca_control_agent_destroy(agent);
 
@@ -270,11 +273,12 @@ mlx5_hv_vhca_agent_create(struct mlx5_hv_vhca *hv_vhca
 		mutex_unlock(&hv_vhca->agents_lock);
 		return ERR_PTR(-EINVAL);
 	}
-	mutex_unlock(&hv_vhca->agents_lock);
 
 	agent = kzalloc_obj(*agent);
-	if (!agent)
+	if (!agent) {
+		mutex_unlock(&hv_vhca->agents_lock);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	agent->type      = type;
 	agent->hv_vhca   = hv_vhca;
@@ -286,7 +290,6 @@ mlx5_hv_vhca_agent_create(struct mlx5_hv_vhca *hv_vhca
 	if (ctx_update)
 		WRITE_ONCE(*ctx_update, agent);
 
-	mutex_lock(&hv_vhca->agents_lock);
 	hv_vhca->agents[type] = agent;
 	mutex_unlock(&hv_vhca->agents_lock);
 
-- 
2.43.0

Re: [PATCH net] net/mlx5: HV VHCA: Fix the agent registration race

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch's second hunk does not achieve its stated purpose
  and leaves the lock coverage of…

--

net/mlx5: HV VHCA: Fix the agent registration race

This keeps agents_lock held across the duplicate-type check, the agent
allocation and the publish into hv_vhca->agents[] in
mlx5_hv_vhca_agent_create(), so two concurrent create calls for the same
type can no longer both pass the check.  It also wraps the read of
hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL] in mlx5_hv_vhca_cleanup()
with agents_lock.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
> index 4c4cf6da519fa..eac8afdfc67a3 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
> @@ -228,7 +228,10 @@ void mlx5_hv_vhca_cleanup(struct mlx5_hv_vhca *hv_vhca)
>  	if (!hv_vhca)
>  		return;
>  
> +	mutex_lock(&hv_vhca->agents_lock);
>  	agent = hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL];
> +	mutex_unlock(&hv_vhca->agents_lock);
> +
>  	if (agent)
>  		mlx5_hv_vhca_control_agent_destroy(agent);

[Severity: Medium]
What does this new critical section in mlx5_hv_vhca_cleanup() actually
protect?  The lock is dropped immediately after the load, and the pointer
is consumed afterwards:

	mutex_lock(&hv_vhca->agents_lock);
	agent = hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL];
	mutex_unlock(&hv_vhca->agents_lock);

	if (agent)
		mlx5_hv_vhca_control_agent_destroy(agent);

So the only guarantee gained over the previous code is a non-torn load of
an aligned pointer, which a plain load already provides.  Nothing is
serialized against the destroy itself.  Extending the lock over the call
is not an option either, since mlx5_hv_vhca_agent_destroy() takes
agents_lock itself and that would self-deadlock.

The changelog says:

    While at it, read hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL] under
    the lock in mlx5_hv_vhca_cleanup() as well, since that slot is
    written under the lock.

Is that accurate?  mlx5_hv_vhca_init() still stores to the same slot with
no lock held:

	agent = mlx5_hv_vhca_control_agent_create(hv_vhca);
	if (IS_ERR_OR_NULL(agent)) {
		mlx5_hv_unregister_invalidate(hv_vhca->dev);
		return;
	}

	hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL] = agent;

That store is now fully redundant with the locked publish that
mlx5_hv_vhca_agent_create() already performs:

	hv_vhca->agents[type] = agent;
	mutex_unlock(&hv_vhca->agents_lock);

and it contradicts the field's own contract:

	struct mutex                agents_lock; /* Protect agents array */

Can it also race with the locked reader?  mlx5_hv_register_invalidate()
has already run by that point, and mlx5_hv_vhca_agent_create() queues
work before returning to init:

mlx5_hv_vhca_agent_create()
  mlx5_hv_vhca_agents_update()
    mlx5_hv_vhca_invalidate()
      queue_work(hv_vhca->work_queue, &work->invalidate_work);

while the worker reads the array under the mutex:

	mutex_lock(&hv_vhca->agents_lock);
	for (i = 0; i < MLX5_HV_VHCA_AGENT_MAX; i++) {
		struct mlx5_hv_vhca_agent *agent = hv_vhca->agents[i];

Since the value stored by init() is identical to the one already
published under the lock, the practical effect today looks like a benign
data race that KCSAN would flag rather than a functional failure.  Would
it be cleaner to drop the unlocked assignment in mlx5_hv_vhca_init()
instead, so agents[] has exactly one locked writer per slot, drop the
no-op lock/unlock in mlx5_hv_vhca_cleanup(), and adjust the changelog
wording accordingly?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922074246.20411-1-ginger.jzllee%40gmail.com