linux-kernel - Re: [PATCH 2/2] configfs: Rework configfs_depend

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]

Message-ID: <20090430172014.GA2762@mail.oracle.com>
Date:	Thu, 30 Apr 2009 10:20:14 -0700
From:	Joel Becker <Joel.Becker@...cle.com>
To:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	cluster-devel@...hat.com, swhiteho@...hat.com, peterz@...radead.org
Subject: Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking
	and make lockdep happy

On Thu, Apr 30, 2009 at 11:18:28AM +0200, Louis Rilling wrote:
> On 29/04/09 11:52 -0700, Joel Becker wrote:
> > On Wed, Jan 28, 2009 at 07:18:33PM +0100, Louis Rilling wrote:
> > > configfs_depend_item() recursively locks all inodes mutex from configfs root to
> > > the target item, which makes lockdep unhappy. The purpose of this recursive
> > > locking is to ensure that the item tree can be safely parsed and that the target
> > > item, if found, is not about to leave.
> > > 
> > > This patch reworks configfs_depend_item() locking using configfs_dirent_lock.
> > > Since configfs_dirent_lock protects all changes to the configfs_dirent tree, and
> > > protects tagging of items to be removed, this lock can be used instead of the
> > > inodes mutex lock chain.
> > > This needs that the check for dependents be done atomically with
> > > CONFIGFS_USET_DROPPING tagging.
> > 
> > 	These patches are now in the 'lockdep' branch of the configfs
> > tree.  I'm planning to send them in the next merge window.  I've made
> > one change.
> > 
> > > + * Note: items in the middle of attachment start with s_type = 0
> > > + * (configfs_new_dirent()), and configfs_make_dirent() (called from
> > > + * create_dir()) sets s_type = CONFIGFS_DIR|CONFIGFS_USET_CREATING. In both
> > > + * cases the item is ignored. Since s_type is an int, we rely on the CPU to
> > > + * atomically update the value, without making configfs_make_dirent() take
> > > + * configfs_dirent_lock.
> > 
> > 	I've added configfs_dirent_lock in configfs_make_dirent(),
> > because it is not safe at all to rely on the fact that s_type is an int.
> > It's an atomic set on one CPU, but there's no guarantee that it's seen
> > correctly on other CPUs.  Plus, there's no real need for speed here.  So
> > we properly take configfs_dirent_lock around s_type in
> > configfs_make_dirent(), and that ensures we see things correctly on SMP.
> 
> Agreed, I was going to suggest something like this. Actually I'd push the
> initialization of s_type down to configfs_new_dirent(), so that s_type either
> is always NULL, or always shows the correct type of object.

	0, not "NULL", but yeah I think that's a good plan.

Joel

-- 

"If at first you don't succeed, cover all traces that you tried."
                                                        -Unknown

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@...cle.com
Phone: (650) 506-8127
--
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/