[PATCH] prevent NULL returns from d_obtain_alias

[PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

As Miklos pointed out a while ago returning NULL from the export
operations and thus d_obtain_alias is not a good idea - the callers
in exportfs convert it to an -ESTALE anyway (or as the audit pointed
out don't deal with it at all in case of ->get_parent), and possibly
returning either NULL or an ERR_PTR value form one function is
confusing.  By auditing the callers I also found various other places
that couldn't deal with the NULL return.  I guess an unlikely error
path of an unlikely pass like this simply doesn't get tested.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/dcache.c
===================================================================
--- vfs-2.6.orig/fs/dcache.c	2008-10-11 04:42:11.000000000 -0400
+++ vfs-2.6/fs/dcache.c	2008-10-11 04:52:45.000000000 -0400
@@ -1123,10 +1123,10 @@ static inline struct hlist_head *d_hash(
  * allocating a new one.
  *
  * On successful return, the reference to the inode has been transferred
- * to the dentry.  If %NULL is returned (indicating kmalloc failure),
- * the reference on the inode has been released.  To make it easier
- * to use in export operations a NULL or IS_ERR inode may be passed in
- * and will be casted to the corresponding NULL or IS_ERR dentry.
+ * to the dentry.  In case of an error the reference on the inode is released.
+ * To make it easier to use in export operations a %NULL or IS_ERR inode may
+ * be passed in and will be the error will be propagate to the return value,
+ * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
  */
 struct dentry *d_obtain_alias(struct inode *inode)
 {
@@ -1135,7 +1135,7 @@ struct dentry *d_obtain_alias(struct ino
 	struct dentry *res;
 
 	if (!inode)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
Index: vfs-2.6/fs/exportfs/expfs.c
===================================================================
--- vfs-2.6.orig/fs/exportfs/expfs.c	2008-10-11 04:52:51.000000000 -0400
+++ vfs-2.6/fs/exportfs/expfs.c	2008-10-11 04:53:49.000000000 -0400
@@ -367,8 +367,6 @@ struct dentry *exportfs_decode_fh(struct
 	 * 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;
 
@@ -422,8 +420,6 @@ struct dentry *exportfs_decode_fh(struct
 
 		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;
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2008-10-11 04:48:35.000000000 -0400
+++ vfs-2.6/fs/fat/inode.c	2008-10-11 04:50:10.000000000 -0400
@@ -697,7 +697,7 @@ static struct dentry *fat_fh_to_dentry(s
 	 * of luck.  But all that is for another day
 	 */
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = sb->s_root->d_op;
 	return result;
 }
Index: vfs-2.6/fs/fuse/inode.c
===================================================================
--- vfs-2.6.orig/fs/fuse/inode.c	2008-10-11 04:41:20.000000000 -0400
+++ vfs-2.6/fs/fuse/inode.c	2008-10-11 04:50:25.000000000 -0400
@@ -597,7 +597,7 @@ static struct dentry *fuse_get_dentry(st
 		goto out_iput;
 
 	entry = d_obtain_alias(inode);
-	if (entry && !IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
 		entry->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(entry);
 	}
@@ -699,7 +699,7 @@ static struct dentry *fuse_get_parent(st
 	}
 
 	parent = d_obtain_alias(inode);
-	if (parent && !IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
 		parent->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(parent);
 	}
Index: vfs-2.6/fs/gfs2/ops_export.c
===================================================================
--- vfs-2.6.orig/fs/gfs2/ops_export.c	2008-10-11 04:48:35.000000000 -0400
+++ vfs-2.6/fs/gfs2/ops_export.c	2008-10-11 04:50:44.000000000 -0400
@@ -139,7 +139,7 @@ static struct dentry *gfs2_get_parent(st
 	gfs2_str2qstr(&dotdot, "..");
 
 	dentry = d_obtain_alias(gfs2_lookupi(child->d_inode, &dotdot, 1));
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 }
@@ -223,7 +223,7 @@ static struct dentry *gfs2_get_dentry(st
 
 out_inode:
 	dentry = d_obtain_alias(inode);
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 
Index: vfs-2.6/fs/nfs/getroot.c
===================================================================
--- vfs-2.6.orig/fs/nfs/getroot.c	2008-10-11 04:48:36.000000000 -0400
+++ vfs-2.6/fs/nfs/getroot.c	2008-10-11 04:51:44.000000000 -0400
@@ -108,9 +108,9 @@ struct dentry *nfs_get_root(struct super
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
@@ -277,9 +277,9 @@ struct dentry *nfs4_get_root(struct supe
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
Index: vfs-2.6/fs/ocfs2/export.c
===================================================================
--- vfs-2.6.orig/fs/ocfs2/export.c	2008-10-11 04:48:36.000000000 -0400
+++ vfs-2.6/fs/ocfs2/export.c	2008-10-11 04:51:59.000000000 -0400
@@ -69,7 +69,7 @@ static struct dentry *ocfs2_get_dentry(s
 	}
 
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = &ocfs2_dentry_ops;
 
 	mlog_exit_ptr(result);
@@ -104,7 +104,7 @@ static struct dentry *ocfs2_get_parent(s
 	}
 
 	parent = d_obtain_alias(ocfs2_iget(OCFS2_SB(dir->i_sb), blkno, 0, 0));
-	if (parent && !IS_ERR(parent))
+	if (!IS_ERR(parent))
 		parent->d_op = &ocfs2_dentry_ops;
 
 bail_unlock:
Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:48:36.000000000 -0400
+++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:52:34.000000000 -0400
@@ -312,7 +312,7 @@ xfs_open_by_handle(
 	}
 
 	dentry = d_obtain_alias(inode);
-	if (dentry == NULL) {
+	if (IS_ERR(dentry)) {
 		put_unused_fd(new_fd);
 		return -XFS_ERROR(ENOMEM);
 	}

Re: [PATCH] prevent NULL returns from d_obtain_alias

[Miklos Szeredi]

On Wed, 15 Oct 2008, Christoph Hellwig wrote:
> As Miklos pointed out a while ago returning NULL from the export
> operations and thus d_obtain_alias is not a good idea - the callers
> in exportfs convert it to an -ESTALE anyway (or as the audit pointed
> out don't deal with it at all in case of ->get_parent), and possibly
> returning either NULL or an ERR_PTR value form one function is
> confusing.  By auditing the callers I also found various other places
> that couldn't deal with the NULL return.  I guess an unlikely error
> path of an unlikely pass like this simply doesn't get tested.

Looks good, except:

> Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
> ===================================================================
> --- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:48:36.000000000 -0400
> +++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:52:34.000000000 -0400
> @@ -312,7 +312,7 @@ xfs_open_by_handle(
>  	}
>  
>  	dentry = d_obtain_alias(inode);
> -	if (dentry == NULL) {
> +	if (IS_ERR(dentry)) {
>  		put_unused_fd(new_fd);
>  		return -XFS_ERROR(ENOMEM);

should be

  		return -XFS_ERROR(-PTR_ERR(dentry));

Thanks,
Miklos

Re: [PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

On Thu, Oct 16, 2008 at 07:50:10PM +0200, Miklos Szeredi wrote:
> should be
> 
>   		return -XFS_ERROR(-PTR_ERR(dentry));

No need for XFS_ERROR at all here, it's only used for error injection in
low-level code.  Updated version that propagates the error below:

(Al, if you rebase vfs.git this should probably be folded into the patch
that switches over to d_obtain_alias)


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/dcache.c
===================================================================
--- vfs-2.6.orig/fs/dcache.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/dcache.c	2008-10-16 20:05:56.000000000 +0200
@@ -1123,10 +1123,10 @@ static inline struct hlist_head *d_hash(
  * allocating a new one.
  *
  * On successful return, the reference to the inode has been transferred
- * to the dentry.  If %NULL is returned (indicating kmalloc failure),
- * the reference on the inode has been released.  To make it easier
- * to use in export operations a NULL or IS_ERR inode may be passed in
- * and will be casted to the corresponding NULL or IS_ERR dentry.
+ * to the dentry.  In case of an error the reference on the inode is released.
+ * To make it easier to use in export operations a %NULL or IS_ERR inode may
+ * be passed in and will be the error will be propagate to the return value,
+ * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
  */
 struct dentry *d_obtain_alias(struct inode *inode)
 {
@@ -1135,7 +1135,7 @@ struct dentry *d_obtain_alias(struct ino
 	struct dentry *res;
 
 	if (!inode)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
Index: vfs-2.6/fs/exportfs/expfs.c
===================================================================
--- vfs-2.6.orig/fs/exportfs/expfs.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/exportfs/expfs.c	2008-10-16 20:05:56.000000000 +0200
@@ -367,8 +367,6 @@ struct dentry *exportfs_decode_fh(struct
 	 * 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;
 
@@ -422,8 +420,6 @@ struct dentry *exportfs_decode_fh(struct
 
 		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;
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/fat/inode.c	2008-10-16 20:05:56.000000000 +0200
@@ -697,7 +697,7 @@ static struct dentry *fat_fh_to_dentry(s
 	 * of luck.  But all that is for another day
 	 */
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = sb->s_root->d_op;
 	return result;
 }
Index: vfs-2.6/fs/fuse/inode.c
===================================================================
--- vfs-2.6.orig/fs/fuse/inode.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/fuse/inode.c	2008-10-16 20:05:56.000000000 +0200
@@ -597,7 +597,7 @@ static struct dentry *fuse_get_dentry(st
 		goto out_iput;
 
 	entry = d_obtain_alias(inode);
-	if (entry && !IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
 		entry->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(entry);
 	}
@@ -699,7 +699,7 @@ static struct dentry *fuse_get_parent(st
 	}
 
 	parent = d_obtain_alias(inode);
-	if (parent && !IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
 		parent->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(parent);
 	}
Index: vfs-2.6/fs/gfs2/ops_export.c
===================================================================
--- vfs-2.6.orig/fs/gfs2/ops_export.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/gfs2/ops_export.c	2008-10-16 20:05:56.000000000 +0200
@@ -139,7 +139,7 @@ static struct dentry *gfs2_get_parent(st
 	gfs2_str2qstr(&dotdot, "..");
 
 	dentry = d_obtain_alias(gfs2_lookupi(child->d_inode, &dotdot, 1));
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 }
@@ -223,7 +223,7 @@ static struct dentry *gfs2_get_dentry(st
 
 out_inode:
 	dentry = d_obtain_alias(inode);
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 
Index: vfs-2.6/fs/nfs/getroot.c
===================================================================
--- vfs-2.6.orig/fs/nfs/getroot.c	2008-10-15 21:26:34.000000000 +0200
+++ vfs-2.6/fs/nfs/getroot.c	2008-10-16 20:05:56.000000000 +0200
@@ -108,9 +108,9 @@ struct dentry *nfs_get_root(struct super
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
@@ -277,9 +277,9 @@ struct dentry *nfs4_get_root(struct supe
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
Index: vfs-2.6/fs/ocfs2/export.c
===================================================================
--- vfs-2.6.orig/fs/ocfs2/export.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/ocfs2/export.c	2008-10-16 20:05:56.000000000 +0200
@@ -69,7 +69,7 @@ static struct dentry *ocfs2_get_dentry(s
 	}
 
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = &ocfs2_dentry_ops;
 
 	mlog_exit_ptr(result);
@@ -104,7 +104,7 @@ static struct dentry *ocfs2_get_parent(s
 	}
 
 	parent = d_obtain_alias(ocfs2_iget(OCFS2_SB(dir->i_sb), blkno, 0, 0));
-	if (parent && !IS_ERR(parent))
+	if (!IS_ERR(parent))
 		parent->d_op = &ocfs2_dentry_ops;
 
 bail_unlock:
Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-16 20:06:20.000000000 +0200
@@ -312,9 +312,9 @@ xfs_open_by_handle(
 	}
 
 	dentry = d_obtain_alias(inode);
-	if (dentry == NULL) {
+	if (IS_ERR(dentry)) {
 		put_unused_fd(new_fd);
-		return -XFS_ERROR(ENOMEM);
+		return PTR_ERR(dentry);
 	}
 
 	/* Ensure umount returns EBUSY on umounts while this file is open. */