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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux