linux-kernel - Re: [PATCH 08/41] Don't replace nameidata path when following links

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: <200911300244.nAU2i41s007505@agora.fsl.cs.sunysb.edu>
Date:	Sun, 29 Nov 2009 21:44:04 -0500
From:	Erez Zadok <ezk@...sunysb.edu>
To:	Valerie Aurora <vaurora@...hat.com>
Cc:	Jan Blunck <jblunck@...e.de>, Ian Kent <raven@...maw.net>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christoph Hellwig <hch@...radead.org>,
	Andy Whitcroft <apw@...onical.com>,
	Scott James Remnant <scott@...onical.com>,
	Sandu Popa Marius <sandupopamarius@...il.com>,
	Jan Rekorajski <baggins@...h.mimuw.edu.pl>,
	"J. R. Okajima" <hooanon05@...oo.co.jp>,
	Arnd Bergmann <arnd@...db.de>,
	Vladimir Dronnikov <dronnikov@...il.com>,
	Felix Fietkau <nbd@...nwrt.org>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jan Blunck <jblunck@...radead.org>
Subject: Re: [PATCH 08/41] Don't replace nameidata path when following links 

In message <1256152779-10054-9-git-send-email-vaurora@...hat.com>, Valerie Aurora writes:
> From: Jan Blunck <jblunck@...radead.org>
> 
> For autofs4 the commit 051d381259eb57d6074d02a6ba6e90e744f1a29f introduced
> some code that is replacing the path embedded in the nameidata with the
> path of the link itself. This was done to have access to the struct
> vfsmount in the autofs4_follow_link function. Instead autofs4 should
> remember the struct vfsmount when it is mounted.

This is an autofs4 patch, mainly: say so in the subject line:

     VFS/Autofs4: don't replace nameidata ...

I'm curious why wasn't Ian Kent CC'ed on this patch originally?  I added him
to the CC list now.

And what does this patch have to do with union mounts?  Can you document why
you needed this change made?

Lastly, if this patch is acceptable to all parties, then it should be pushed
to the autofs4 maintainers and hopefully upstream well before UM.

> ---
>  fs/autofs4/autofs_i.h |    1 +
>  fs/autofs4/init.c     |   11 ++++++++++-
>  fs/autofs4/root.c     |    6 ++++++
>  fs/namei.c            |    7 ++-----
>  4 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index 8f7cdde..db2bfce 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -130,6 +130,7 @@ struct autofs_sb_info {
>  	int reghost_enabled;
>  	int needs_reghost;
>  	struct super_block *sb;
> +	struct vfsmount *mnt;
>  	struct mutex wq_mutex;
>  	spinlock_t fs_lock;
>  	struct autofs_wait_queue *queues; /* Wait queue pointer */
> diff --git a/fs/autofs4/init.c b/fs/autofs4/init.c
> index 9722e4b..5e0dcd7 100644
> --- a/fs/autofs4/init.c
> +++ b/fs/autofs4/init.c
> @@ -17,7 +17,16 @@
>  static int autofs_get_sb(struct file_system_type *fs_type,
>  	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
>  {
> -	return get_sb_nodev(fs_type, flags, data, autofs4_fill_super, mnt);
> +	struct autofs_sb_info *sbi;
> +	int ret;
> +
> +	ret = get_sb_nodev(fs_type, flags, data, autofs4_fill_super, mnt);
> +	if (ret)
> +		return ret;
> +
> +	sbi = autofs4_sbi(mnt->mnt_sb);
> +	sbi->mnt = mnt;
> +	return 0;
>  }
>  
>  static struct file_system_type autofs_fs_type = {
> diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> index b96a3c5..cb991b8 100644
> --- a/fs/autofs4/root.c
> +++ b/fs/autofs4/root.c
> @@ -179,6 +179,12 @@ static void *autofs4_follow_link(struct dentry *dentry, struct nameidata *nd)
>  	DPRINTK("dentry=%p %.*s oz_mode=%d nd->flags=%d",
>  		dentry, dentry->d_name.len, dentry->d_name.name, oz_mode,
>  		nd->flags);
> +
> +	dput(nd->path.dentry);
> +	mntput(nd->path.mnt);
> +	nd->path.mnt = mntget(sbi->mnt);
> +	nd->path.dentry = dget(dentry);
> +
>  	/*
>  	 * For an expire of a covered direct or offset mount we need
>  	 * to break out of follow_down() at the autofs mount trigger
> diff --git a/fs/namei.c b/fs/namei.c
> index a338496..46cf1cb 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -636,11 +636,8 @@ static __always_inline int __do_follow_link(struct path *path, struct nameidata
>  	touch_atime(path->mnt, dentry);
>  	nd_set_link(nd, NULL);
>  
> -	if (path->mnt != nd->path.mnt) {
> -		path_to_nameidata(path, nd);
> -		dget(dentry);
> -	}
> -	mntget(path->mnt);
> +	if (path->mnt == nd->path.mnt)
> +		mntget(nd->path.mnt);
>  	cookie = dentry->d_inode->i_op->follow_link(dentry, nd);
>  	error = PTR_ERR(cookie);
>  	if (!IS_ERR(cookie)) {

Just want to mention that the five lines you replace with the two lines, in
the above patch snippet, are not functionally equivalent.  Is this the
intention of "reversing" what commit
051d381259eb57d6074d02a6ba6e90e744f1a29f introduced?  If not, then please
explain the change in __do_follow_link.

Thanks,
Erez.
--
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/