linux-kernel - Re: Dcache oops

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]

Date:	Fri, 3 Jun 2016 19:58:37 -0400
From:	Oleg Drokin <green@...uxhacker.ru>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	"<linux-kernel@...r.kernel.org> Mailing List" 
	<linux-kernel@...r.kernel.org>,
	"<linux-fsdevel@...r.kernel.org>" <linux-fsdevel@...r.kernel.org>
Subject: Re: Dcache oops


On Jun 3, 2016, at 6:37 PM, Al Viro wrote:

> On Fri, Jun 03, 2016 at 11:23:55PM +0100, Al Viro wrote:
> 
>> It's not that.  It's explicit put_link() in do_last(), followed by
>> ESTALEOPEN and subsequent misbegotten "retry the last step on ESTALEOPEN"
>> looking at now-freed nd->last.name.  IOW, the bug predates delayed_call
>> stuff.
> 
> EOPENSTALE, that is...  Oleg, could you check if the following works?

Yes, this one lasted for an hour with no crashing, so it must be good.
Thanks.
(note, I am not equipped to verify correctness of NFS operations, though).

> diff --git a/fs/namei.c b/fs/namei.c
> index 4c4f95a..3d9511e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3166,9 +3166,7 @@ static int do_last(struct nameidata *nd,
> 	int acc_mode = op->acc_mode;
> 	unsigned seq;
> 	struct inode *inode;
> -	struct path save_parent = { .dentry = NULL, .mnt = NULL };
> 	struct path path;
> -	bool retried = false;
> 	int error;
> 
> 	nd->flags &= ~LOOKUP_PARENT;
> @@ -3211,7 +3209,6 @@ static int do_last(struct nameidata *nd,
> 			return -EISDIR;
> 	}
> 
> -retry_lookup:
> 	if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) {
> 		error = mnt_want_write(nd->path.mnt);
> 		if (!error)
> @@ -3292,23 +3289,14 @@ finish_lookup:
> 	if (unlikely(error))
> 		return error;
> 
> -	if ((nd->flags & LOOKUP_RCU) || nd->path.mnt != path.mnt) {
> -		path_to_nameidata(&path, nd);
> -	} else {
> -		save_parent.dentry = nd->path.dentry;
> -		save_parent.mnt = mntget(path.mnt);
> -		nd->path.dentry = path.dentry;
> -
> -	}
> +	path_to_nameidata(&path, nd);
> 	nd->inode = inode;
> 	nd->seq = seq;
> 	/* Why this, you ask?  _Now_ we might have grown LOOKUP_JUMPED... */
> finish_open:
> 	error = complete_walk(nd);
> -	if (error) {
> -		path_put(&save_parent);
> +	if (error)
> 		return error;
> -	}
> 	audit_inode(nd->name, nd->path.dentry, 0);
> 	error = -EISDIR;
> 	if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry))
> @@ -3331,13 +3319,9 @@ finish_open_created:
> 		goto out;
> 	BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */
> 	error = vfs_open(&nd->path, file, current_cred());
> -	if (!error) {
> -		*opened |= FILE_OPENED;
> -	} else {
> -		if (error == -EOPENSTALE)
> -			goto stale_open;
> +	if (error)
> 		goto out;
> -	}
> +	*opened |= FILE_OPENED;
> opened:
> 	error = open_check_o_direct(file);
> 	if (!error)
> @@ -3353,26 +3337,7 @@ out:
> 	}
> 	if (got_write)
> 		mnt_drop_write(nd->path.mnt);
> -	path_put(&save_parent);
> 	return error;
> -
> -stale_open:
> -	/* If no saved parent or already retried then can't retry */
> -	if (!save_parent.dentry || retried)
> -		goto out;
> -
> -	BUG_ON(save_parent.dentry != dir);
> -	path_put(&nd->path);
> -	nd->path = save_parent;
> -	nd->inode = dir->d_inode;
> -	save_parent.mnt = NULL;
> -	save_parent.dentry = NULL;
> -	if (got_write) {
> -		mnt_drop_write(nd->path.mnt);
> -		got_write = false;
> -	}
> -	retried = true;
> -	goto retry_lookup;
> }
> 
> static int do_tmpfile(struct nameidata *nd, unsigned flags,