Skip to contentSkip to conversation

ocfs2: fix error handling in ocfs2_ioctl_move_extents()

PATCH In review Jeff Liu 2 participants 2 messages Apr 9, 2013

Subsystems (2)
lore
Share & tools
lore

Jeff Liu

author
#0 lore raw

PATCH 1/2 ocfs2: fix error handling in ocfs2_ioctl_move_extents()

From: Dan Carpenter <[email protected]>

Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.

I've re-written the error handling to avoid this problem and make it a little cleaner as well.

---
v2: check for argp earlier

fs/ocfs2/move_extents.c +17 -20
index 9f8dcad..8f3d3cb 100644
@@ -1057,42 +1057,40 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 	struct inode *inode = file_inode(filp);
 	struct ocfs2_move_extents range;
-	struct ocfs2_move_extents_context *context = NULL;
+	struct ocfs2_move_extents_context *context;
+
+	if (!argp)
+		return -EINVAL;
 
 	status = mnt_want_write_file(filp);
 	if (status)
 		return status;
 
 	if ((!S_ISREG(inode->i_mode)) || !(filp->f_mode & FMODE_WRITE))
-		goto out;
+		goto out_drop;
 
 	if (inode->i_flags & (S_IMMUTABLE|S_APPEND)) {
 		status = -EPERM;
-		goto out;
+		goto out_drop;
 	}
 
 	context = kzalloc(sizeof(struct ocfs2_move_extents_context), GFP_NOFS);
 	if (!context) {
 		status = -ENOMEM;
 		mlog_errno(status);
-		goto out;
+		goto out_drop;
 	}
 
 	context->inode = inode;
 	context->file = filp;
 
-	if (argp) {
-		if (copy_from_user(&range, argp, sizeof(range))) {
-			status = -EFAULT;
-			goto out;
-		}
-	} else {
-		status = -EINVAL;
-		goto out;
+	if (copy_from_user(&range, argp, sizeof(range))) {
+		status = -EFAULT;
+		goto out_free;
 	}
 
 	if (range.me_start > i_size_read(inode))
-		goto out;
+		goto out_free;
 
 	if (range.me_start + range.me_len > i_size_read(inode))
 			range.me_len = i_size_read(inode) - range.me_start;
@@ -1124,25 +1122,24 @@ int ocfs2_ioctl_move_extents(struct file *filp, void __user *argp)
 
 		status = ocfs2_validate_and_adjust_move_goal(inode, &range);
 		if (status)
-			goto out;
+			goto out_copy;
 	}
 
 	status = ocfs2_move_extents(context);
 	if (status)
 		mlog_errno(status);
-out:
+out_copy:
 	/*
 	 * movement/defragmentation may end up being partially completed,
 	 * that's the reason why we need to return userspace the finished
 	 * length and new_offset even if failure happens somewhere.
 	 */
-	if (argp) {
-		if (copy_to_user(argp, &range, sizeof(range)))
-			status = -EFAULT;
-	}
+	if (copy_to_user(argp, &range, sizeof(range)))
+		status = -EFAULT;
 
+out_free:
 	kfree(context);
-
+out_drop:
 	mnt_drop_write_file(filp);
 
 	return status;
#1 lore raw
Quoted 11 lines · On Tue, 09 Apr 2013 11:17:40 +0800 Jeff Liu <[email protected]> wrote:

From: Dan Carpenter <[email protected]>

Smatch complains that if we hit an error (for example if the file is immutable) then "range" has uninitialized stack data and we copy it to the user.

I've re-written the error handling to avoid this problem and make it a little cleaner as well.

I've added your signed-off-by to these patches - it should have been included because you were on the patch delivery path.