| 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: <CAJnrk1Z_M4XP7dApmuLA9Na+7+9OO0he9EcaZJrubTrHKKUk8w@mail.gmail.com>
Date: Wed, 21 Jan 2026 16:23:08 -0800
From: Joanne Koong <joannelkoong@...il.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: miklos@...redi.hu, bernd@...ernd.com, neal@...pa.dev,
linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 03/31] fuse: make debugging configurable at runtime
On Wed, Jan 21, 2026 at 4:02 PM Darrick J. Wong <djwong@...nel.org> wrote:
>
> On Wed, Jan 21, 2026 at 03:42:04PM -0800, Joanne Koong wrote:
> > On Tue, Oct 28, 2025 at 5:45 PM Darrick J. Wong <djwong@...nel.org> wrote:
> > >
> > > From: Darrick J. Wong <djwong@...nel.org>
> > >
> > > Use static keys so that we can configure debugging assertions and dmesg
> > > warnings at runtime. By default this is turned off so the cost is
> > > merely scanning a nop sled. However, fuse server developers can turn
> > > it on for their debugging systems.
> > >
> > > Signed-off-by: "Darrick J. Wong" <djwong@...nel.org>
> > > ---
> > > fs/fuse/fuse_i.h | 8 +++++
> > > fs/fuse/iomap_i.h | 16 ++++++++--
> > > fs/fuse/Kconfig | 15 +++++++++
> > > fs/fuse/file_iomap.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > fs/fuse/inode.c | 7 ++++
> > > 5 files changed, 124 insertions(+), 3 deletions(-)
> > >
> > >
> > > diff --git a/fs/fuse/file_iomap.c b/fs/fuse/file_iomap.c
> > > index a88f5d8d2bce15..b6fc70068c5542 100644
> > > --- a/fs/fuse/file_iomap.c
> > > +++ b/fs/fuse/file_iomap.c
> > > @@ -8,6 +8,12 @@
> > > #include "fuse_trace.h"
> > > #include "iomap_i.h"
> > >
> > > +#if IS_ENABLED(CONFIG_FUSE_IOMAP_DEBUG_DEFAULT)
> > > +DEFINE_STATIC_KEY_TRUE(fuse_iomap_debug);
> > > +#else
> > > +DEFINE_STATIC_KEY_FALSE(fuse_iomap_debug);
> > > +#endif
> > > +
> > > static bool __read_mostly enable_iomap =
> > > #if IS_ENABLED(CONFIG_FUSE_IOMAP_BY_DEFAULT)
> > > true;
> > > @@ -17,6 +23,81 @@ static bool __read_mostly enable_iomap =
> > > module_param(enable_iomap, bool, 0644);
> > > MODULE_PARM_DESC(enable_iomap, "Enable file I/O through iomap");
> > >
> > > +#if IS_ENABLED(CONFIG_FUSE_IOMAP_DEBUG)
> > > +static struct kobject *iomap_kobj;
> > > +
> > > +static ssize_t fuse_iomap_debug_show(struct kobject *kobject,
> > > + struct kobj_attribute *a, char *buf)
> > > +{
> > > + return sysfs_emit(buf, "%d\n", !!static_key_enabled(&fuse_iomap_debug));
> > > +}
> > > +
> > > +static ssize_t fuse_iomap_debug_store(struct kobject *kobject,
> > > + struct kobj_attribute *a,
> > > + const char *buf, size_t count)
> > > +{
> > > + int ret;
> > > + int val;
> > > +
> > > + ret = kstrtoint(buf, 0, &val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (val < 0 || val > 1)
> > > + return -EINVAL;
> > > +
> > > + if (val)
> > > + static_branch_enable(&fuse_iomap_debug);
> > > + else
> > > + static_branch_disable(&fuse_iomap_debug);
> > > +
> > > + return count;
> > > +}
> > > +
> > > +#define __INIT_KOBJ_ATTR(_name, _mode, _show, _store) \
> > > +{ \
> > > + .attr = { .name = __stringify(_name), .mode = _mode }, \
> > > + .show = _show, \
> > > + .store = _store, \
> > > +}
> > > +
> > > +#define FUSE_ATTR_RW(_name, _show, _store) \
> > > + static struct kobj_attribute fuse_attr_##_name = \
> > > + __INIT_KOBJ_ATTR(_name, 0644, _show, _store)
> > > +
> > > +#define FUSE_ATTR_PTR(_name) \
> > > + (&fuse_attr_##_name.attr)
> > > +
> > > +FUSE_ATTR_RW(debug, fuse_iomap_debug_show, fuse_iomap_debug_store);
> > > +
> > > +static const struct attribute *fuse_iomap_attrs[] = {
> > > + FUSE_ATTR_PTR(debug),
> > > + NULL,
> > > +};
> > > +
> > > +int fuse_iomap_sysfs_init(struct kobject *fuse_kobj)
> > > +{
> > > + int error;
> > > +
> > > + iomap_kobj = kobject_create_and_add("iomap", fuse_kobj);
> > > + if (!iomap_kobj)
> > > + return -ENOMEM;
> > > +
> > > + error = sysfs_create_files(iomap_kobj, fuse_iomap_attrs);
> > > + if (error) {
> > > + kobject_put(iomap_kobj);
> > > + return error;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void fuse_iomap_sysfs_cleanup(struct kobject *fuse_kobj)
> > > +{
> >
> > Is sysfs_remove_files() also needed here?
>
> kobject_put is supposed to tear down the attrs that sysfs_create_files
> attaches to iomap_kobj. Though you're right to be suspicious -- there
> are a lot of places that explicitly call sysfs_remove_files to undo
> sysfs_create_files; and also a lot of places that just let kobject_put
> do the dirty work.
Makes sense, thanks for the context.
>
> > > + kobject_put(iomap_kobj);
> > > +}
> > > +#endif /* IS_ENABLED(CONFIG_FUSE_IOMAP_DEBUG) */
> > > +
> > > bool fuse_iomap_enabled(void)
> > > {
> > > /* Don't let anyone touch iomap until the end of the patchset. */
> > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > index 1eea8dc6e723c6..eec711302a4a13 100644
> > > --- a/fs/fuse/inode.c
> > > +++ b/fs/fuse/inode.c
> > > @@ -2277,8 +2277,14 @@ static int fuse_sysfs_init(void)
> > > if (err)
> > > goto out_fuse_unregister;
> > >
> > > + err = fuse_iomap_sysfs_init(fuse_kobj);
> > > + if (err)
> > > + goto out_fuse_connections;
> > > +
> > > return 0;
> > >
> > > + out_fuse_connections:
> > > + sysfs_remove_mount_point(fuse_kobj, "connections");
> > > out_fuse_unregister:
> > > kobject_put(fuse_kobj);
> > > out_err:
> > > @@ -2287,6 +2293,7 @@ static int fuse_sysfs_init(void)
> > >
> > > static void fuse_sysfs_cleanup(void)
> > > {
> > > + fuse_iomap_sysfs_cleanup(fuse_kobj);
> > > sysfs_remove_mount_point(fuse_kobj, "connections");
> > > kobject_put(fuse_kobj);
> > > }
> > >
> > Could you explain why it's better that this goes through sysfs than
> > through a module param?
>
> You can dynamically enable debugging on a production system. I (by
> which I really mean the support org) wishes they could do that with XFS.
>
> Module parameters don't come with setter functions so you can't call
> static_branch_{enable,disable} when the parameter value updates.
>
Ohh I thought the "module_param_cb()" stuff does let you do that and
can be dynamically enabled/disabled as well? I mostly ask because it
feels like it'd be nicer from a user POV if all the config stuff (eg
enable uring, enable iomap, etc.) is in one place.
Thanks,
Joanne
> --D
>
> > Thanks,
> > Joanne
Powered by blists - more mailing lists