linux-kernel - Re: [PATCH] vfs: make real_lookup do dentry revalidation with i

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: <Pine.LNX.4.64.0903170957560.18956@cobra.newdream.net>
Date:	Tue, 17 Mar 2009 10:03:35 -0700 (PDT)
From:	Sage Weil <sage@...dream.net>
To:	Christoph Hellwig <hch@...radead.org>
cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, viro@...iv.linux.org.uk, adilger@....com
Subject: Re: [PATCH] vfs: make real_lookup do dentry revalidation with i_mutex
 held

On Tue, 17 Mar 2009, Christoph Hellwig wrote:
> Keeping i_mutes over do_revalidate seem fine from a first glance, but
> can you please do it without rearranging the whole code?

Yeah, but not without an extra goto.  Holding i_mutex over revalidate is 
only half of it... we also want to go ahead with the ->lookup if the 
revalidate fails (instead of returning -ENOENT).  I make the patch easier 
to read (with a goto), but I assumed we'd want the resulting code to be 
more clear?

FWIW, here's the patched result:

	result = d_lookup(parent, name);
	if (result) {
		/*
		 * The cache was re-populated while we waited on the
		 * mutex.  We need to revalidate, this time while
		 * holding i_mutex (to avoid another race).
		 */
		if (result->d_op && result->d_op->d_revalidate) {
			result = do_revalidate(result, nd);
			if (result)
				goto out_unlock;
			/*
			 * The dentry was left behind invalid.  Just
			 * do the lookup.
			 */
		} else {
			goto out_unlock;
		}
	}

	/* Don't create child dentry for a dead directory. */
	result = ERR_PTR(-ENOENT);
	if (IS_DEADDIR(dir))
		goto out_unlock;

	dentry = d_alloc(parent, name);
	result = ERR_PTR(-ENOMEM);
	if (dentry) {
		result = dir->i_op->lookup(dir, dentry, nd);
		if (result)
			dput(dentry);
		else
			result = dentry;
	}
out_unlock:

Let me know!
sage


> Something like the tiny untested patch below should archive the same
> thing:
> 
> 
> Index: linux-2.6/fs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/namei.c	2009-03-17 09:15:53.430978739 +0100
> +++ linux-2.6/fs/namei.c	2009-03-17 09:16:19.553981306 +0100
> @@ -512,12 +512,12 @@ out_unlock:
>  	 * Uhhuh! Nasty case: the cache was re-populated while
>  	 * we waited on the semaphore. Need to revalidate.
>  	 */
> -	mutex_unlock(&dir->i_mutex);
>  	if (result->d_op && result->d_op->d_revalidate) {
>  		result = do_revalidate(result, nd);
>  		if (!result)
>  			result = ERR_PTR(-ENOENT);
>  	}
> +	mutex_unlock(&dir->i_mutex);
>  	return result;
>  }
>  
> 
> 
--
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/