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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <SY8P300MB046047D3CF98899A011F9AB5C0C32@SY8P300MB0460.AUSP300.PROD.OUTLOOK.COM>
Date: Tue, 25 Feb 2025 03:13:29 +0000
From: Gui-Dong Han <hanguidong02@...look.com>
To: Jens Axboe <axboe@...nel.dk>, "justin@...aid.com" <justin@...aid.com>
CC: "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"baijiaju1990@...il.com" <baijiaju1990@...il.com>
Subject: Re: [PATCH] aoe: consolidate flags update to prevent race condition

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