From 417b962ddeca2b70eb72d28c87541bdad4e234f8 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 25 Aug 2021 07:42:44 +0200 Subject: [PATCH 1/4] configfs: return -ENAMETOOLONG earlier in configfs_lookup Just like most other file systems: get the simple checks out of the way first. Signed-off-by: Christoph Hellwig --- fs/configfs/dir.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c index ac5e0c0e9181..cf08bbde55f3 100644 --- a/fs/configfs/dir.c +++ b/fs/configfs/dir.c @@ -456,6 +456,9 @@ static struct dentry * configfs_lookup(struct inode *dir, int found = 0; int err; + if (dentry->d_name.len > NAME_MAX) + return ERR_PTR(-ENAMETOOLONG); + /* * Fake invisibility if dir belongs to a group/default groups hierarchy * being attached @@ -486,8 +489,6 @@ static struct dentry * configfs_lookup(struct inode *dir, * If it doesn't exist and it isn't a NOT_PINNED item, * it must be negative. */ - if (dentry->d_name.len > NAME_MAX) - return ERR_PTR(-ENAMETOOLONG); d_add(dentry, NULL); return NULL; } From 899587c8d0908e5124fd074d52bf05b4b0633a79 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 25 Aug 2021 07:43:55 +0200 Subject: [PATCH 2/4] configfs: simplify the configfs_dirent_is_ready Return the error directly instead of using a goto. Signed-off-by: Christoph Hellwig --- fs/configfs/dir.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c index cf08bbde55f3..5d58569f0eea 100644 --- a/fs/configfs/dir.c +++ b/fs/configfs/dir.c @@ -467,9 +467,8 @@ static struct dentry * configfs_lookup(struct inode *dir, * not complete their initialization, since the dentries of the * attributes won't be instantiated. */ - err = -ENOENT; if (!configfs_dirent_is_ready(parent_sd)) - goto out; + return ERR_PTR(-ENOENT); list_for_each_entry(sd, &parent_sd->s_children, s_sibling) { if (sd->s_type & CONFIGFS_NOT_PINNED) { @@ -493,7 +492,6 @@ static struct dentry * configfs_lookup(struct inode *dir, return NULL; } -out: return ERR_PTR(err); } From d07f132a225c013e59aa77f514ad9211ecab82ee Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 25 Aug 2021 07:50:32 +0200 Subject: [PATCH 3/4] configfs: fold configfs_attach_attr into configfs_lookup This makes it more clear what gets added to the dcache and prepares for an additional locking fix. Signed-off-by: Christoph Hellwig --- fs/configfs/dir.c | 73 ++++++++++++++++------------------------------- 1 file changed, 24 insertions(+), 49 deletions(-) diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c index 5d58569f0eea..fc20bd8a6337 100644 --- a/fs/configfs/dir.c +++ b/fs/configfs/dir.c @@ -45,7 +45,7 @@ static void configfs_d_iput(struct dentry * dentry, /* * Set sd->s_dentry to null only when this dentry is the one * that is going to be killed. Otherwise configfs_d_iput may - * run just after configfs_attach_attr and set sd->s_dentry to + * run just after configfs_lookup and set sd->s_dentry to * NULL even it's still in use. */ if (sd->s_dentry == dentry) @@ -417,44 +417,13 @@ static void configfs_remove_dir(struct config_item * item) dput(dentry); } - -/* attaches attribute's configfs_dirent to the dentry corresponding to the - * attribute file - */ -static int configfs_attach_attr(struct configfs_dirent * sd, struct dentry * dentry) -{ - struct configfs_attribute * attr = sd->s_element; - struct inode *inode; - - spin_lock(&configfs_dirent_lock); - dentry->d_fsdata = configfs_get(sd); - sd->s_dentry = dentry; - spin_unlock(&configfs_dirent_lock); - - inode = configfs_create(dentry, (attr->ca_mode & S_IALLUGO) | S_IFREG); - if (IS_ERR(inode)) { - configfs_put(sd); - return PTR_ERR(inode); - } - if (sd->s_type & CONFIGFS_ITEM_BIN_ATTR) { - inode->i_size = 0; - inode->i_fop = &configfs_bin_file_operations; - } else { - inode->i_size = PAGE_SIZE; - inode->i_fop = &configfs_file_operations; - } - d_add(dentry, inode); - return 0; -} - static struct dentry * configfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) { struct configfs_dirent * parent_sd = dentry->d_parent->d_fsdata; struct configfs_dirent * sd; - int found = 0; - int err; + struct inode *inode = NULL; if (dentry->d_name.len > NAME_MAX) return ERR_PTR(-ENAMETOOLONG); @@ -471,28 +440,34 @@ static struct dentry * configfs_lookup(struct inode *dir, return ERR_PTR(-ENOENT); list_for_each_entry(sd, &parent_sd->s_children, s_sibling) { - if (sd->s_type & CONFIGFS_NOT_PINNED) { - const unsigned char * name = configfs_get_name(sd); + if ((sd->s_type & CONFIGFS_NOT_PINNED) && + !strcmp(configfs_get_name(sd), dentry->d_name.name)) { + struct configfs_attribute *attr = sd->s_element; + umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG; - if (strcmp(name, dentry->d_name.name)) - continue; + spin_lock(&configfs_dirent_lock); + dentry->d_fsdata = configfs_get(sd); + sd->s_dentry = dentry; + spin_unlock(&configfs_dirent_lock); - found = 1; - err = configfs_attach_attr(sd, dentry); + inode = configfs_create(dentry, mode); + if (IS_ERR(inode)) { + configfs_put(sd); + return ERR_CAST(inode); + } + if (sd->s_type & CONFIGFS_ITEM_BIN_ATTR) { + inode->i_size = 0; + inode->i_fop = &configfs_bin_file_operations; + } else { + inode->i_size = PAGE_SIZE; + inode->i_fop = &configfs_file_operations; + } break; } } - if (!found) { - /* - * If it doesn't exist and it isn't a NOT_PINNED item, - * it must be negative. - */ - d_add(dentry, NULL); - return NULL; - } - - return ERR_PTR(err); + d_add(dentry, inode); + return NULL; } /* From c42dd069be8dfc9b2239a5c89e73bbd08ab35de0 Mon Sep 17 00:00:00 2001 From: Sishuai Gong Date: Wed, 25 Aug 2021 07:52:20 +0200 Subject: [PATCH 4/4] configfs: fix a race in configfs_lookup() When configfs_lookup() is executing list_for_each_entry(), it is possible that configfs_dir_lseek() is calling list_del(). Some unfortunate interleavings of them can cause a kernel NULL pointer dereference error Thread 1 Thread 2 //configfs_dir_lseek() //configfs_lookup() list_del(&cursor->s_sibling); list_for_each_entry(sd, ...) Fix this by grabbing configfs_dirent_lock in configfs_lookup() while iterating ->s_children. Signed-off-by: Sishuai Gong Signed-off-by: Christoph Hellwig --- fs/configfs/dir.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c index fc20bd8a6337..1466b5d01cbb 100644 --- a/fs/configfs/dir.c +++ b/fs/configfs/dir.c @@ -439,13 +439,13 @@ static struct dentry * configfs_lookup(struct inode *dir, if (!configfs_dirent_is_ready(parent_sd)) return ERR_PTR(-ENOENT); + spin_lock(&configfs_dirent_lock); list_for_each_entry(sd, &parent_sd->s_children, s_sibling) { if ((sd->s_type & CONFIGFS_NOT_PINNED) && !strcmp(configfs_get_name(sd), dentry->d_name.name)) { struct configfs_attribute *attr = sd->s_element; umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG; - spin_lock(&configfs_dirent_lock); dentry->d_fsdata = configfs_get(sd); sd->s_dentry = dentry; spin_unlock(&configfs_dirent_lock); @@ -462,10 +462,11 @@ static struct dentry * configfs_lookup(struct inode *dir, inode->i_size = PAGE_SIZE; inode->i_fop = &configfs_file_operations; } - break; + goto done; } } - + spin_unlock(&configfs_dirent_lock); +done: d_add(dentry, inode); return NULL; }