Re: [PATCH 5.10 000/406] 5.10.195-rc1 review

[Kyle Zeng <zengyhkyle@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]

On Wed, Sep 20, 2023 at 10:01:55AM -0700, Florian Fainelli wrote:
> On 9/20/23 08:18, Guenter Roeck wrote:
> > On 9/20/23 01:11, Greg Kroah-Hartman wrote:
> > > On Tue, Sep 19, 2023 at 09:57:25PM -0700, Guenter Roeck wrote:
> > > > On 9/17/23 12:07, Greg Kroah-Hartman wrote:
> > > > > This is the start of the stable review cycle for the 5.10.195 release.
> > > > > There are 406 patches in this series, all will be posted as a response
> > > > > to this one.  If anyone has any issues with these being applied, please
> > > > > let me know.
> > > > > 
> > > > > Responses should be made by Tue, 19 Sep 2023 19:10:04 +0000.
> > > > > Anything received after that time might be too late.
> > > > > 
> > > > 
> > > > chromeos-5.10 locks up in configfs_lookup() after the merge of
> > > > v5.10.195.
> > > > 
> > > > I am a bit puzzled because I see
> > > > 
> > > > c709c7ca020a configfs: fix a race in configfs_lookup()
> > > > 
> > > > in v5.10.195 but not in the list of commits below. I guess I must be
> > > > missing something.
> > > 
> > > It was part of the big patchset, it was posted here:
> > >     https://lore.kernel.org/r/20230917191101.511939651@linuxfoundation.org
> > > 
> > > Not hidden at all :)
> > > 
> > > and was submitted here:
> > >     https://lore.kernel.org/r/ZPOZFHHA0abVmGx+@westworld
> > > 
> > > > Either case, the code now looks as follows.
> > > > 
> > > > configfs_lookup()
> > > > {
> > > >      ...
> > > >      spin_lock(&configfs_dirent_lock);
> > > >      ...
> > > >          err = configfs_attach_attr(sd, dentry);
> > > >      ...
> > > >      spin_unlock(&configfs_dirent_lock);
> > > >      ...
> > > > }
> > > > 
> > > > and
> > > > 
> > > > configfs_attach_attr(...)
> > > > {
> > > >      ...
> > > >      spin_lock(&configfs_dirent_lock);
> > > >      ...
> > > > }
> > > > 
> > > > which unless it is way too late here and I really need to go to sleep
> > > > just won't work.
> > > 
> > > Kyle, you did the backport, any comments?
> > > 
> > 
> > After a good night sleep, the code still looks wrong to me. Reverting
> > the offending patch in chromeos-5.10 solved the problem there.
> > That makes me suspect that no one actually tests configfs.
> 
> Humm indeed, looking at our testing we don't have our USB devices being
> tested which would exercise configfs since we switch the USB device between
> different configurations (mass storage, serial, networking etc.). Let me see
> about adding that so we get some coverage.
> -- 
> Florian
> 

Sorry for the wrong patch. My intention was to backport c42dd069be8dfc9b2239a5c89e73bbd08ab35de0
to v5.10 to avoid a race condition triggered in my test. I tested the
patch with my PoC program and made sure it won't trigger the crash. But
I didn't notice that it could hang the kernel.
I sincerely apologize for the mistake.

My new proposed patch backports both
c42dd069be8dfc9b2239a5c89e73bbd08ab35de0 and d07f132a225c013e59aa77f514ad9211ecab82ee.
I made sure it does not trigger the race condition anymore.
Can anyone having access to more comprehensive tests please check whether it works?

Also, I'm not sure whether it is OK or how to backport two patches in
one patch. Please advise on how to do it properly.

The crash triggering PoC program is also attached.

Thanks,
Kyle Zeng

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3493 bytes --]

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 12388ed4faa5..43093c218628 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -55,7 +55,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)
@@ -428,44 +428,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;
 
 	/*
 	 * Fake invisibility if dir belongs to a group/default groups hierarchy
@@ -475,36 +444,39 @@ 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);
 
+	spin_lock(&configfs_dirent_lock);
 	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;
+			dentry->d_fsdata = configfs_get(sd);
+			sd->s_dentry = dentry;
+			spin_unlock(&configfs_dirent_lock);
 
-			found = 1;
-			err = configfs_attach_attr(sd, dentry);
-			break;
+			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;
+			}
+			goto done;
 		}
 	}
-
-	if (!found) {
-		/*
-		 * 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;
-	}
-
-out:
-	return ERR_PTR(err);
+	spin_unlock(&configfs_dirent_lock);
+done:
+	d_add(dentry, inode);
+	return NULL;
 }
 
 /*

[-- Attachment #3: poc.c --]
[-- Type: text/x-csrc, Size: 240 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>

int main(void)
{
	int fd = open("/sys/kernel/config", 0);

	if(!fork()) {
		while(1) lseek(fd, SEEK_CUR, 1);
	}
	while(1) unlinkat(fd, "file", 0);

	return 0;
}