[WTFoTW] ->quota_on() deadlocks

[WTFoTW] ->quota_on() deadlocks

[Al Viro]

	All quotactl callbacks are done with s_umount held shared.
Fine, but ->quota_on() will do kern_path() and _that_ can try to
grab the same thing exclusive - suppose we pass a pathname that
walks into autofs and triggers mounting of the same fs (at a different
mountpoint, that is).  That'll end up calling sget(), finding our
superblock and trying to grab s_umount on it.  mount(8) sits
uninterruptibly sleeping in mount(2), kern_path() waits for it
to complete and that's not going to happen until the caller of
kern_path() (do_quotactl(), ultimately) finishes.

	Obvious solution is b0rken - we _can't_ take the call of
kern_path() to a point prior to getting (and locking) the superblock.
Why?  Because ocfs2 ignores the pathname argument, so failing on
bogus pathnames will blow the userland API compatibility.

	Other alternatives are also not particulary pleasant since
we need s_umount at some point there - we want some exclusion with
remounting.

	Ideas?

Re: [WTFoTW] ->quota_on() deadlocks

[Christoph Hellwig]

I have patches in my queue that should fix this as a side effect.

First thing is splitting the set_xstate method currently only
used by XFS into ->quota_enable, ->quota_disable and ->quota_remove,
second is using merging ->quota_disable and ->quota_off, third
is switching ocfs2 to ->quota_disable and fourth is taking the
path lookup from ->quota_on into the caller.

I'll try to get them out to the list in a couple of days.

Re: [WTFoTW] ->quota_on() deadlocks

[Christoph Hellwig]

On Fri, Jul 02, 2010 at 12:38:22AM +0200, Christoph Hellwig wrote:
> I have patches in my queue that should fix this as a side effect.
> 
> First thing is splitting the set_xstate method currently only
> used by XFS into ->quota_enable, ->quota_disable and ->quota_remove,
> second is using merging ->quota_disable and ->quota_off, third
> is switching ocfs2 to ->quota_disable and fourth is taking the
> path lookup from ->quota_on into the caller.
> 
> I'll try to get them out to the list in a couple of days.

Shouldn't have replied in the middle of the night.  The above doesn't
fix the deadlock yet, but by allowing different methods would allow to
fix it in hacky ways.

Now the real questions is: do we need to support quota files on
different filesystems?  For ext3/4 and reiserfs with journaled quota
we already reject that.  If we always reject it or even further restrict
quota files to sit in the root of the filesystem we can replace the
kernel_path with a relative lookup that doesn't cross mountpoints.
In the idea case of restricting it to the root that would be just a
lookup_one_len, just as we do in dquot_quota_on_mount, otherwise
something hand crafted.

Re: [WTFoTW] ->quota_on() deadlocks

[Jan Kara]

On Thu 01-07-10 19:56:29, Al Viro wrote:
> 	All quotactl callbacks are done with s_umount held shared.
> Fine, but ->quota_on() will do kern_path() and _that_ can try to
> grab the same thing exclusive - suppose we pass a pathname that
> walks into autofs and triggers mounting of the same fs (at a different
> mountpoint, that is).  That'll end up calling sget(), finding our
> superblock and trying to grab s_umount on it.  mount(8) sits
> uninterruptibly sleeping in mount(2), kern_path() waits for it
> to complete and that's not going to happen until the caller of
> kern_path() (do_quotactl(), ultimately) finishes.
> 
> 	Obvious solution is b0rken - we _can't_ take the call of
> kern_path() to a point prior to getting (and locking) the superblock.
> Why?  Because ocfs2 ignores the pathname argument, so failing on
> bogus pathnames will blow the userland API compatibility.
> 
> 	Other alternatives are also not particulary pleasant since
> we need s_umount at some point there - we want some exclusion with
> remounting.
> 
> 	Ideas?
  Not now :(. I'm on vacation this and next week. I'll look into the bug when
I return.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR