prevent NULL returns from d_obtain_alias

PATCH In review Christoph Hellwig 6 participants 10 messages Oct 15, 2008 – Oct 20, 2008

Subsystems (8)

VFS NFS client XFS filesystem OCFS2 +4 more subsystems

lore

Share & tools

lore

Christoph Hellwig

author

2008-10-15 19:28 UTC #0 lore raw

PATCH 1/1 prevent NULL returns from d_obtain_alias

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 <[email protected]>

fs/dcache.c +5 -5

Index: vfs-2.6/fs/dcache.c
===================================================================
@@ -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);

fs/exportfs/expfs.c +0 -4

Index: vfs-2.6/fs/exportfs/expfs.c
===================================================================
@@ -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;

fs/fat/inode.c +1 -1

Index: vfs-2.6/fs/fat/inode.c
===================================================================
@@ -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;
 }

fs/fuse/inode.c +2 -2

Index: vfs-2.6/fs/fuse/inode.c
===================================================================
@@ -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);
 	}

fs/gfs2/ops_export.c +2 -2

Index: vfs-2.6/fs/gfs2/ops_export.c
===================================================================
@@ -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;

fs/nfs/getroot.c +4 -4

Index: vfs-2.6/fs/nfs/getroot.c
===================================================================
@@ -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);

fs/ocfs2/export.c +2 -2

Index: vfs-2.6/fs/ocfs2/export.c
===================================================================
@@ -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:

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

Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
@@ -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);
 	}

Miklos Szeredi

maintainer

2008-10-16 17:50 UTC #1 lore raw

Quoted 8 lines · 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.

Looks good, except:

fs/xfs/linux-2.6/xfs_ioctl.c xfs_open_by_handle( · line 317

Quoted 12 lines

Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
fs/xfs/linux-2.6/xfs_ioctl.c +1 -1
@@ -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

Christoph Hellwig

author

2008-10-16 18:09 UTC #2 lore raw

Quoted 3 lines · Miklos Szeredi

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 <[email protected]>

fs/dcache.c +5 -5

Index: vfs-2.6/fs/dcache.c
===================================================================
@@ -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);

fs/exportfs/expfs.c +0 -4

Index: vfs-2.6/fs/exportfs/expfs.c
===================================================================
@@ -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;

fs/fat/inode.c +1 -1

Index: vfs-2.6/fs/fat/inode.c
===================================================================
@@ -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;
 }

fs/fuse/inode.c +2 -2

Index: vfs-2.6/fs/fuse/inode.c
===================================================================
@@ -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);
 	}

fs/gfs2/ops_export.c +2 -2

Index: vfs-2.6/fs/gfs2/ops_export.c
===================================================================
@@ -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;

fs/nfs/getroot.c +4 -4

Index: vfs-2.6/fs/nfs/getroot.c
===================================================================
@@ -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);

fs/ocfs2/export.c +2 -2

Index: vfs-2.6/fs/ocfs2/export.c
===================================================================
@@ -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:

fs/xfs/linux-2.6/xfs_ioctl.c +2 -2

Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
@@ -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. */

Timothy Shimmin

2008-10-17 00:09 UTC #3 lore raw

XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Miklos Szeredi
should be
  		return -XFS_ERROR(-PTR_ERR(dentry));

Quoted 9 lines

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 <[email protected]>

[...stuff deleted]

Quoted 7 lines

Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
fs/xfs/linux-2.6/xfs_ioctl.c +0 -0
@@ -312,9 +312,9 @@ xfs_open_by_handle(
 	}
 

fs/xfs/linux-2.6/xfs_ioctl.c xfs_open_by_handle(

Quoted 10 lines

fs/xfs/linux-2.6/xfs_ioctl.c +2 -3

 	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. */
--

Fair enough.
But XFS_ERROR is used throughout the function.
I've found the whole idea of when and when not to use XFS_ERROR annoying :)

I've never used it (other than calling it to stay consistent with the code).
Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] - and it does this in xfs_error_trap().
Can one not decide to do this at any error point?
I can't see where we hook in to set up xfs_etrap.

Quoted 8 lines

#ifdef DEBUG
#define XFS_ERROR_NTRAP 10
extern int xfs_etrap[XFS_ERROR_NTRAP];
extern int xfs_error_trap(int);
#define XFS_ERROR(e) xfs_error_trap(e)
#else
#define XFS_ERROR(e) (e)
#endif

Cheers,
Tim.

Dave Chinner

2008-10-17 01:52 UTC #4 lore raw

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

Quoted 9 lines · Timothy Shimmin

