Re: [PATCH v3 1/6] locks: consolidate common code in the flock_to_posix_lock routines

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Dec 10, 2013 at 04:22:53PM -0500, bfields wrote:
> On Tue, Dec 10, 2013 at 02:17:30PM -0500, Jeff Layton wrote:
> > Currently, there's a lot of copy and paste between the two.  Add some
> > functions to do the initialization of the file_lock from values
> > passed in, and turn the flock/flock64 variants of those functions into
> > wrappers around them.
> > 
> > Unfortunately it's harder to consolidate the fl_start/fl_end
> > calculations due to the differently sized types involved so I've left
> > them separate for now.
> 
> I'd think you could assign everything to the flock64 type and do the
> common work there or something.
> 
> But I'm confused about what the current code is actually trying to do:
> if I'm chasing down the definitions right, these quantities are all
> signed, and when start is defined as an off_t it can overflow in the
> SEEK_CUR and SEEK_END cases.  And
> 
> 	if (fl->fl_end < fl->fl_start)
> 		return -EOVERFLOW
> 
> is counting on overlow wrapping around, which I thought wasn't
> guaranteed in the case of signed arithmetic?

E.g. the following (untested) removes the duplication and should return
-EOVERFLOW in the cases we currently do a random conversion from 64- to
32-bit and back.  Susv3 says:

	[EOVERFLOW]
	The cmd argument is F_GETLK, F_SETLK, or F_SETLKW and the
	smallest or, if l_len is non-zero, the largest offset of any
	byte in the requested segment cannot be represented correctly in
	an object of type off_t.

so that's what I tried to do.

--b.

diff --git a/fs/locks.c b/fs/locks.c
index 92a0f0a..47832f5 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -344,48 +344,41 @@ static int assign_type(struct file_lock *fl, long type)
 	return 0;
 }
 
-/* Verify a "struct flock" and copy it to a "struct file_lock" as a POSIX
- * style lock.
- */
-static int flock_to_posix_lock(struct file *filp, struct file_lock *fl,
-			       struct flock *l)
+static int flock_to_posix_lock_common(struct file *filp, struct file_lock *fl,
+					struct flock64 *l, loff_t offset_max)
 {
-	off_t start, end;
+	loff_t start;
 
 	switch (l->l_whence) {
 	case SEEK_SET:
-		start = 0;
-		break;
+		fl->fl_start = 0;
 	case SEEK_CUR:
-		start = filp->f_pos;
-		break;
+		fl->fl_start = filp->f_pos;
 	case SEEK_END:
-		start = i_size_read(file_inode(filp));
-		break;
+		fl->fl_start = i_size_read(file_inode(filp));
 	default:
 		return -EINVAL;
 	}
+	if (l->l_start < 0)
+		return -EINVAL;
+	if (l->l_start > offset_max - fl->fl_start)
+		return -EOVERFLOW;
+	fl->fl_start += l->l_start;
+	if (l->l_len > offset_max - fl->fl_start)
+		return -EOVERFLOW;
+	if (fl->fl_start + l->l_len < 0)
+		return -EINVAL;
 
 	/* POSIX-1996 leaves the case l->l_len < 0 undefined;
 	   POSIX-2001 defines it. */
-	start += l->l_start;
-	if (start < 0)
-		return -EINVAL;
-	fl->fl_end = OFFSET_MAX;
-	if (l->l_len > 0) {
-		end = start + l->l_len - 1;
-		fl->fl_end = end;
-	} else if (l->l_len < 0) {
-		end = start - 1;
-		fl->fl_end = end;
-		start += l->l_len;
-		if (start < 0)
-			return -EINVAL;
-	}
-	fl->fl_start = start;	/* we record the absolute position */
-	if (fl->fl_end < fl->fl_start)
-		return -EOVERFLOW;
-	
+	if (l->l_len > 0)
+		fl->fl_end = fl->fl_start + l->l_len - 1;
+	else if (l->l_len < 0) {
+		fl->fl_end = start - 1;
+		fl->fl_start += l->l_len;
+	} else
+		fl->fl_end = OFFSET_MAX;
+
 	fl->fl_owner = current->files;
 	fl->fl_pid = current->tgid;
 	fl->fl_file = filp;
@@ -396,50 +389,27 @@ static int flock_to_posix_lock(struct file *filp, struct file_lock *fl,
 	return assign_type(fl, l->l_type);
 }
 
+/* Verify a "struct flock" and copy it to a "struct file_lock" as a POSIX
+ * style lock.
+ */
+static int flock_to_posix_lock(struct file *filp, struct file_lock *fl,
+			       struct flock *l)
+{
+	struct flock64 ll = {
+		.l_type = l->l_type,
+		.l_whence = l->l_whence,
+		.l_start = l->l_start,
+		.l_len = l->l_len,
+	};
+	
+	return flock_to_posix_lock_common(filp, fl, &ll, OFFT_OFFSET_MAX);
+}
+
 #if BITS_PER_LONG == 32
 static int flock64_to_posix_lock(struct file *filp, struct file_lock *fl,
 				 struct flock64 *l)
 {
-	loff_t start;
-
-	switch (l->l_whence) {
-	case SEEK_SET:
-		start = 0;
-		break;
-	case SEEK_CUR:
-		start = filp->f_pos;
-		break;
-	case SEEK_END:
-		start = i_size_read(file_inode(filp));
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	start += l->l_start;
-	if (start < 0)
-		return -EINVAL;
-	fl->fl_end = OFFSET_MAX;
-	if (l->l_len > 0) {
-		fl->fl_end = start + l->l_len - 1;
-	} else if (l->l_len < 0) {
-		fl->fl_end = start - 1;
-		start += l->l_len;
-		if (start < 0)
-			return -EINVAL;
-	}
-	fl->fl_start = start;	/* we record the absolute position */
-	if (fl->fl_end < fl->fl_start)
-		return -EOVERFLOW;
-	
-	fl->fl_owner = current->files;
-	fl->fl_pid = current->tgid;
-	fl->fl_file = filp;
-	fl->fl_flags = FL_POSIX;
-	fl->fl_ops = NULL;
-	fl->fl_lmops = NULL;
-
-	return assign_type(fl, l->l_type);
+	return flock_to_posix_lock_common(filp, fl, l, OFFSET_MAX);
 }
 #endif
 
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 95e46c8..36025f7 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -186,8 +186,6 @@ struct flock {
 };
 #endif
 
-#ifndef CONFIG_64BIT
-
 #ifndef HAVE_ARCH_STRUCT_FLOCK64
 #ifndef __ARCH_FLOCK64_PAD
 #define __ARCH_FLOCK64_PAD
@@ -202,6 +200,5 @@ struct flock64 {
 	__ARCH_FLOCK64_PAD
 };
 #endif
-#endif /* !CONFIG_64BIT */
 
 #endif /* _ASM_GENERIC_FCNTL_H */