linux-kernel - Re: [PATCH] EXPORTFS: Don't return NULL from fh_to_dentry()/fh_to

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: <20081205174228.GC29227@fieldses.org>
Date:	Fri, 5 Dec 2008 12:42:28 -0500
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	David Howells <dhowells@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>, hugh@...itas.com,
	hch@...radead.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] EXPORTFS: Don't return NULL from
	fh_to_dentry()/fh_to_parent() [ver #4]

On Fri, Dec 05, 2008 at 08:52:13AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 5 Dec 2008, David Howells wrote:
> >
> > Returning NULL from fh_to_dentry() and fh_to_parent() is not permitted, so
> > return -ESTALE instead.  exportfs_decode_fh() does not check for NULL, but
> > will try to dereference it as IS_ERR() does not check for it.
> > 
> > The generic_fh_to_dentry() and generic_fh_to_parent() functions also no longer
> > return NULL, but return -ESTALE instead.
> 
> I really don't like this one.
> 
> So just about every fh_to_dentry() returns NULL for some error case. 
> Instead of considering that a problem, maybe it should tell us something.
> 
> There is _one_ single caller of that function afaik, why shouldn't we just 
> teach the caller that dammit, lots of people want to just return NULL.
> 
> It's not like returning NULL doesn't make sense. Quite frankly, I think it 
> makes a lot more sense than returning -ESTALE, which is a very unnatural 
> error for most filesystems.
> 
> So quite frankly, I'd much rather just see the simpler patch. Have 
> exportfs handle the case.
>
> Are there other callers?

Not of fh_to_dentry(), but it looks like fh_to_parent() is the same.  So
you'd want the below.

This is partially reverting 440037287c5, which appears to have also do
some changing of NULLs to ESTALEs which we might also want to reconsider
at some point.  Maybe Christoph could explain what the rationale was.

--b.

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 80246ba..890e018 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -367,6 +367,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 	 * Try to get any dentry for the given file handle from the filesystem.
 	 */
 	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
+	if (!result)
+		result = ERR_PTR(-ESTALE);
 	if (IS_ERR(result))
 		return result;
 
@@ -420,6 +422,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 
 		target_dir = nop->fh_to_parent(mnt->mnt_sb, fid,
 				fh_len, fileid_type);
+		if (!target_dir)
+			goto err_result;
 		err = PTR_ERR(target_dir);
 		if (IS_ERR(target_dir))
 			goto err_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/