[bug report] Potential refcounting

[bug report] Potential refcounting

[Ginger]

Dear Linux kernel maintainers,

My research-based static analyzer found a potential
refcounting/atomicity bug within the
'drivers/net/ethernet/mellanox/mlx4' subsystem, more specifically, in
'drivers/net/ethernet/mellanox/mlx4/cq.c'.

Kernel version: long-term kernel v6.18.9

Potential concurrent triggering executions:
T0:
mlx4_cq_tasklet_cb
     --> if (refcount_dec_and_test(&mcq->refcount))
     --> complete(&mcq->free)

T1:
mlx4_cq_completion
    --> cq->comp(cq);
        --> mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
            --> spin_lock_irqsave(&tasklet_ctx->lock, flags);
            --> refcount_inc(&cq->refcount);
            --> spin_unlock_irqrestore(&tasklet_ctx->lock, flags);

In T1, the refcounting increment on 'cq->refcount)', although within
the protection range of the 'tasklet_ctx->locl', is not synchronized
against T0 because 'refcount_inc()' does not check whether the
refcount has reached zero in T0. This case is potentially problematic
because T0 decrements he 'mcq->refcount' and can enable the
'mlx4_cq_free()' to proceed.

Thank you for your time and consideration.

Best regards,
Ginger

Re: [bug report] Potential refcounting

[Tariq Toukan]

On 27/04/2026 5:07, Ginger wrote:
> Dear Linux kernel maintainers,
> 
> My research-based static analyzer found a potential
> refcounting/atomicity bug within the
> 'drivers/net/ethernet/mellanox/mlx4' subsystem, more specifically, in
> 'drivers/net/ethernet/mellanox/mlx4/cq.c'.
> 
> Kernel version: long-term kernel v6.18.9
> 
> Potential concurrent triggering executions:
> T0:
> mlx4_cq_tasklet_cb
>       --> if (refcount_dec_and_test(&mcq->refcount))
>       --> complete(&mcq->free)
> 
> T1:
> mlx4_cq_completion
>      --> cq->comp(cq);
>          --> mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
>              --> spin_lock_irqsave(&tasklet_ctx->lock, flags);
>              --> refcount_inc(&cq->refcount);
>              --> spin_unlock_irqrestore(&tasklet_ctx->lock, flags);
> 
> In T1, the refcounting increment on 'cq->refcount)', although within
> the protection range of the 'tasklet_ctx->locl', is not synchronized
> against T0 because 'refcount_inc()' does not check whether the
> refcount has reached zero in T0. This case is potentially problematic
> because T0 decrements he 'mcq->refcount' and can enable the
> 'mlx4_cq_free()' to proceed.
> 
> Thank you for your time and consideration.
> 
> Best regards,
> Ginger
> 

Hi,

Thanks for your report.

IMO the described race is impossible.

CQs that work with mlx4_add_cq_to_tasklet as their comp() callback (i.e. 
T1) are added to the relevant list only after refcount is incremented.

Hence, if a CQ exists in the list in T0, it necessarily means that 
refcount is already elevated, and calling refcount_dec_and_test is safe.

Regards,
Tariq

Re: [bug report] Potential refcounting

[Ginger]

Hi Tariq,

Thank you for your prompt and detailed response. I agree with your
point that calling refcount_dec_and_test() is safe.
However, the major concern of the original report is that whether
simply calling 'refcount_inc()' is safe in 'mlx4_add_cq_to_tasklet()':

T0:
if during a tasklet callback calling 'mlx4_cq_tasklet_cb', a destroy
verb arrives, which eventually leads to 'mlx4_cq_free()'. Both
functions can decrement the 'cq->refcount' and potentially enable
cq->free.

T1:
irq fires --> mlx4_cq_completion --> mlx4_add_cq_to_tasklet -->
refcount_inc(). Here, the refcount increment does not check whether it
has been zeroed in T0.

Side note: to see if T1 (i.e., IRQ) can fire, I checked the calling
path to 'mlx4_cq_free' and only 'synchronize_irq()' is seen, but not
'disable_irq()'. It means that T1 may still happen after T0 calls
'mlx4_cq_free()'.

Is the above race possible? IMHO, perhaps it would better to change
'refcount_inc()' to something like 'refcount_inc_not_zero()' if the
race may happen.


Best regards,
Ginger

On Wed, Apr 29, 2026 at 4:36 PM Tariq Toukan <tariqt@nvidia.com> wrote:
>
>
>
> On 27/04/2026 5:07, Ginger wrote:
> > Dear Linux kernel maintainers,
> >
> > My research-based static analyzer found a potential
> > refcounting/atomicity bug within the
> > 'drivers/net/ethernet/mellanox/mlx4' subsystem, more specifically, in
> > 'drivers/net/ethernet/mellanox/mlx4/cq.c'.
> >
> > Kernel version: long-term kernel v6.18.9
> >
> > Potential concurrent triggering executions:
> > T0:
> > mlx4_cq_tasklet_cb
> >       --> if (refcount_dec_and_test(&mcq->refcount))
> >       --> complete(&mcq->free)
> >
> > T1:
> > mlx4_cq_completion
> >      --> cq->comp(cq);
> >          --> mlx4_add_cq_to_tasklet(struct mlx4_cq *cq)
> >              --> spin_lock_irqsave(&tasklet_ctx->lock, flags);
> >              --> refcount_inc(&cq->refcount);
> >              --> spin_unlock_irqrestore(&tasklet_ctx->lock, flags);
> >
> > In T1, the refcounting increment on 'cq->refcount)', although within
> > the protection range of the 'tasklet_ctx->locl', is not synchronized
> > against T0 because 'refcount_inc()' does not check whether the
> > refcount has reached zero in T0. This case is potentially problematic
> > because T0 decrements he 'mcq->refcount' and can enable the
> > 'mlx4_cq_free()' to proceed.
> >
> > Thank you for your time and consideration.
> >
> > Best regards,
> > Ginger
> >
>
> Hi,
>
> Thanks for your report.
>
> IMO the described race is impossible.
>
> CQs that work with mlx4_add_cq_to_tasklet as their comp() callback (i.e.
> T1) are added to the relevant list only after refcount is incremented.
>
> Hence, if a CQ exists in the list in T0, it necessarily means that
> refcount is already elevated, and calling refcount_dec_and_test is safe.
>
> Regards,
> Tariq

Re: [bug report] Potential refcounting

[Jason Gunthorpe]

On Wed, Apr 29, 2026 at 06:13:40PM +0800, Ginger wrote:
> Side note: to see if T1 (i.e., IRQ) can fire, I checked the calling
> path to 'mlx4_cq_free' and only 'synchronize_irq()' is seen, but not
> 'disable_irq()'. It means that T1 may still happen after T0 calls
> 'mlx4_cq_free()'.

synchronize_irq() is supposed to wait for any concurrently running IRQ
threads to finish. New IRQ threads will then be guarenteed to see the
radix_tree_delete() and can't see the CQ being removed here.

Since the refcount is != 0 until after synchronize_irq(), and the irq
can't see the cq after synchronize_irq() there should be no issue.

Jason