Message-ID: <m1eij1rued.fsf@fess.ebiederm.org>
Date:	Tue, 30 Mar 2010 21:23:54 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	"Serge E. Hallyn" <serue@...ibm.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.

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

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