Hello Qiu-ji Chen,
The code change looks okay to me.
Reviewed-by: Philipp Reisner <philipp.reisner@xxxxxxxxxx>
On Fri, Sep 13, 2024 at 10:35 AM Qiu-ji Chen <chenqiuji666@xxxxxxxxx> wrote:
>
> The violation of atomicity occurs when the drbd_uuid_set_bm function is
> executed simultaneously with modifying the value of
> device->ldev->md.uuid[UI_BITMAP]. Consider a scenario where, while
> device->ldev->md.uuid[UI_BITMAP] passes the validity check when its value
> is not zero, the value of device->ldev->md.uuid[UI_BITMAP] is written to
> zero. In this case, the check in drbd_uuid_set_bm might refer to the old
> value of device->ldev->md.uuid[UI_BITMAP] (before locking), which allows
> an invalid value to pass the validity check, resulting in inconsistency.
>
> To address this issue, it is recommended to include the data validity check
> within the locked section of the function. This modification ensures that
> the value of device->ldev->md.uuid[UI_BITMAP] does not change during the
> validation process, thereby maintaining its integrity.
>
> This possible bug is found by an experimental static analysis tool
> developed by our team. This tool analyzes the locking APIs to extract
> function pairs that can be concurrently executed, and then analyzes the
> instructions in the paired functions to identify possible concurrency bugs
> including data races and atomicity violations.
>
> Fixes: 9f2247bb9b75 ("drbd: Protect accesses to the uuid set with a spinlock")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Qiu-ji Chen <chenqiuji666@xxxxxxxxx>
> ---
> drivers/block/drbd/drbd_main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index a9e49b212341..abafc4edf9ed 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -3399,10 +3399,12 @@ void drbd_uuid_new_current(struct drbd_device *device) __must_hold(local)
> void drbd_uuid_set_bm(struct drbd_device *device, u64 val) __must_hold(local)
> {
> unsigned long flags;
> - if (device->ldev->md.uuid[UI_BITMAP] == 0 && val == 0)
> + spin_lock_irqsave(&device->ldev->md.uuid_lock, flags);
> + if (device->ldev->md.uuid[UI_BITMAP] == 0 && val == 0) {
> + spin_unlock_irqrestore(&device->ldev->md.uuid_lock, flags);
> return;
> + }
>
> - spin_lock_irqsave(&device->ldev->md.uuid_lock, flags);
> if (val == 0) {
> drbd_uuid_move_history(device);
> device->ldev->md.uuid[UI_HISTORY_START] = device->ldev->md.uuid[UI_BITMAP];
> --
> 2.34.1
>