Fair enough.
But XFS_ERROR is used throughout the function.
I've found the whole idea of when and when not to use XFS_ERROR annoying :)
I've never used it (other than calling it to stay consistent with the code).
Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] - and it does this in xfs_error_trap().
Can one not decide to do this at any error point?
I can't see where we hook in to set up xfs_etrap.

You break into the debugger, modify the xfs_etrap array to contain the set of errors you want to catch, then continue onwards. I've used this several times in the past couple of months to locate the source of strange ENOSPC errors...

The conceptual idea is that every point where an error is first detected gets wrapped with XFS_ERROR() so you don't need to sprinkle printk's all through the code to find where the error is coming from.

ISTR the Irix kernel debugger had a command to set the values in the etrap array - if it did it never got ported to kdb....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Eric Sandeen

2008-10-17 03:03 UTC #5 lore raw

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

Quoted 5 lines · Dave Chinner

Quoted 4 lines · Timothy Shimmin
Fair enough.
But XFS_ERROR is used throughout the function.
I've found the whole idea of when and when not to use XFS_ERROR annoying :)

Quoted 8 lines

Quoted 5 lines
I've never used it (other than calling it to stay consistent with the code).
Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] - and it does this in xfs_error_trap().
Can one not decide to do this at any error point?
I can't see where we hook in to set up xfs_etrap.
You break into the debugger, modify the xfs_etrap array to contain the set of errors you want to catch, then continue onwards.

bleah :)

Could these just be turned into tracepoints eventually? Or maybe for now allow modifying the array via proc or something... would be easier to ask a user to do that if you don't have direct access to their kdb console ;) It is a handy thing to have, though.

-Eric

Christoph Hellwig

author

2008-10-17 17:08 UTC #6 lore raw

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

In reply to Timothy Shimmin

Quoted 3 lines

Fair enough.
But XFS_ERROR is used throughout the function.

Can we leave it the simple way for now? I have to revamp that whole function anyway as it's extremly buggy in many ways, especially when used to open directories (can lead to multiple dentries for a single directory - ouch) and then I'll kill the other uses.

Timothy Shimmin

2008-10-20 00:48 UTC #7 lore raw

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines
Fair enough.
But XFS_ERROR is used throughout the function.
Can we leave it the simple way for now?

Yep.

Quoted 4 lines

I have to revamp that whole
function anyway as it's extremly buggy in many ways, especially when used to open directories (can lead to multiple dentries for a single directory - ouch) and then I'll kill the other uses.

Oh ok.

In userspace,
we use it for opening directories on xfsdump via jdm_open in order to do bulkstat driven dirent dumping.
We also use it in xfsrestore - though I am not convinced we should - it was initially done for "performance" reasons apparently.

--Tim

Christoph Hellwig

2008-10-20 09:22 UTC #8 lore raw

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

Quoted 5 lines · Timothy Shimmin

Quoted 4 lines
I have to revamp that whole
function anyway as it's extremly buggy in many ways, especially when used to open directories (can lead to multiple dentries for a single directory - ouch) and then I'll kill the other uses.

Quoted 7 lines

Oh ok.
In userspace,
we use it for opening directories on xfsdump via jdm_open in order to do bulkstat driven dirent dumping.
We also use it in xfsrestore - though I am not convinced we should - it was initially done for "performance" reasons apparently.

I think the use of the API is fine, the problem is that the current implemention is buggy.

Timothy Shimmin

2008-10-20 23:14 UTC #9 lore raw

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

Quoted 7 lines · Christoph Hellwig

Quoted 6 lines · Timothy Shimmin
Quoted 4 lines
I have to revamp that whole
function anyway as it's extremly buggy in many ways, especially when used to open directories (can lead to multiple dentries for a single directory - ouch) and then I'll kill the other uses.
Oh ok.

Quoted 8 lines

Quoted 5 lines
In userspace,
we use it for opening directories on xfsdump via jdm_open in order to do bulkstat driven dirent dumping.
We also use it in xfsrestore - though I am not convinced we should - it was initially done for "performance" reasons apparently.
I think the use of the API is fine, the problem is that the current implemention is buggy.

Yeah, I have no problem for xfsdump as it is driven by bulkstat but for xfsrestore it seems unnecessary.
In fact, it can restore to other filesystems so it must only be using this in certain ways on restore.
Having a look, yeah it is predicated by:
if ( tranp->t_dstdirisxfspr )
and then just uses the fd for XFS_IOC_FSSETDM, XFS_IOC_FSSETXATTR for those xfs-specific things and in the same function it uses the path for non-xfs-specific: utime, chown, chmod
So it seems unnecessary to me in restore.

--Tim