Date:	Tue, 30 Mar 2010 23:53:11 -0500
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>,
	Kay Sievers <kay.sievers@...y.org>,
	linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>,
	Cornelia Huck <cornelia.huck@...ibm.com>,
	linux-fsdevel@...r.kernel.org,
	Eric Dumazet <eric.dumazet@...il.com>,
	Benjamin LaHaise <bcrl@...et.ca>, netdev@...r.kernel.org,
	Benjamin Thery <benjamin.thery@...l.net>
Subject: Re: [PATCH 3/6] sysfs: Implement sysfs tagged directory support.

Quoting Eric W. Biederman (ebiederm@...ssion.com):
> "Serge E. Hallyn" <serue@...ibm.com> writes:
> 
> >> > This is a huge patch, and for the most part I haven't found any problems,
> >> > except potentially this one.  It looks like sysfs_rename_link() checks
> >> > old_ns and new_ns before calling sysfs_rename().  But sysfs_mutex isn't
> >> > taken until sysfs_rename().  sysfs_rename() will then proceed to do
> >> > the rename, and unconditionally set sd->ns = new_ns.
> >> >
> >> > In the meantime, it seems as though new_ns might have exited, and
> >> > sysfs_exit_ns() unset new_ns on the new parent dir.  This means that
> >> > we'll end up with the namespace code having thought that it cleared
> >> > all new_ns's, but this file will have snuck by.  Meaning an action on
> >> > the renamed file might dereference a freed namespace.
> >> >
> >> > Or am I way off base?
> >> 
> >> There are a couple of reasons why this is not a concern.
> >> 
> >> The only new_ns we clear is on the super block.
> >
> > Oops, yeah - I failed to note that.
> >
> >> sysfs itself never dereferences namespace arguments and only uses them
> >> for comparison purposes.  They are just cookies that cause comparisons
> >> to differ from a sysfs perspective.
> >> 
> >> The upper levels are responsible for taking care of them selves
> >> sysfs_mutex does not protect them.  If you compile out sysfs the sysfs
> >> mutex is not even present.
> >> 
> >> In the worst case if the upper levels mess up we will have a stale
> >> token that we never dereference on a sysfs dirent, which in a pathological
> >> case will happen to be the same as a new namespace and we will have
> >> a spurious directory entry that we have leaked.
> >> 
> >> In practice we move all network devices (and thus sysfs files) out of
> >> a network namespace before allowing it to exit.
> >
> > Ok, that makes sense too - so any tagged sysfs file created for some object
> > in a ns must be deleted at netns exit.  I could imagine someone expecting
> > that if the ns exits, the tasks in the ns will exit, causing the sysfs
> > mount to be umounted and auto-deleting the files?  (which of course would
> > get buggered if task in other ns was examining the mount which it got
> > through mounts propagation)  We'll have to make sure noone does that.  Should
> > it be documented somewhere, or is that obvious enough?
> 
> In general it is simply true.  An object in a namespace either keeps
> the namespace alive, or it is destroyed when the namespace exits
> because the object is unreachable.

I guess you'd hope so :)

> So the only possible problem I can think of is of ordering the object
> destruction and calling sysfs_exit_ns.    So for the moment I am going
> to vote that this is simply obvious enough not to worry about in detail.
> 
> It is also pretty obvious if you trace the code and ask how does sysfs
> dirent X get destroyed.
> 
> Today there is just a wee bit of automatic file destruction at the sysfs
> level.    The device layer does not take advantage of it, and in hierarchical
> situation it leads to bugs.  So even I think if we document anything it
> should be that sysfs can not safely automatically delete anything, for
> you.
> 
> Eric

Ok.  I'm convinced.

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists