Re: [PATCH V2] sbitmap: order READ/WRITE freed instance and setting clear bit

[Bart Van Assche <bvanassche@acm.org>]

On Tue, 2019-03-19 at 16:38 +0800, Ming Lei wrote:
> Inside sbitmap_queue_clear(), once the clear bit is set, it will be
> visiable to allocation path immediately. Meantime READ/WRITE on old
> associated instance(such as request in case of blk-mq) may be
> out-of-order with the setting clear bit, so race with re-allocation
> may be triggered.
> 
> Adds one memory barrier for ordering READ/WRITE of the freed associated
> instance with setting clear bit for avoiding race with re-allocation.
> 
> The following kernel oops triggerd by block/006 on aarch64 may be fixed:
                                                             ^^^

Does that mean that it has not been verified whether this patch fixes the
NULL pointer issue mentioned in the patch description?

> diff --git a/lib/sbitmap.c b/lib/sbitmap.c
> index 5b382c1244ed..941a46495e12 100644
> --- a/lib/sbitmap.c
> +++ b/lib/sbitmap.c
> @@ -591,6 +591,17 @@ EXPORT_SYMBOL_GPL(sbitmap_queue_wake_up);
>  void sbitmap_queue_clear(struct sbitmap_queue *sbq, unsigned int nr,
>                          unsigned int cpu)
>  {
> +       /*
> +        * Once the clear bit is set, the bit may be allocated out.
             ^^^^^^^^^^^^^^^^^^^^^^^^^

This comment is confusing. Did you perhaps mean "Once the bit is cleared"?

> +        *
> +        * Orders READ/WRITE on the asssociated instance(such as request
> +        * of blk_mq) by this bit for avoiding race with re-allocation,
> +        * and its pair is the memory barrier implied in __sbitmap_get_word.
> +        *
> +        * One invarient is that the clear bit has to be zero when the bit
                 ^^^^^^^^^             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                 invariant?            I cant' make sense of this.
> +        * is in use.
> +        */
> +       smp_mb__before_atomic();
>         sbitmap_deferred_clear_bit(&sbq->sb, nr);

Thanks,

Bart.