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 <htejun@gmail.com>
---
 fs/sysfs/dir.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: work/fs/sysfs/dir.c
===================================================================
--- work.orig/fs/sysfs/dir.c
+++ 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);
--

[ message continues ]

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
--

[ message continues ]

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.
--

[ message continues ]

Hello.


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

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.

-- 
tejun

--

[ message continues ]

How about "what happens after that move-to-NULL if you have a cwd inside

No, but I'd rather see the rules for callers of sysfs/kobject primitives

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

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.
--

[ message continues ]

On Wed, 16 Jan 2008 15:47:11 +0900,

Hm, don't know either. It never blew for my tests (and I did extensive



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

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

[ message continues ]