linux-kernel - Re: [PATCH 0/9] [RFC v2] safely drop directory dentry on failed revalidate

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: <1377818677.2355.25.camel@perseus.fritz.box>
Date:	Fri, 30 Aug 2013 07:24:37 +0800
From:	Ian Kent <raven@...maw.net>
To:	Miklos Szeredi <mszeredi@...e.cz>
Cc:	Al Viro <viro@...IV.linux.org.uk>,
	Miklos Szeredi <miklos@...redi.hu>, rwheeler@...hat.com,
	avati@...hat.com, bfoster@...hat.com, dhowells@...hat.com,
	eparis@...hat.com, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	KONISHI Ryusuke <konishi.ryusuke@....ntt.co.jp>
Subject: Re: [PATCH 0/9] [RFC v2] safely drop directory dentry on failed
 revalidate

On Thu, 2013-08-29 at 05:51 +0200, Miklos Szeredi wrote:
> Ian,
> 
> I'm having problems fully understanding what autofs4 is trying to do
> with have_submounts().

OK, I don't really care how I do it so I'm happy to change.

> 
> 
> > On Wed, 2013-08-21 at 06:40 +0100, Al Viro wrote:
> 
> > fs/autofs4/dev-ioctl.c:542:             err = have_submounts(path.dentry);
> 
> This is an ioctl() asking whether we have anything mounted on the autofs
> mount.  Using have_submounts() and then a separate follow_down() looks
> racy.  have_submounts() could succeed and then follow_down() could fail.
> Or the other way round.  Shouldn't the two cases be handled separately
> here?  If the autofs is a just a simple trigger then use follow_down().
> If it's a multi-mount thing, then use have_submounts().

Right but IIRC I don't think I actually use the returned s_magic ATM but
I use the return of have_submounts() a lot.

> 
> What is the userspace automount daemon using this for?  Do we really
> need the recursive check for submounts?
> 
> 
> > fs/autofs4/root.c:381:                  if (have_submounts(dentry)) {
> 
> Here it explicitly says it's for v5 and for rootless mutli-mount.  So
> for example:
> 
> /mnt/auto/            root of an indirect mount

or the root of direct mount for that matter.

> /mnt/auto/foo         directory with DCACHE_NEED_AUTOMOUNT
> /mnt/auto/foo/bar     directory without DCACHE_NEED_AUTOMOUNT
> /mnt/auto/foo/bar/baz directory with an automount trigger mounted on it
> 
> In this case when d_automount for "foo" is called we don't call the
> userspace daemon because things are mounted under foo.  If there was no
> trigger under baz, then we would try to handle "foo" as an indirect
> mount and call userspace.
> 
> But it's pretty confusing.  Do we really *ever* need to call automount
> on "foo" if it was part of a multi-mount thing? 

That's right, the directory isn't simple_empty() so there's no callback.

The problem is we can't just use the fact that the directory is empty to
determine that there are no mounts at all underneath.

I understand your thinking, about deciding whether to callback to the
daemon, but that's not what the ioctl above is used for.

The main use is to be able to find out if the given directory is a
mountpoint as defined by the description in the comment above the
function. This saves having to scan the mount table to find out and is a
huge saving on systems with lots of mounts. In the past I've often
needed an answer the question "is this an autofs mount or some other
type" and that's why I stick s_magic in the return as well.

> 
> > fs/autofs4/waitq.c:338:         if (have_submounts(dentry))
> 
> And here we re-validate the thing after taking another autofs4 lock.
> Why this double checking?

This is a different case and is often not in play at times when autofs
is checking if the directory is a "mountpoint". Such as when trying to
re-construct a tree of mounts at startup.

The check in waitq.c above "is" used to validate the need to callback to
the daemon to request a mount.

As I said, any suggestions how to achieve this without calling
have_submounts() are welcome.

Ian

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