[PATCH] aoe: consolidate flags update to prevent race condition

[PATCH] aoe: consolidate flags update to prevent race condition

[Gui-Dong Han]

In aoecmd_sleepwork, there is a race condition caused by two consecutive
writes to the 'flags' variable within a critical section. If a read 
operation occurs between these writes, an intermediate state may be 
read, potentially causing bugs.

To address this issue, the 'flags' variable should be updated in a 
single operation to ensure atomicity and prevent any intermediate state
from being read.

Fixes: 3ae1c24e395b ("[PATCH] aoe [2/8]: support dynamic resizing of AoE devices")
Signed-off-by: Gui-Dong Han <hanguidong02@outlook.com>
---
 drivers/block/aoe/aoecmd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index cc9077b588d7..37d556f019c0 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -897,8 +897,7 @@ aoecmd_sleepwork(struct work_struct *work)
 		set_capacity_and_notify(d->gd, d->ssize);
 
 		spin_lock_irq(&d->lock);
-		d->flags |= DEVFL_UP;
-		d->flags &= ~DEVFL_NEWSIZE;
+		d->flags = (d->flags | DEVFL_UP) & ~DEVFL_NEWSIZE;
 		spin_unlock_irq(&d->lock);
 	}
 }
-- 
2.34.1

Re: [PATCH] aoe: consolidate flags update to prevent race condition

[Jens Axboe]

On 6/10/24 9:52 PM, Gui-Dong Han wrote:
> In aoecmd_sleepwork, there is a race condition caused by two consecutive
> writes to the 'flags' variable within a critical section. If a read 
> operation occurs between these writes, an intermediate state may be 
> read, potentially causing bugs.
> 
> To address this issue, the 'flags' variable should be updated in a 
> single operation to ensure atomicity and prevent any intermediate state
> from being read.
> 
> Fixes: 3ae1c24e395b ("[PATCH] aoe [2/8]: support dynamic resizing of AoE devices")
> Signed-off-by: Gui-Dong Han <hanguidong02@outlook.com>
> ---
>  drivers/block/aoe/aoecmd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
> index cc9077b588d7..37d556f019c0 100644
> --- a/drivers/block/aoe/aoecmd.c
> +++ b/drivers/block/aoe/aoecmd.c
> @@ -897,8 +897,7 @@ aoecmd_sleepwork(struct work_struct *work)
>  		set_capacity_and_notify(d->gd, d->ssize);
>  
>  		spin_lock_irq(&d->lock);
> -		d->flags |= DEVFL_UP;
> -		d->flags &= ~DEVFL_NEWSIZE;
> +		d->flags = (d->flags | DEVFL_UP) & ~DEVFL_NEWSIZE;
>  		spin_unlock_irq(&d->lock);
>  	}

It's modified under the lock, and any reader should do so as well. If
not, there's a race regardless of your change or not.

-- 
Jens Axboe

Re: [PATCH] aoe: consolidate flags update to prevent race condition

[Gui-Dong Han]

Hi Jens,

Thank you for your feedback. Let me clarify the rationale behind this change.

On 2024-06-12 10:54, Jens Axboe wrote:
> It's modified under the lock, and any reader should do so as well. If
> not, there's a race regardless of your change or not.

The existing implementation contains multiple unsynchronized reads of d->flags.
For example, in aoecmd_sleepwork() itself:

void aoecmd_sleepwork(struct work_struct *work)
{
    struct aoedev *d = container_of(work, struct aoedev, work);

    /* These flag checks are performed WITHOUT LOCKING: */
    if (d->flags & DEVFL_GDALLOC) <--- Unsynchronized read
        aoeblk_gdalloc(d);
    if (d->flags & DEVFL_NEWSIZE) {  <--- Unsynchronized read
        set_capacity_and_notify(d->gd, d->ssize);
        spin_lock_irq(&d->lock);
        d->flags |= DEVFL_UP;        <--- Two separate writes
        d->flags &= ~DEVFL_NEWSIZE;  <--- to flags under lock
        spin_unlock_irq(&d->lock);
    }
}

The problematic sequence would be:
1. Thread A sets DEVFL_UP under lock
2. Thread B (or same thread) reads d->flags without lock, sees DEVFL_UP set but
   DEVFL_NEWSIZE still present (before the second write clears it)
3. This could lead to unexpected behavior since DEVFL_NEWSIZE indicates a state
   transition that should be atomic with DEVFL_UP setting

By consolidating the flag updates into a single atomic operation under lock:

d->flags = (d->flags | DEVFL_UP) & ~DEVFL_NEWSIZE;

We ensure any unsynchronized reader (like the DEVFL_NEWSIZE check in this same
function) will either see both changes or neither, avoiding intermediate states.

Would you agree this provides stronger consistency guarantees for readers that
(per current code) don't take the lock when checking flags?

Best regards,
Gui-Dong Han