debugfs: don't mess with bits in ->d_fsdata

The reason we need that crap is the dual use ->d_fsdata has there -
it's both holding a debugfs_fsdata reference after the first
debugfs_file_get() (actually, after the call of proxy ->open())
*and* it serves as a place to stash a reference to real file_operations
from object creation to the first open.  Oh, and it's triple use,
actually - that stashed reference might be to debugfs_short_fops.

Bugger that for a game of solidiers - just put the operations
reference into debugfs-private augmentation of inode.  And split
debugfs_full_file_operations into full and short cases, so that
debugfs_get_file() could tell one from another.

Voila - ->d_fsdata holds NULL until the first (successful) debugfs_get_file()
and a reference to struct debugfs_fsdata afterwards.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Reviewed-by: Christian Brauner <brauner@kernel.org>
Link: https://lore.kernel.org/r/20250112080705.141166-4-viro@zeniv.linux.org.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index eb59b01f5f2588e4d9cd30241836acda973f5574..ae014bd36a6ff641c2ae1005d6e8b7889ea7cace 100644 (file)
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -51,7 +51,7 @@ const struct file_operations *debugfs_real_fops(const struct file *filp)
 {
        struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
 
-       if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT) {
+       if (!fsd) {
                /*
                 * Urgh, we've been called w/o a protecting
                 * debugfs_file_get().
@@ -84,9 +84,11 @@ static int __debugfs_file_get(struct dentry *dentry, enum dbgfs_get_mode mode)
                return -EINVAL;
 
        d_fsd = READ_ONCE(dentry->d_fsdata);
-       if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
+       if (d_fsd) {
                fsd = d_fsd;
        } else {
+               struct inode *inode = dentry->d_inode;
+
                if (WARN_ON(mode == DBGFS_GET_ALREADY))
                        return -EINVAL;
 
@@ -96,9 +98,7 @@ static int __debugfs_file_get(struct dentry *dentry, enum dbgfs_get_mode mode)
 
                if (mode == DBGFS_GET_SHORT) {
                        const struct debugfs_short_fops *ops;
-                       ops = (void *)((unsigned long)d_fsd &
-                                       ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
-                       fsd->short_fops = ops;
+                       ops = fsd->short_fops = DEBUGFS_I(inode)->short_fops;
                        if (ops->llseek)
                                fsd->methods |= HAS_LSEEK;
                        if (ops->read)
@@ -107,9 +107,7 @@ static int __debugfs_file_get(struct dentry *dentry, enum dbgfs_get_mode mode)
                                fsd->methods |= HAS_WRITE;
                } else {
                        const struct file_operations *ops;
-                       ops = (void *)((unsigned long)d_fsd &
-                                       ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
-                       fsd->real_fops = ops;
+                       ops = fsd->real_fops = DEBUGFS_I(inode)->real_fops;
                        if (ops->llseek)
                                fsd->methods |= HAS_LSEEK;
                        if (ops->read)
@@ -126,10 +124,11 @@ static int __debugfs_file_get(struct dentry *dentry, enum dbgfs_get_mode mode)
                INIT_LIST_HEAD(&fsd->cancellations);
                mutex_init(&fsd->cancellations_mtx);
 
-               if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) {
+               d_fsd = cmpxchg(&dentry->d_fsdata, NULL, fsd);
+               if (d_fsd) {
                        mutex_destroy(&fsd->cancellations_mtx);
                        kfree(fsd);
-                       fsd = READ_ONCE(dentry->d_fsdata);
+                       fsd = d_fsd;
                }
        }
 
@@ -226,8 +225,7 @@ void debugfs_enter_cancellation(struct file *file,
                return;
 
        fsd = READ_ONCE(dentry->d_fsdata);
-       if (WARN_ON(!fsd ||
-                   ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
+       if (WARN_ON(!fsd))
                return;
 
        mutex_lock(&fsd->cancellations_mtx);
@@ -258,8 +256,7 @@ void debugfs_leave_cancellation(struct file *file,
                return;
 
        fsd = READ_ONCE(dentry->d_fsdata);
-       if (WARN_ON(!fsd ||
-                   ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
+       if (WARN_ON(!fsd))
                return;
 
        mutex_lock(&fsd->cancellations_mtx);
@@ -427,22 +424,21 @@ static int full_proxy_release(struct inode *inode, struct file *filp)
         * not to leak any resources. Releasers must not assume that
         * ->i_private is still being meaningful here.
         */
-       if (real_fops && real_fops->release)
+       if (real_fops->release)
                r = real_fops->release(inode, filp);
 
        fops_put(real_fops);
        return r;
 }
 
-static int full_proxy_open(struct inode *inode, struct file *filp,
-                          enum dbgfs_get_mode mode)
+static int full_proxy_open_regular(struct inode *inode, struct file *filp)
 {
        struct dentry *dentry = F_DENTRY(filp);
        const struct file_operations *real_fops;
        struct debugfs_fsdata *fsd;
        int r;
 
-       r = __debugfs_file_get(dentry, mode);
+       r = __debugfs_file_get(dentry, DBGFS_GET_REGULAR);
        if (r)
                return r == -EIO ? -ENOENT : r;
 
@@ -452,7 +448,7 @@ static int full_proxy_open(struct inode *inode, struct file *filp,
        if (r)
                goto out;
 
-       if (real_fops && !fops_get(real_fops)) {
+       if (!fops_get(real_fops)) {
 #ifdef CONFIG_MODULES
                if (real_fops->owner &&
                    real_fops->owner->state == MODULE_STATE_GOING) {
@@ -468,11 +464,8 @@ static int full_proxy_open(struct inode *inode, struct file *filp,
                goto out;
        }
 
-       if (!real_fops || real_fops->open) {
-               if (real_fops)
-                       r = real_fops->open(inode, filp);
-               else
-                       r = simple_open(inode, filp);
+       if (real_fops->open) {
+               r = real_fops->open(inode, filp);
                if (r) {
                        fops_put(real_fops);
                } else if (filp->f_op != &debugfs_full_proxy_file_operations) {
@@ -487,11 +480,6 @@ out:
        return r;
 }
 
-static int full_proxy_open_regular(struct inode *inode, struct file *filp)
-{
-       return full_proxy_open(inode, filp, DBGFS_GET_REGULAR);
-}
-
 const struct file_operations debugfs_full_proxy_file_operations = {
        .open = full_proxy_open_regular,
        .release = full_proxy_release,
@@ -504,7 +492,17 @@ const struct file_operations debugfs_full_proxy_file_operations = {
 
 static int full_proxy_open_short(struct inode *inode, struct file *filp)
 {
-       return full_proxy_open(inode, filp, DBGFS_GET_SHORT);
+       struct dentry *dentry = F_DENTRY(filp);
+       int r;
+
+       r = __debugfs_file_get(dentry, DBGFS_GET_SHORT);
+       if (r)
+               return r == -EIO ? -ENOENT : r;
+       r = debugfs_locked_down(inode, filp, NULL);
+       if (!r)
+               r = simple_open(inode, filp);
+       debugfs_file_put(dentry);
+       return r;
 }
 
 const struct file_operations debugfs_full_short_proxy_file_operations = {

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 2f5afd7b1b942d3f9685ffd3e74e2fb6a61f5972..c4e8b7f758e0e29ace5653e2e2aba038b4f645fe 100644 (file)
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -243,15 +243,10 @@ static void debugfs_release_dentry(struct dentry *dentry)
 {
        struct debugfs_fsdata *fsd = dentry->d_fsdata;
 
-       if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
-               return;
-
-       /* check it wasn't a dir or automount (no fsdata) */
        if (fsd) {
                WARN_ON(!list_empty(&fsd->cancellations));
                mutex_destroy(&fsd->cancellations_mtx);
        }
-
        kfree(fsd);
 }
 
@@ -459,9 +454,10 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
        inode->i_private = data;
 
        inode->i_op = &debugfs_file_inode_operations;
+       if (!real_fops)
+               proxy_fops = &debugfs_noop_file_operations;
        inode->i_fop = proxy_fops;
-       dentry->d_fsdata = (void *)((unsigned long)real_fops |
-                               DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+       DEBUGFS_I(inode)->raw = real_fops;
 
        d_instantiate(dentry, inode);
        fsnotify_create(d_inode(dentry->d_parent), dentry);
@@ -472,13 +468,8 @@ struct dentry *debugfs_create_file_full(const char *name, umode_t mode,
                                        struct dentry *parent, void *data,
                                        const struct file_operations *fops)
 {
-       if (WARN_ON((unsigned long)fops &
-                   DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
-               return ERR_PTR(-EINVAL);
-
        return __debugfs_create_file(name, mode, parent, data,
-                               fops ? &debugfs_full_proxy_file_operations :
-                                       &debugfs_noop_file_operations,
+                               &debugfs_full_proxy_file_operations,
                                fops);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_file_full);
@@ -487,13 +478,8 @@ struct dentry *debugfs_create_file_short(const char *name, umode_t mode,
                                         struct dentry *parent, void *data,
                                         const struct debugfs_short_fops *fops)
 {
-       if (WARN_ON((unsigned long)fops &
-                   DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
-               return ERR_PTR(-EINVAL);
-
        return __debugfs_create_file(name, mode, parent, data,
-                               fops ? &debugfs_full_short_proxy_file_operations :
-                                       &debugfs_noop_file_operations,
+                               &debugfs_full_short_proxy_file_operations,
                                fops);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_file_short);
@@ -531,8 +517,7 @@ struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
 {
 
        return __debugfs_create_file(name, mode, parent, data,
-                               fops ? &debugfs_open_proxy_file_operations :
-                                       &debugfs_noop_file_operations,
+                               &debugfs_open_proxy_file_operations,
                                fops);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
@@ -737,7 +722,7 @@ static void __debugfs_file_removed(struct dentry *dentry)
         */
        smp_mb();
        fsd = READ_ONCE(dentry->d_fsdata);
-       if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
+       if (!fsd)
                return;
 
        /* if this was the last reference, we're done */

diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index 011ef8b1a99a121a635699ad6e05cce662afa76d..8d2de647b42cb60119b3469cbcb02a69dccf2563 100644 (file)
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -14,6 +14,9 @@ struct file_operations;
 struct debugfs_inode_info {
        struct inode vfs_inode;
        union {
+               const void *raw;
+               const struct file_operations *real_fops;
+               const struct debugfs_short_fops *short_fops;
                debugfs_automount_t automount;
        };
 };
@@ -51,15 +54,6 @@ enum {
        HAS_IOCTL = 16
 };
 
-/*
- * A dentry's ->d_fsdata either points to the real fops or to a
- * dynamically allocated debugfs_fsdata instance.
- * In order to distinguish between these two cases, a real fops
- * pointer gets its lowest bit set.
- */
-#define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
-
-/* Access BITS */
 #define DEBUGFS_ALLOW_API      BIT(0)
 #define DEBUGFS_ALLOW_MOUNT    BIT(1)