sysfs: make sysfs_lookup() return ERR_PTR(-ENOENT) on failed lookup

PATCH In review Tejun Heo 4 participants 8 messages Jan 16, 2008 – Jan 17, 2008

Subsystems (2)

Bluetooth drivers Driver core

lore

Share & tools

lore

Tejun Heo

author

2008-01-16 03:06 UTC #0 lore raw

PATCH 1/2 sysfs: make sysfs_lookup() return ERR_PTR(-ENOENT) on failed lookup

sysfs tries to keep dcache a strict subset of sysfs_dirent tree by shooting down dentries when a node is removed, that is, no negative dentry for sysfs. However, the lookup function returned NULL and thus created negative dentries when the target node didn't exist.

Make sysfs_lookup() return ERR_PTR(-ENOENT) on lookup failure. This fixes the NULL dereference bug in sysfs_get_dentry() discovered by bluetooth rfcomm device moving around.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/sysfs/dir.c +3 -1

Index: work/fs/sysfs/dir.c
===================================================================
@@ -678,8 +678,10 @@ static struct dentry * sysfs_lookup(stru
 	sd = sysfs_find_dirent(parent_sd, dentry->d_name.name);
 
 	/* no such entry */
-	if (!sd)
+	if (!sd) {
+		ret = ERR_PTR(-ENOENT);
 		goto out_unlock;
+	}
 
 	/* attach dentry and inode */
 	inode = sysfs_get_inode(sd);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Tejun Heo

author

2008-01-16 03:10 UTC #1 lore raw

PATCH 2/2 sysfs: fix bugs in sysfs_rename/move_dir()

sysfs_rename/move_dir() have the following bugs.

* On dentry lookup failure, kfree() is called on ERR_PTR() value.
* sysfs_move_dir() has an extra dput() on success path.

Fix them.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/sysfs/dir.c +3 -1

Index: work/fs/sysfs/dir.c
===================================================================
@@ -783,6 +783,7 @@ int sysfs_rename_dir(struct kobject * ko
 	old_dentry = sysfs_get_dentry(sd);
 	if (IS_ERR(old_dentry)) {
 		error = PTR_ERR(old_dentry);
+		old_dentry = NULL;
 		goto out;
 	}

@@ -850,6 +851,7 @@ int sysfs_move_dir(struct kobject *kobj,
 	old_dentry = sysfs_get_dentry(sd);
 	if (IS_ERR(old_dentry)) {
 		error = PTR_ERR(old_dentry);
+		old_dentry = NULL;
 		goto out;
 	}
 	old_parent = old_dentry->d_parent;

@@ -857,6 +859,7 @@ int sysfs_move_dir(struct kobject *kobj,
 	new_parent = sysfs_get_dentry(new_parent_sd);
 	if (IS_ERR(new_parent)) {
 		error = PTR_ERR(new_parent);
+		new_parent = NULL;
 		goto out;
 	}

@@ -880,7 +883,6 @@ again:
 	error = 0;
 	d_add(new_dentry, NULL);
 	d_move(old_dentry, new_dentry);
-	dput(new_dentry);
 
 	/* Remove from old parent's list and insert into new parent's list. */
 	sysfs_unlink_sibling(sd);

Linus Torvalds

2008-01-16 03:41 UTC #2 lore raw

Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in sysfs_rename/move_dir()

Quoted 2 lines · Tejun Heo

* sysfs_move_dir() has an extra dput() on success path.

Are you sure? How did this ever work?

Also, looking at this, I think the "how did this ever work" question is answered by "it didn't", but I also think there are still serious problems there. Look at

	again:
	        mutex_lock(&old_parent->d_inode->i_mutex);
	        if (!mutex_trylock(&new_parent->d_inode->i_mutex)) {
	                mutex_unlock(&old_parent->d_inode->i_mutex);
	                goto again;
	        }

and wonder what happen sif old_parent == new_parent. Is that trying to avoid an ABBA deadlock? Normally you'd do it by ordering the locks, or by taking a third lock to guarantee serialization at a higher level (ie the "s_vfs_rename_mutex" on the VFS layer)

I'd like to apply these two patches, but I really want to get more of an ack for them from somebody like Al, or at least more of an explanation for why it's all the right thing.

Linus

Al Viro

2008-01-16 03:52 UTC #3 lore raw

Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in sysfs_rename/move_dir()

Quoted 8 lines · Linus Torvalds

and wonder what happen sif old_parent == new_parent. Is that trying to avoid an ABBA deadlock? Normally you'd do it by ordering the locks, or by taking a third lock to guarantee serialization at a higher level (ie the "s_vfs_rename_mutex" on the VFS layer)
I'd like to apply these two patches, but I really want to get more of an ack for them from somebody like Al, or at least more of an explanation for why it's all the right thing.

No ACK is coming until we get something resembling analysis of locking scheme. Which won't happen until we at least get the "what callers are allowed to do" written down, damnit. As it is, I'm more than inclined to propose ripping kobject_move() out, especially since it has only two users - something s390-specific and rfcomm, with its shitloads of problems beyond just sysfs interaction.

Tejun Heo

author

2008-01-16 07:23 UTC #4 lore raw

Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in sysfs_rename/move_dir()

Hello.

Quoted 3 lines · Al Viro

No ACK is coming until we get something resembling analysis of locking scheme. Which won't happen until we at least get the "what callers are allowed to do" written down, damnit.

I agree that sysfs needs further clean up. As I wrote in the earlier thread, sysfs has been under constant flux of cleanups and updates although it has slowed down recently due to the hazy number of libata bugs. For example, several months ago with buggy dentry / inode reclamation, sysfs could trigger pretty cryptic oopses under memory pressure and locking was more awkward and buggy.

The two posted patches are bug fixes for apparent bugs which can be triggered by the current two users of the interface. AFAICS, locking there is weird but correct for the current two users. If you can find any problem there, please lemme know. We shouldn't hold this type of fixes for future clean ups.

Quoted 4 lines

As it is, I'm more than inclined
to propose ripping kobject_move() out, especially since it has only two users - something s390-specific and rfcomm, with its shitloads of problems beyond just sysfs interaction.

Can you please elaborate? All sysfs problems discovered by the rfcomm are fixed by the posted patches. Dave Young has a patch waiting for verification by the tester. Furthermore, even if we rip out kobject_move() in the future, I don't think -rc7 is the right time to do it.

I posted some patches a while back which did sysfs locking reorganization, separation from and proper layering with kobject / driver model. There were some disagreements regarding the interface and I got struck by load of ATA bugs. I'll dig it up and give it another shot in a few weeks.

Thanks.

Signature · 11 lines

tejun


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Al Viro

2008-01-17 04:22 UTC #5 lore raw

Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in sysfs_rename/move_dir()

Quoted 4 lines · Tejun Heo

The two posted patches are bug fixes for apparent bugs which can be triggered by the current two users of the interface. AFAICS, locking there is weird but correct for the current two users. If you can find any problem there, please lemme know.

How about "what happens after that move-to-NULL if you have a cwd inside the subtree", for starters?

Quoted 2 lines

We shouldn't hold this type of
fixes for future clean ups.

No, but I'd rather see the rules for callers of sysfs/kobject primitives spelled out - before cleanups or review become even possible.

Quoted 8 lines

Quoted 4 lines
As it is, I'm more than inclined
to propose ripping kobject_move() out, especially since it has only two users - something s390-specific and rfcomm, with its shitloads of problems beyond just sysfs interaction.
Can you please elaborate? All sysfs problems discovered by the rfcomm are fixed by the posted patches. Dave Young has a patch waiting for verification by the tester.

Umm... IIRC, there'd been a lot of fun with tty and procfs sides of that; will check.

Quoted 2 lines

Furthermore, even if we rip out
kobject_move() in the future, I don't think -rc7 is the right time to do it.

OK... You do have a point, but at this stage I'm not convinced that this thing is safe and usable. I agree that patches do not make things worse, but I suspect that the real problem with kobject_move() is that it's a fundamentally broken interface.

Tejun Heo

author

2008-01-16 06:47 UTC #6 lore raw

Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in sysfs_rename/move_dir()

In reply to Linus Torvalds

Quoted 5 lines · Linus Torvalds

Quoted 1 line · Tejun Heo
* sysfs_move_dir() has an extra dput() on success path.
Are you sure? How did this ever work?

I'm pretty sure. I've seen dentry blowing up due to early release && compared it with older code. It was my mistake during restructuring error path. The only user of sysfs_move_dir() was S390 Cornelia works on (cc'd). Cornelia is usually very good at spotting and debugging sysfs bugs. Dunno how it got slipped this time.

Quoted 2 lines

Also, looking at this, I think the "how did this ever work" question is answered by "it didn't",

Before dput() bug was introduced, it worked although error handling path was broken.

Quoted 10 lines

but I also think there are still serious problems
there. Look at

	again:
	        mutex_lock(&old_parent->d_inode->i_mutex);
	        if (!mutex_trylock(&new_parent->d_inode->i_mutex)) {
	                mutex_unlock(&old_parent->d_inode->i_mutex);
	                goto again;
	        }

Quoted 2 lines

and wonder what happen sif old_parent == new_parent. Is that trying to avoid an ABBA deadlock?

It will fall in infinite loop if old_parent == new_parent and for the question, I suppose so. Cornelia, right?

Quoted 3 lines

Normally you'd do it by ordering the locks, or by
taking a third lock to guarantee serialization at a higher level (ie the "s_vfs_rename_mutex" on the VFS layer)

sysfs currently doesn't depend on VFS locking. VFS locking is done just to keep VFS layer happy. sysfs_dirent hierarchy is protected by sysfs_mutex and renaming/moving are protected by sysfs_rename_mutex. As both ops are under rename_mutex, I think the above code just can grab both mutexes in any order. It's probably a remnant of the days when sysfs used VFS locking to protect internal structures.

s390 was the only user of the move interface till now and through all the recent sysfs change, it didn't receive enough attention other than Cornelia's testing. Eventually, I think sysfs_rename_dir() and sysfs_move_dir() should be merged into sysfs_move() but for the current two users, I don't see anything wrong with the locking.

Thanks.

Signature · 10 lines

tejun

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Cornelia Huck

2008-01-16 08:20 UTC #7 lore raw

Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in sysfs_rename/move_dir()

Quoted 5 lines · Tejun Heo

Quoted 4 lines · Linus Torvalds
Quoted 1 line · Tejun Heo
* sysfs_move_dir() has an extra dput() on success path.

Quoted 7 lines

Quoted 1 line
Are you sure? How did this ever work?
I'm pretty sure. I've seen dentry blowing up due to early release && compared it with older code. It was my mistake during restructuring error path. The only user of sysfs_move_dir() was S390 Cornelia works on (cc'd). Cornelia is usually very good at spotting and debugging sysfs bugs. Dunno how it got slipped this time.

Hm, don't know either. It never blew for my tests (and I did extensive moving around of ccw devices...)

Quoted 17 lines

Quoted 3 lines
Also, looking at this, I think the "how did this ever work" question is answered by "it didn't",
Before dput() bug was introduced, it worked although error handling path was broken.
Quoted 10 lines
but I also think there are still serious problems
there. Look at
	again:
	        mutex_lock(&old_parent->d_inode->i_mutex);
	        if (!mutex_trylock(&new_parent->d_inode->i_mutex)) {
	                mutex_unlock(&old_parent->d_inode->i_mutex);
	                goto again;
	        }

Quoted 5 lines

Quoted 2 lines
and wonder what happen sif old_parent == new_parent. Is that trying to avoid an ABBA deadlock?
It will fall in infinite loop if old_parent == new_parent and for the question, I suppose so. Cornelia, right?

Yes. But we jump out early if old_parent == new_parent.

Quoted 11 lines

Quoted 4 lines
Normally you'd do it by ordering the locks, or by
taking a third lock to guarantee serialization at a higher level (ie the "s_vfs_rename_mutex" on the VFS layer)
sysfs currently doesn't depend on VFS locking. VFS locking is done just to keep VFS layer happy. sysfs_dirent hierarchy is protected by sysfs_mutex and renaming/moving are protected by sysfs_rename_mutex. As both ops are under rename_mutex, I think the above code just can grab both mutexes in any order. It's probably a remnant of the days when sysfs used VFS locking to protect internal structures.

Yes, I think so.

Quoted 4 lines

s390 was the only user of the move interface till now and through all the recent sysfs change, it didn't receive enough attention other than Cornelia's testing.

I just wonder why I never hit it since I regularily do some testing of the moving stuff (on Greg's tree and on mainline). But bluetooth has been using this interface for some time as well (and they were the ones with the problems triggering this patch).

Quoted 3 lines

Eventually, I think sysfs_rename_dir() and
sysfs_move_dir() should be merged into sysfs_move() but for the current two users, I don't see anything wrong with the locking.

Me too. But maybe I'm just too familiar with the code...