linux-kernel - Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in sysfs_rename/move

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 16 Jan 2008 09:20:11 +0100
From:	Cornelia Huck <cornelia.huck@...ibm.com>
To:	Tejun Heo <htejun@...il.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Al Viro <viro@...IV.linux.org.uk>,
	Gabor Gombas <gombasg@...aki.hu>, Greg KH <greg@...ah.com>,
	Dave Young <hidave.darkstar@...il.com>,
	bluez-devel@...ts.sourceforge.net
Subject: Re: [PATCH 2.6.24-rc7 2/2] sysfs: fix bugs in
 sysfs_rename/move_dir()

On Wed, 16 Jan 2008 15:47:11 +0900,
Tejun Heo <htejun@...il.com> wrote:

> Linus Torvalds wrote:
> > 
> > On Wed, 16 Jan 2008, Tejun Heo wrote:
> >> * 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.

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

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

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

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

> 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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/