EXPORTFS: Don't return NULL from fh_to_dentry()/fh_to_parent() [ver #4]

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.

Signed-off-by David Howells <[email protected]>

Acked-by J. Bruce Fields <[email protected]>

---

 fs/fat/inode.c                |    2 +-
 fs/fuse/inode.c               |    4 ++--
 fs/gfs2/ops_export.c          |    4 ++--
 fs/isofs/export.c             |    4 ++--
 fs/libfs.c                    |    4 ++--
 fs/ocfs2/export.c             |    4 ++--
 fs/udf/namei.c                |    4 ++--
 fs/xfs/linux-2.6/xfs_export.c |    2 +-
 mm/shmem.c                    |   14 +++++++-------
 9 files changed, 21 insertions(+), 21 deletions(-)

fs/fat/inode.c +1 -1

index d937aaf..cac84f2 100644
@@ -657,7 +657,7 @@ static struct dentry *fat_fh_to_dentry(struct super_block *sb,
 	u32 *fh = fid->raw;
 
 	if (fh_len < 5 || fh_type != 3)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	inode = ilookup(sb, fh[0]);
 	if (!inode || inode->i_generation != fh[1]) {

fs/fuse/inode.c +2 -2

index 2e99f34..0eb8990 100644
@@ -653,7 +653,7 @@ static struct dentry *fuse_fh_to_dentry(struct super_block *sb,
 	struct fuse_inode_handle handle;
 
 	if ((fh_type != 0x81 && fh_type != 0x82) || fh_len < 3)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	handle.nodeid = (u64) fid->raw[0] << 32;
 	handle.nodeid |= (u64) fid->raw[1];

@@ -667,7 +667,7 @@ static struct dentry *fuse_fh_to_parent(struct super_block *sb,
 	struct fuse_inode_handle parent;
 
 	if (fh_type != 0x82 || fh_len < 6)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	parent.nodeid = (u64) fid->raw[3] << 32;
 	parent.nodeid |= (u64) fid->raw[4];

fs/gfs2/ops_export.c +2 -2

index bbb8c36..2864873 100644
@@ -254,7 +254,7 @@ static struct dentry *gfs2_fh_to_dentry(struct super_block *sb, struct fid *fid,
 		this.no_addr |= be32_to_cpu(fh[3]);
 		return gfs2_get_dentry(sb, &this);
 	default:
-		return NULL;
+		return ERR_PTR(-ESTALE);
 	}
 }
 

@@ -273,7 +273,7 @@ static struct dentry *gfs2_fh_to_parent(struct super_block *sb, struct fid *fid,
 		parent.no_addr |= be32_to_cpu(fh[7]);
 		return gfs2_get_dentry(sb, &parent);
 	default:
-		return NULL;
+		return ERR_PTR(-ESTALE);
 	}
 }
 

fs/isofs/export.c +2 -2

index e81a305..1b4ee23 100644
@@ -164,7 +164,7 @@ static struct dentry *isofs_fh_to_dentry(struct super_block *sb,
 	struct isofs_fid *ifid = (struct isofs_fid *)fid;
 
 	if (fh_len < 3 || fh_type > 2)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	return isofs_export_iget(sb, ifid->block, ifid->offset,
 			ifid->generation);

@@ -176,7 +176,7 @@ static struct dentry *isofs_fh_to_parent(struct super_block *sb,
 	struct isofs_fid *ifid = (struct isofs_fid *)fid;
 
 	if (fh_type != 2)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	return isofs_export_iget(sb,
 			fh_len > 2 ? ifid->parent_block : 0,

fs/libfs.c +2 -2

index e960a83..4fe5b5b 100644
@@ -751,7 +751,7 @@ struct dentry *generic_fh_to_dentry(struct super_block *sb, struct fid *fid,
 	struct inode *inode = NULL;
 
 	if (fh_len < 2)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	switch (fh_type) {
 	case FILEID_INO32_GEN:

@@ -784,7 +784,7 @@ struct dentry *generic_fh_to_parent(struct super_block *sb, struct fid *fid,
 	struct inode *inode = NULL;
 
 	if (fh_len <= 2)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	switch (fh_type) {
 	case FILEID_INO32_GEN_PARENT:

fs/ocfs2/export.c +2 -2

index 2f27b33..0f1c80f 100644
@@ -182,7 +182,7 @@ static struct dentry *ocfs2_fh_to_dentry(struct super_block *sb,
 	struct ocfs2_inode_handle handle;
 
 	if (fh_len < 3 || fh_type > 2)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	handle.ih_blkno = (u64)le32_to_cpu(fid->raw[0]) << 32;
 	handle.ih_blkno |= (u64)le32_to_cpu(fid->raw[1]);

@@ -196,7 +196,7 @@ static struct dentry *ocfs2_fh_to_parent(struct super_block *sb,
 	struct ocfs2_inode_handle parent;
 
 	if (fh_type != 2 || fh_len < 6)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	parent.ih_blkno = (u64)le32_to_cpu(fid->raw[3]) << 32;
 	parent.ih_blkno |= (u64)le32_to_cpu(fid->raw[4]);

fs/udf/namei.c +2 -2

index f84bfaa..e1a1c16 100644
@@ -1297,7 +1297,7 @@ static struct dentry *udf_fh_to_dentry(struct super_block *sb,
 	if ((fh_len != 3 && fh_len != 5) ||
 	    (fh_type != FILEID_UDF_WITH_PARENT &&
 	     fh_type != FILEID_UDF_WITHOUT_PARENT))
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	return udf_nfs_get_inode(sb, fid->udf.block, fid->udf.partref,
 			fid->udf.generation);

@@ -1307,7 +1307,7 @@ static struct dentry *udf_fh_to_parent(struct super_block *sb,
 				       struct fid *fid, int fh_len, int fh_type)
 {
 	if (fh_len != 5 || fh_type != FILEID_UDF_WITH_PARENT)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	return udf_nfs_get_inode(sb, fid->udf.parent_block,
 				 fid->udf.parent_partref,

fs/xfs/linux-2.6/xfs_export.c +1 -1

index 7f7abec..f9a51c4 100644
@@ -150,7 +150,7 @@ xfs_fs_fh_to_dentry(struct super_block *sb, struct fid *fid,
 	struct inode		*inode = NULL;
 
 	if (fh_len < xfs_fileid_length(fileid_type))
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	switch (fileid_type) {
 	case FILEID_INO32_GEN_PARENT:

mm/shmem.c +7 -7

index f1b0d48..c9bdbf7 100644
@@ -2050,21 +2050,21 @@ static struct dentry *shmem_fh_to_dentry(struct super_block *sb,
 		struct fid *fid, int fh_len, int fh_type)
 {
 	struct inode *inode;
-	struct dentry *dentry = NULL;
+	struct dentry *dentry;
 	u64 inum = fid->raw[2];
 	inum = (inum << 32) | fid->raw[1];
 
 	if (fh_len < 3)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 
 	inode = ilookup5(sb, (unsigned long)(inum + fid->raw[0]),
 			shmem_match, fid->raw);
-	if (inode) {
-		dentry = d_find_alias(inode);
-		iput(inode);
-	}
+	if (!inode)
+		return ERR_PTR(-ESTALE);
 
-	return dentry;
+	dentry = d_find_alias(inode);
+	iput(inode);
+	return dentry ?: ERR_PTR(-ESTALE);
 }
 
 static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len,

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.

Quoted 7 lines · David Howells

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.

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.

The d_obtain_alias() function will immediately return -ESTALE if given a NULL inode, though, and sometimes it'll return some other error.

It would also seem odd to sometimes return NULL to indicate an error, and sometimes return a -ve error code to indicate an error. Perhaps one or the other should be selected for consistency.

So it really seems to make more sense to just make the ESTALE handling be a NFS issue.