saner replacement for debugfs_rename()
authorAl Viro <viro@zeniv.linux.org.uk>
Sun, 12 Jan 2025 08:07:05 +0000 (08:07 +0000)
committerGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Wed, 15 Jan 2025 12:14:37 +0000 (13:14 +0100)
Existing primitive has several problems:
1) calling conventions are clumsy - it returns a dentry reference
that is either identical to its second argument or is an ERR_PTR(-E...);
in both cases no refcount changes happen.  Inconvenient for users and
bug-prone; it would be better to have it return 0 on success and -E... on
failure.
2) it allows cross-directory moves; however, no such caller have
ever materialized and considering the way debugfs is used, it's unlikely
to happen in the future.  What's more, any such caller would have fun
issues to deal with wrt interplay with recursive removal.  It also makes
the calling conventions clumsier...
3) tautological rename fails; the callers have no race-free way
to deal with that.
4) new name must have been formed by the caller; quite a few
callers have it done by sprintf/kasprintf/etc., ending up with considerable
boilerplate.

Proposed replacement: int debugfs_change_name(dentry, fmt, ...).  All callers
convert to that easily, and it's simpler internally.

IMO debugfs_rename() should go; if we ever get a real-world use case for
cross-directory moves in debugfs, we can always look into the right way
to handle that.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Link: https://lore.kernel.org/r/20250112080705.141166-21-viro@zeniv.linux.org.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
13 files changed:
Documentation/filesystems/debugfs.rst
drivers/net/bonding/bond_debugfs.c
drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
drivers/net/ethernet/marvell/skge.c
drivers/net/ethernet/marvell/sky2.c
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
drivers/opp/debugfs.c
fs/debugfs/inode.c
include/linux/debugfs.h
mm/shrinker_debug.c
net/hsr/hsr_debugfs.c
net/mac80211/debugfs_netdev.c
net/wireless/core.c

index dc35da8b8792c32bad183689c2a54854e372ca4a..f7f977ffbf8d4ee96b17165915b49618afea0a51 100644 (file)
@@ -211,18 +211,16 @@ seq_file content.
 
 There are a couple of other directory-oriented helper functions::
 
-    struct dentry *debugfs_rename(struct dentry *old_dir,
-                                 struct dentry *old_dentry,
-                                 struct dentry *new_dir,
-                                 const char *new_name);
+    struct dentry *debugfs_change_name(struct dentry *dentry,
+                                         const char *fmt, ...);
 
     struct dentry *debugfs_create_symlink(const char *name,
                                           struct dentry *parent,
                                          const char *target);
 
-A call to debugfs_rename() will give a new name to an existing debugfs
-file, possibly in a different directory.  The new_name must not exist prior
-to the call; the return value is old_dentry with updated information.
+A call to debugfs_change_name() will give a new name to an existing debugfs
+file, always in the same directory.  The new_name must not exist prior
+to the call; the return value is 0 on success and -E... on failuer.
 Symbolic links can be created with debugfs_create_symlink().
 
 There is one important thing that all debugfs users must take into account:
