| 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: <20260122004027.GM5966@frogsfrogsfrogs>
Date: Wed, 21 Jan 2026 16:40:27 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Joanne Koong <joannelkoong@...il.com>
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 04:23:08PM -0800, Joanne Koong wrote:
> 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.
TIL today.
HAH well that's been there since 2.6.0. Silly me, that's been there
forever.
I'll switch it to a magic module parameter that has a setter. Much
easier than thinking about /anything/ related to sysfs.
--D
> Thanks,
> Joanne
>
> > --D
> >
> > > Thanks,
> > > Joanne
Powered by blists - more mailing lists