| 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
| ||
|
Message-ID: <CAHrEdeuuj_WQQ00i344TqgwoxLJJv--3sCEx5rFrseXtLjog4w@mail.gmail.com>
Date: Wed, 31 Dec 2025 19:02:30 +0800
From: wale zhang <wale.zhang.ftd@...il.com>
To: Coly Li <colyli@...as.com>
Cc: kent.overstreet@...ux.dev, linux-bcache@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] bcache: make bcache_is_reboot atomic.
On Tue, Dec 30, 2025 at 8:22 PM Coly Li <colyli@...as.com> wrote:
>
> On Tue, Dec 30, 2025 at 07:33:57PM +0800, Wale Zhang wrote:
> > bcache: make bcache_is_reboot atomic.
> >
> > The smp_mb is mainly used to ensure the dependency relationship between
> > variables, but there is only one variable bcache_is_reboot. Therefore,
> > using smp_mb is not very appropriate.
> >
> > When bcache_reboot and register_bcache occur concurrently, register_bcache
> > cannot immediately detect that bcache_is_reboot has been set to true.
> >
> > cpu0 cpu1
> > bcache_reboot
> > bcache_is_reboot = true;
> > smp_mb(); register_bcache
> > smp_mb();
> > if (bcache_is_reboot)
> > // bcache_is_reboot may still be false.
> >
>
> From the above comments, I don't see what problem this patch tries to solve.
> And read or write bcache_is_reboot is atomic indeed, I don't see the difference
> this patch makes.
Hello Coly,
I'm puzzled by the use of smp_mb in this bcache_is_reboot scenario.
I know that smp_mb is typically used for the scenarios where data
dependency is involved, such as
---------------------------------
CPU0 CPU1
x=1 y=1
smp_mb smb_mb
load y load x
---------------------------------
If CPU1 observe x==1, then CPU0 must also observe y==1.
Thank you for clearing up my puzzlement.
Thanks
Wale
> Thanks.
>
> Coly Li
>
> > Signed-off-by: Wale Zhang <wale.zhang.ftd@...il.com>
> > ---
> > drivers/md/bcache/super.c | 19 ++++++-------------
> > drivers/md/bcache/sysfs.c | 14 +++++++-------
> > 2 files changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index c17d4517af22..0b2098aa7234 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -41,7 +41,7 @@ static const char invalid_uuid[] = {
> >
> > static struct kobject *bcache_kobj;
> > struct mutex bch_register_lock;
> > -bool bcache_is_reboot;
> > +atomic_t bcache_is_reboot = ATOMIC_INIT(0);
> > LIST_HEAD(bch_cache_sets);
> > static LIST_HEAD(uncached_devices);
> >
> > @@ -2561,10 +2561,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> > if (!try_module_get(THIS_MODULE))
> > goto out;
> >
> > - /* For latest state of bcache_is_reboot */
> > - smp_mb();
> > err = "bcache is in reboot";
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > goto out_module_put;
> >
> > ret = -ENOMEM;
> > @@ -2735,7 +2733,7 @@ static ssize_t bch_pending_bdevs_cleanup(struct kobject *k,
> >
> > static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
> > {
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return NOTIFY_DONE;
> >
> > if (code == SYS_DOWN ||
> > @@ -2750,16 +2748,11 @@ static int bcache_reboot(struct notifier_block *n, unsigned long code, void *x)
> >
> > mutex_lock(&bch_register_lock);
> >
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > goto out;
> >
> > /* New registration is rejected since now */
> > - bcache_is_reboot = true;
> > - /*
> > - * Make registering caller (if there is) on other CPU
> > - * core know bcache_is_reboot set to true earlier
> > - */
> > - smp_mb();
> > + atomic_set(&bcache_is_reboot, 1);
> >
> > if (list_empty(&bch_cache_sets) &&
> > list_empty(&uncached_devices))
> > @@ -2935,7 +2928,7 @@ static int __init bcache_init(void)
> >
> > bch_debug_init();
> >
> > - bcache_is_reboot = false;
> > + atomic_set(&bcache_is_reboot, 0);
> >
> > return 0;
> > err:
> > diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> > index 72f38e5b6f5c..5384653c5bbb 100644
> > --- a/drivers/md/bcache/sysfs.c
> > +++ b/drivers/md/bcache/sysfs.c
> > @@ -17,7 +17,7 @@
> > #include <linux/sort.h>
> > #include <linux/sched/clock.h>
> >
> > -extern bool bcache_is_reboot;
> > +extern atomic_t bcache_is_reboot;
> >
> > /* Default is 0 ("writethrough") */
> > static const char * const bch_cache_modes[] = {
> > @@ -296,7 +296,7 @@ STORE(__cached_dev)
> > struct kobj_uevent_env *env;
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > #define d_strtoul(var) sysfs_strtoul(var, dc->var)
> > @@ -459,7 +459,7 @@ STORE(bch_cached_dev)
> > disk.kobj);
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > mutex_lock(&bch_register_lock);
> > @@ -571,7 +571,7 @@ STORE(__bch_flash_dev)
> > struct uuid_entry *u = &d->c->uuids[d->id];
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > sysfs_strtoul(data_csum, d->data_csum);
> > @@ -814,7 +814,7 @@ STORE(__bch_cache_set)
> > ssize_t v;
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > if (attr == &sysfs_unregister)
> > @@ -941,7 +941,7 @@ STORE(bch_cache_set_internal)
> > struct cache_set *c = container_of(kobj, struct cache_set, internal);
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > return bch_cache_set_store(&c->kobj, attr, buf, size);
> > @@ -1137,7 +1137,7 @@ STORE(__bch_cache)
> > ssize_t v;
> >
> > /* no user space access if system is rebooting */
> > - if (bcache_is_reboot)
> > + if (atomic_read(&bcache_is_reboot))
> > return -EBUSY;
> >
> > if (attr == &sysfs_cache_replacement_policy) {
> > --
> > 2.43.0
Powered by blists - more mailing lists