index b19492a7f6ad133c53ad68a58386704a19c653b8..8adbec7c5084aac1d043a8c36f76aa7c7ba166a5 100644 (file)
@@ -63,13 +63,8 @@ void bond_debug_unregister(struct bonding *bond)
 
 void bond_debug_reregister(struct bonding *bond)
 {
-       struct dentry *d;
-
-       d = debugfs_rename(bonding_debug_root, bond->debug_dir,
-                          bonding_debug_root, bond->dev->name);
-       if (!IS_ERR(d)) {
-               bond->debug_dir = d;
-       } else {
+       int err = debugfs_change_name(bond->debug_dir, "%s", bond->dev->name);
+       if (err) {
                netdev_warn(bond->dev, "failed to reregister, so just unregister old one\n");
                bond_debug_unregister(bond);
        }
index b0a6c96b6ef46418422647616804dd48a8b34610..b35808d3d07f79d88739a77370a798e0b4982692 100644 (file)
@@ -505,21 +505,6 @@ void xgbe_debugfs_exit(struct xgbe_prv_data *pdata)
 
 void xgbe_debugfs_rename(struct xgbe_prv_data *pdata)
 {
-       char *buf;
-
-       if (!pdata->xgbe_debugfs)
-               return;
-
-       buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
-       if (!buf)
-               return;
-
-       if (!strcmp(pdata->xgbe_debugfs->d_name.name, buf))
-               goto out;
-
-       debugfs_rename(pdata->xgbe_debugfs->d_parent, pdata->xgbe_debugfs,
-                      pdata->xgbe_debugfs->d_parent, buf);
-
-out:
-       kfree(buf);
+       debugfs_change_name(pdata->xgbe_debugfs,
+                           "amd-xgbe-%s", pdata->netdev->name);
 }
index 25bf6ec4428964a485cbae3d856097cb19e83d44..a1bada9eaaf62987bde3ac167938acfdf99c9f3c 100644 (file)
@@ -3742,10 +3742,7 @@ static int skge_device_event(struct notifier_block *unused,
        skge = netdev_priv(dev);
        switch (event) {
        case NETDEV_CHANGENAME:
-               if (skge->debugfs)
-                       skge->debugfs = debugfs_rename(skge_debug,
-                                                      skge->debugfs,
-                                                      skge_debug, dev->name);
+               debugfs_change_name(skge->debugfs, "%s", dev->name);
                break;
 
        case NETDEV_GOING_DOWN:
index 988fa28cfb5ff842ea4872dcc588b37ae8e354bb..d7121c83650801638c63921bc59f2c995f940de5 100644 (file)
@@ -4494,10 +4494,7 @@ static int sky2_device_event(struct notifier_block *unused,
 
        switch (event) {
        case NETDEV_CHANGENAME:
-               if (sky2->debugfs) {
-                       sky2->debugfs = debugfs_rename(sky2_debug, sky2->debugfs,
-                                                      sky2_debug, dev->name);
-               }
+               debugfs_change_name(sky2->debugfs, "%s", dev->name);
                break;
 
        case NETDEV_GOING_DOWN:
index c81ea8cdfe6eb8f8cfe492a0c486d26b6b87551b..82e2908016bdd0db589f1bdeff55ddb5c7ab6861 100644 (file)
@@ -6489,11 +6489,7 @@ static int stmmac_device_event(struct notifier_block *unused,
 
        switch (event) {
        case NETDEV_CHANGENAME:
-               if (priv->dbgfs_dir)
-                       priv->dbgfs_dir = debugfs_rename(stmmac_fs_dir,
-                                                        priv->dbgfs_dir,
-                                                        stmmac_fs_dir,
-                                                        dev->name);
+               debugfs_change_name(priv->dbgfs_dir, "%s", dev->name);
                break;
        }
 done:
index 105de7c3274ad43d93b715b6a6c645b45d835055..8fc6238b17284183558c4b5d3ecf92dfdf931b8b 100644 (file)
@@ -217,7 +217,7 @@ static void opp_migrate_dentry(struct opp_device *opp_dev,
 {
        struct opp_device *new_dev = NULL, *iter;
        const struct device *dev;
-       struct dentry *dentry;
+       int err;
 
        /* Look for next opp-dev */
        list_for_each_entry(iter, &opp_table->dev_list, node)
@@ -234,16 +234,14 @@ static void opp_migrate_dentry(struct opp_device *opp_dev,
 
        opp_set_dev_name(dev, opp_table->dentry_name);
 
-       dentry = debugfs_rename(rootdir, opp_dev->dentry, rootdir,
-                               opp_table->dentry_name);
-       if (IS_ERR(dentry)) {
+       err = debugfs_change_name(opp_dev->dentry, "%s", opp_table->dentry_name);
+       if (err) {
                dev_err(dev, "%s: Failed to rename link from: %s to %s\n",
                        __func__, dev_name(opp_dev->dev), dev_name(dev));
                return;
        }
 
-       new_dev->dentry = dentry;
-       opp_table->dentry = dentry;
+       new_dev->dentry = opp_table->dentry = opp_dev->dentry;
 }
 
 /**
index 51d4c3e9d4220558d6f51f04a2734a7190dbf326..75715d8877eedf7c500a69c648cb577c930106eb 100644 (file)
@@ -830,76 +830,70 @@ void debugfs_lookup_and_remove(const char *name, struct dentry *parent)
 EXPORT_SYMBOL_GPL(debugfs_lookup_and_remove);
 
 /**
- * debugfs_rename - rename a file/directory in the debugfs filesystem
- * @old_dir: a pointer to the parent dentry for the renamed object. This
- *          should be a directory dentry.
- * @old_dentry: dentry of an object to be renamed.
- * @new_dir: a pointer to the parent dentry where the object should be
- *          moved. This should be a directory dentry.
- * @new_name: a pointer to a string containing the target name.
+ * debugfs_change_name - rename a file/directory in the debugfs filesystem
+ * @dentry: dentry of an object to be renamed.
+ * @fmt: format for new name
  *
  * This function renames a file/directory in debugfs.  The target must not
  * exist for rename to succeed.
  *
- * This function will return a pointer to old_dentry (which is updated to
- * reflect renaming) if it succeeds. If an error occurs, ERR_PTR(-ERROR)
- * will be returned.
+ * This function will return 0 on success and -E... on failure.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
  * returned.
  */
-struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
-               struct dentry *new_dir, const char *new_name)
+int __printf(2, 3) debugfs_change_name(struct dentry *dentry, const char *fmt, ...)
 {
-       int error;
-       struct dentry *dentry = NULL, *trap;
+       int error = 0;
+       const char *new_name;
        struct name_snapshot old_name;
+       struct dentry *parent, *target;
+       struct inode *dir;
+       va_list ap;
 
-       if (IS_ERR(old_dir))
-               return old_dir;
-       if (IS_ERR(new_dir))
-               return new_dir;
-       if (IS_ERR_OR_NULL(old_dentry))
-               return old_dentry;
-
-       trap = lock_rename(new_dir, old_dir);
-       /* Source or destination directories don't exist? */
-       if (d_really_is_negative(old_dir) || d_really_is_negative(new_dir))
-               goto exit;
-       /* Source does not exist, cyclic rename, or mountpoint? */
-       if (d_really_is_negative(old_dentry) || old_dentry == trap ||
-           d_mountpoint(old_dentry))
-               goto exit;
-       dentry = lookup_one_len(new_name, new_dir, strlen(new_name));
-       /* Lookup failed, cyclic rename or target exists? */
-       if (IS_ERR(dentry) || dentry == trap || d_really_is_positive(dentry))
-               goto exit;
-
-       take_dentry_name_snapshot(&old_name, old_dentry);
-
-       error = simple_rename(&nop_mnt_idmap, d_inode(old_dir), old_dentry,
-                             d_inode(new_dir), dentry, 0);
-       if (error) {
-               release_dentry_name_snapshot(&old_name);
-               goto exit;
+       if (IS_ERR_OR_NULL(dentry))
+               return 0;
+
+       va_start(ap, fmt);
+       new_name = kvasprintf_const(GFP_KERNEL, fmt, ap);
+       va_end(ap);
+       if (!new_name)
+               return -ENOMEM;
+
+       parent = dget_parent(dentry);
+       dir = d_inode(parent);
+       inode_lock(dir);
+
+       take_dentry_name_snapshot(&old_name, dentry);
+
+       if (WARN_ON_ONCE(dentry->d_parent != parent)) {
+               error = -EINVAL;
+               goto out;
+       }
+       if (strcmp(old_name.name.name, new_name) == 0)
+               goto out;
+       target = lookup_one_len(new_name, parent, strlen(new_name));
+       if (IS_ERR(target)) {
+               error = PTR_ERR(target);
+               goto out;
        }
-       d_move(old_dentry, dentry);
-       fsnotify_move(d_inode(old_dir), d_inode(new_dir), &old_name.name,
-               d_is_dir(old_dentry),
-               NULL, old_dentry);
+       if (d_really_is_positive(target)) {
+               dput(target);
+               error = -EINVAL;
+               goto out;
+       }
+       simple_rename_timestamp(dir, dentry, dir, target);
+       d_move(dentry, target);
+       dput(target);
+       fsnotify_move(dir, dir, &old_name.name, d_is_dir(dentry), NULL, dentry);
+out:
        release_dentry_name_snapshot(&old_name);
-       unlock_rename(new_dir, old_dir);
-       dput(dentry);
-       return old_dentry;
-exit:
-       if (dentry && !IS_ERR(dentry))
-               dput(dentry);
-       unlock_rename(new_dir, old_dir);
-       if (IS_ERR(dentry))
-               return dentry;
-       return ERR_PTR(-EINVAL);
+       inode_unlock(dir);
+       dput(parent);
+       kfree_const(new_name);
+       return error;
 }
-EXPORT_SYMBOL_GPL(debugfs_rename);
+EXPORT_SYMBOL_GPL(debugfs_change_name);
 
 /**
  * debugfs_initialized - Tells whether debugfs has been registered
index 68e9c6cbd835bb132f4363aafee023e8330cd306..fa2568b4380da0f03ec2261cb863d074d6a33efa 100644 (file)
@@ -175,8 +175,7 @@ ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
 ssize_t debugfs_attr_write_signed(struct file *file, const char __user *buf,
                        size_t len, loff_t *ppos);
 
-struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
-                struct dentry *new_dir, const char *new_name);
+int debugfs_change_name(struct dentry *dentry, const char *fmt, ...) __printf(2, 3);
 
 void debugfs_create_u8(const char *name, umode_t mode, struct dentry *parent,
                       u8 *value);
@@ -361,10 +360,10 @@ static inline ssize_t debugfs_attr_write_signed(struct file *file,
        return -ENODEV;
 }
 
-static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
-                struct dentry *new_dir, char *new_name)
+static inline int __printf(2, 3) debugfs_change_name(struct dentry *dentry,
+                                       const char *fmt, ...)
 {
-       return ERR_PTR(-ENODEV);
+       return -ENODEV;
 }
 
 static inline void debugfs_create_u8(const char *name, umode_t mode,
index 4a85b94d12ce2fef467645a8b699bc325571cc7b..794bd433cce0c6e249c070f42fcd18c64bcd582c 100644 (file)
@@ -195,8 +195,6 @@ int shrinker_debugfs_add(struct shrinker *shrinker)
 
 int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...)
 {
-       struct dentry *entry;
-       char buf[128];
        const char *new, *old;
        va_list ap;
        int ret = 0;
@@ -213,18 +211,8 @@ int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...)
        old = shrinker->name;
        shrinker->name = new;
 
-       if (shrinker->debugfs_entry) {
-               snprintf(buf, sizeof(buf), "%s-%d", shrinker->name,
-                        shrinker->debugfs_id);
-
-               entry = debugfs_rename(shrinker_debugfs_root,
-                                      shrinker->debugfs_entry,
-                                      shrinker_debugfs_root, buf);
-               if (IS_ERR(entry))
-                       ret = PTR_ERR(entry);
-               else
-                       shrinker->debugfs_entry = entry;
-       }
+       ret = debugfs_change_name(shrinker->debugfs_entry, "%s-%d",
+                       shrinker->name, shrinker->debugfs_id);
 
        mutex_unlock(&shrinker_mutex);
 
index 1a195efc79cd17e89109430b9992354c1ffcae2f..5b2cfac3b2bacb69330d477c896e4bf9940490eb 100644 (file)
@@ -57,14 +57,11 @@ DEFINE_SHOW_ATTRIBUTE(hsr_node_table);
 void hsr_debugfs_rename(struct net_device *dev)
 {
        struct hsr_priv *priv = netdev_priv(dev);
-       struct dentry *d;
+       int err;
 
-       d = debugfs_rename(hsr_debugfs_root_dir, priv->node_tbl_root,
-                          hsr_debugfs_root_dir, dev->name);
-       if (IS_ERR(d))
+       err = debugfs_change_name(priv->node_tbl_root, "%s", dev->name);
+       if (err)
                netdev_warn(dev, "failed to rename\n");
-       else
-               priv->node_tbl_root = d;
 }
 
 /* hsr_debugfs_init - create hsr node_table file for dumping
index a9bc2fd59f55ad3b1efec7f47d499fbeb4448584..9fa38c489edcce963cc8b98b13a21cbf42033432 100644 (file)
@@ -1025,16 +1025,7 @@ void ieee80211_debugfs_remove_netdev(struct ieee80211_sub_if_data *sdata)
 
 void ieee80211_debugfs_rename_netdev(struct ieee80211_sub_if_data *sdata)
 {
-       struct dentry *dir;
-       char buf[10 + IFNAMSIZ];
-
-       dir = sdata->vif.debugfs_dir;
-
-       if (IS_ERR_OR_NULL(dir))
-               return;
-
-       sprintf(buf, "netdev:%s", sdata->name);
-       debugfs_rename(dir->d_parent, dir, dir->d_parent, buf);
+       debugfs_change_name(sdata->vif.debugfs_dir, "netdev:%s", sdata->name);
 }
 
 void ieee80211_debugfs_recreate_netdev(struct ieee80211_sub_if_data *sdata,
index afbdc549fb4a51cb732d1c8dd420133e70b07190..9130cb872ed3469f79bdd9c8d4c7da41bf42c15c 100644 (file)
@@ -143,10 +143,7 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
        if (result)
                return result;
 
-       if (!IS_ERR_OR_NULL(rdev->wiphy.debugfsdir))
-               debugfs_rename(rdev->wiphy.debugfsdir->d_parent,
-                              rdev->wiphy.debugfsdir,
-                              rdev->wiphy.debugfsdir->d_parent, newname);
+       debugfs_change_name(rdev->wiphy.debugfsdir, "%s", newname);
 
        nl80211_notify_wiphy(rdev, NL80211_CMD_NEW_WIPHY);