locks: implement "filp-private" (aka UNPOSIX) locks

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.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/locks.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

fs/locks.c +15 -16

index 6084f5a..a5848ed 100644
@@ -344,6 +344,19 @@ static int assign_type(struct file_lock *fl, long type)
 	return 0;
 }
 
+static int
+flock_to_posix_lock_common(struct file_lock *fl, struct file *filp, short type)
+{
+	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, type);
+}
+
 /* Verify a "struct flock" and copy it to a "struct file_lock" as a POSIX
  * style lock.
  */

@@ -386,14 +399,7 @@ static int flock_to_posix_lock(struct file *filp, struct file_lock *fl,
 	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(fl, filp, l->l_type);
 }
 
 #if BITS_PER_LONG == 32

@@ -431,15 +437,8 @@ static int flock64_to_posix_lock(struct file *filp, struct file_lock *fl,
 	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(fl, filp, l->l_type);
 }
 #endif
 

Signature · 7 lines

1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 8 lines · Jeff Layton

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?

Quoted 18 lines · bfields

Quoted 8 lines · Jeff Layton

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

Quoted 5 lines

	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.

fs/locks.c +40 -70

index 92a0f0a..47832f5 100644
@@ -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
 

include/uapi/asm-generic/fcntl.h +0 -2

index 95e46c8..36025f7 100644
@@ -186,8 +186,6 @@ struct flock {
 };
 #endif
 
-#ifndef CONFIG_64BIT
-
 #ifndef HAVE_ARCH_STRUCT_FLOCK64
 #ifndef __ARCH_FLOCK64_PAD
 #define __ARCH_FLOCK64_PAD

Quoted 202 lines · On Tue, 10 Dec 2013 18:22:04 -0500 "J. Bruce Fields" <[email protected]>…

Quoted 18 lines · bfields

Quoted 8 lines · Jeff Layton

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

Quoted 5 lines

	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.

fs/locks.c +40 -70

index 92a0f0a..47832f5 100644
@@ -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
 

include/uapi/asm-generic/fcntl.h +0 -3

index 95e46c8..36025f7 100644
@@ -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 */
 

include/uapi/asm-generic/fcntl.h struct flock64 { · line 204

Quoted 1 line

include/uapi/asm-generic/fcntl.h +0 -0

 #endif /* _ASM_GENERIC_FCNTL_H */

Nice. I had started to consolidate them, but then figured out that there are so many edge cases and I didn't have a good way to test them all.

As far as I can tell though, this looks correct. I'll plan to drop my patch and base the rest of the set on top of yours.

Well, it'd be weird if I didn't screw up something somewhere.

Quoted 1 line · J. Bruce Fields

Well, it'd be weird if I didn't screw up something somewhere.

Yes, a classic: I forgot the breaks after each switch case.

Here's a version that at least doesn't return -EINVAL on every lock attempt.

--b.

commit 70b7b9a442d06a71306736eb01cedba5dd6d86cf

Author: J. Bruce Fields <[email protected]>
Date:   Tue Dec 10 18:14:28 2013 -0500

locks: fix posix lock range overflow handling

In the 32-bit case fcntl is converting signed 64-bit to signed 32-bit quantities in a couple places, with probably incorrect results.

So instead let's return -EOVERFLOW as described in SUSv3, whenever "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."

While we're here, do some cleanup including consolidating code for the flock and flock64 cases.

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

fs/locks.c +40 -67

index 92a0f0a..b70486a 100644
@@ -344,48 +344,44 @@ 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;
+		fl->fl_start = 0;
 		break;
 	case SEEK_CUR:
-		start = filp->f_pos;
+		fl->fl_start = filp->f_pos;
 		break;
 	case SEEK_END:
-		start = i_size_read(file_inode(filp));
+		fl->fl_start = i_size_read(file_inode(filp));
 		break;
 	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 +392,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
 

include/uapi/asm-generic/fcntl.h +0 -2

index 95e46c8..36025f7 100644
@@ -186,8 +186,6 @@ struct flock {
 };
 #endif
 
-#ifndef CONFIG_64BIT
-
 #ifndef HAVE_ARCH_STRUCT_FLOCK64
 #ifndef __ARCH_FLOCK64_PAD
 #define __ARCH_FLOCK64_PAD

Quoted 187 lines · On Wed, 11 Dec 2013 10:19:31 -0500 "J. Bruce Fields" <[email protected]>…

Quoted 1 line · J. Bruce Fields

Well, it'd be weird if I didn't screw up something somewhere.

Yes, a classic: I forgot the breaks after each switch case.

Here's a version that at least doesn't return -EINVAL on every lock attempt.

--b.

commit 70b7b9a442d06a71306736eb01cedba5dd6d86cf

Author: J. Bruce Fields <[email protected]>
Date:   Tue Dec 10 18:14:28 2013 -0500

locks: fix posix lock range overflow handling

In the 32-bit case fcntl is converting signed 64-bit to signed 32-bit quantities in a couple places, with probably incorrect results.

So instead let's return -EOVERFLOW as described in SUSv3, whenever "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."

While we're here, do some cleanup including consolidating code for the flock and flock64 cases.

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

fs/locks.c +40 -67

index 92a0f0a..b70486a 100644
@@ -344,48 +344,44 @@ 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;
+		fl->fl_start = 0;
 		break;
 	case SEEK_CUR:
-		start = filp->f_pos;
+		fl->fl_start = filp->f_pos;
 		break;
 	case SEEK_END:
-		start = i_size_read(file_inode(filp));
+		fl->fl_start = i_size_read(file_inode(filp));
 		break;
 	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 +392,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
 

include/uapi/asm-generic/fcntl.h +0 -2

index 95e46c8..36025f7 100644
@@ -186,8 +186,6 @@ struct flock {
 };
 #endif
 
-#ifndef CONFIG_64BIT
-
 #ifndef HAVE_ARCH_STRUCT_FLOCK64
 #ifndef __ARCH_FLOCK64_PAD
 #define __ARCH_FLOCK64_PAD

include/uapi/asm-generic/fcntl.h struct flock64 { · line 204

Quoted 6 lines

include/uapi/asm-generic/fcntl.h +0 -1

@@ -202,6 +200,5 @@ struct flock64 {
 	__ARCH_FLOCK64_PAD
 };
 #endif
-#endif /* !CONFIG_64BIT */
 
 #endif /* _ASM_GENERIC_FCNTL_H */

Thanks -- looks like this is hitting the same discrepancy that I was getting with my consolidation attempt. Setting a lock like this on x86_64 fails with the current kernel:

	struct flock lck1 = {
		.l_type		= F_WRLCK,
		.l_whence	= SEEK_SET,
		.l_start	= 0,
		.l_len		= LONG_MAX,
	};

...but succeeds with your patch in place. I've not sat down yet to figure out whether you broke or fixed something though :).

Signature · 1 line

Jeff Layton <[email protected]>

Quoted 201 lines · Jeff Layton

Quoted 187 lines · On Wed, 11 Dec 2013 10:19:31 -0500 "J. Bruce Fields" <[email protected]>…

Quoted 1 line · J. Bruce Fields

Well, it'd be weird if I didn't screw up something somewhere.

Yes, a classic: I forgot the breaks after each switch case.

Here's a version that at least doesn't return -EINVAL on every lock attempt.

--b.

commit 70b7b9a442d06a71306736eb01cedba5dd6d86cf

Author: J. Bruce Fields <[email protected]>
Date:   Tue Dec 10 18:14:28 2013 -0500

locks: fix posix lock range overflow handling

In the 32-bit case fcntl is converting signed 64-bit to signed 32-bit quantities in a couple places, with probably incorrect results.

So instead let's return -EOVERFLOW as described in SUSv3, whenever "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."

While we're here, do some cleanup including consolidating code for the flock and flock64 cases.

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

fs/locks.c +40 -67

index 92a0f0a..b70486a 100644
@@ -344,48 +344,44 @@ 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;
+		fl->fl_start = 0;
 		break;
 	case SEEK_CUR:
-		start = filp->f_pos;
+		fl->fl_start = filp->f_pos;
 		break;
 	case SEEK_END:
-		start = i_size_read(file_inode(filp));
+		fl->fl_start = i_size_read(file_inode(filp));
 		break;
 	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 +392,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
 

include/uapi/asm-generic/fcntl.h +0 -2

index 95e46c8..36025f7 100644
@@ -186,8 +186,6 @@ struct flock {
 };
 #endif
 
-#ifndef CONFIG_64BIT
-
 #ifndef HAVE_ARCH_STRUCT_FLOCK64
 #ifndef __ARCH_FLOCK64_PAD
 #define __ARCH_FLOCK64_PAD

include/uapi/asm-generic/fcntl.h struct flock64 { · line 204

Quoted 6 lines

include/uapi/asm-generic/fcntl.h +0 -1

@@ -202,6 +200,5 @@ struct flock64 {
 	__ARCH_FLOCK64_PAD
 };
 #endif
-#endif /* !CONFIG_64BIT */
 
 #endif /* _ASM_GENERIC_FCNTL_H */

Thanks -- looks like this is hitting the same discrepancy that I was getting with my consolidation attempt. Setting a lock like this on x86_64 fails with the current kernel:

Quoted 6 lines

	struct flock lck1 = {
		.l_type		= F_WRLCK,
		.l_whence	= SEEK_SET,
		.l_start	= 0,
		.l_len		= LONG_MAX,
	};

Are you sure? I've tried but can't reproduce that. I'm on 3.13-rc3 plus a couple DRC patches from you.

--b.

Quoted 1 line · J. Bruce Fields

Well, it'd be weird if I didn't screw up something somewhere.

Yes, a classic: I forgot the breaks after each switch case.

Here's a version that at least doesn't return -EINVAL on every lock attempt.

--b.

commit 70b7b9a442d06a71306736eb01cedba5dd6d86cf

Author: J. Bruce Fields <[email protected]>
Date:   Tue Dec 10 18:14:28 2013 -0500

locks: fix posix lock range overflow handling

In the 32-bit case fcntl is converting signed 64-bit to signed 32-bit quantities in a couple places, with probably incorrect results.

So instead let's return -EOVERFLOW as described in SUSv3, whenever "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."

While we're here, do some cleanup including consolidating code for the flock and flock64 cases.

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

fs/locks.c +15 -21

index 92a0f0a..b70486a 100644
@@ -344,48 +344,44 @@ 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;
+		fl->fl_start = 0;
 		break;
 	case SEEK_CUR:
-		start = filp->f_pos;
+		fl->fl_start = filp->f_pos;
 		break;
 	case SEEK_END:
-		start = i_size_read(file_inode(filp));
+		fl->fl_start = i_size_read(file_inode(filp));
 		break;
 	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)

fs/locks.c +21 -40

+		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 +392,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
 

include/uapi/asm-generic/fcntl.h +0 -3

index 95e46c8..36025f7 100644
@@ -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 */

On Wed, 11 Dec 2013 10:19:31 -0500
"J. Bruce Fields" <[email protected]> wrote:

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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;

Erm... I think this is not quite right...

"start" is uninitialized here. I think this should be:

    fl->fl_end = fl->fl_start - 1

With that too, we can get rid of the local "start" variable. I think this may explain why I'm tripping over the BUG() in locks_remove_file.

Quoted 2 lines · Jeff Layton

On Wed, 11 Dec 2013 10:19:31 -0500
"J. Bruce Fields" <[email protected]> wrote:

...

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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;

Erm... I think this is not quite right...

Quoted 6 lines

"start" is uninitialized here. I think this should be:

    fl->fl_end = fl->fl_start - 1

With that too, we can get rid of the local "start" variable. I think this may explain why I'm tripping over the BUG() in locks_remove_file.

Yep.

One other bug: I think l_start < 0 is actually fine in the SEEK_CUR/SEEK_END cases.

With that fixed and another comment (though I don't know how much it helps), it looks like the below.

Quoted 18 lines · J. Bruce Fields

Quoted 2 lines · Jeff Layton

On Wed, 11 Dec 2013 10:19:31 -0500
"J. Bruce Fields" <[email protected]> wrote:

...

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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;

Erm... I think this is not quite right...

Quoted 6 lines

"start" is uninitialized here. I think this should be:

    fl->fl_end = fl->fl_start - 1

With that too, we can get rid of the local "start" variable. I think this may explain why I'm tripping over the BUG() in locks_remove_file.

Quoted 7 lines

Yep.

One other bug: I think l_start < 0 is actually fine in the SEEK_CUR/SEEK_END cases.

With that fixed and another comment (though I don't know how much it helps), it looks like the below.

Alternatively, maybe we could simplify? (On top of the previous):

commit d4bf5cb021a3ac1ec07530ebda904e262cc89d11

Author: J. Bruce Fields <[email protected]>
Date:   Wed Dec 11 17:42:32 2013 -0500

locks: simplify overflow checking

Or maybe we don't actually care about indicating overflow in the 32-bit
case: sure we could fail if e.g. f_pos+start or f_pos+start+len would exceed 32-bits, but do we really need to?

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

fs/locks.c +5 -11

index 39f2ca9..efbf577 100644
@@ -344,8 +344,8 @@ static int assign_type(struct file_lock *fl, long type)
 	return 0;
 }
 
-static int flock_to_posix_lock_common(struct file *filp, struct file_lock *fl,
-					struct flock64 *l, loff_t offset_max)
+static int flock64_to_posix_lock(struct file *filp, struct file_lock *fl,
+				 struct flock64 *l)
 {
 	switch (l->l_whence) {
 	case SEEK_SET:

@@ -360,12 +360,12 @@ static int flock_to_posix_lock_common(struct file *filp, struct file_lock *fl,
 	default:
 		return -EINVAL;
 	}
-	if (l->l_start > offset_max - fl->fl_start)
+	if (l->l_start > OFFSET_MAX - fl->fl_start)
 		return -EOVERFLOW;
 	fl->fl_start += l->l_start;
 	if (fl->fl_start < 0)
 		return -EINVAL;
-	if (l->l_len > offset_max - fl->fl_start)
+	if (l->l_len > OFFSET_MAX - fl->fl_start)
 		return -EOVERFLOW;
 	if (fl->fl_start + l->l_len < 0)
 		return -EINVAL;

@@ -403,22 +403,9 @@ static int flock_to_posix_lock(struct file *filp, struct file_lock *fl,
 		.l_len = l->l_len,
 	};
 
-	/*
-	 * The use of OFFT_OFFSET_MAX here ensures we return -EOVERFLOW
-	 * if the start or end of the lock could not be represented as
-	 * an off_t, following SUSv3.
-	 */
-	return flock_to_posix_lock_common(filp, fl, &ll, OFFT_OFFSET_MAX);
+	return flock64_to_posix_lock(filp, fl, &ll);
 }
 
-#if BITS_PER_LONG == 32

Quoted 2 lines · Jeff Layton

On Wed, 11 Dec 2013 10:19:31 -0500
"J. Bruce Fields" <[email protected]> wrote:

...

Quoted 7 lines

fs/locks.c static int assign_type(struct file_lock *fl, long type)

Quoted 4 lines

fs/locks.c +4 -0

+	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;

Erm... I think this is not quite right...

Quoted 6 lines

"start" is uninitialized here. I think this should be:

    fl->fl_end = fl->fl_start - 1

With that too, we can get rid of the local "start" variable. I think this may explain why I'm tripping over the BUG() in locks_remove_file.

Yep.

One other bug: I think l_start < 0 is actually fine in the SEEK_CUR/SEEK_END cases.

With that fixed and another comment (though I don't know how much it helps), it looks like the below.

--b.

commit 052f395880af17919d4c13037deed581f367b2a8

Author: J. Bruce Fields <[email protected]>
Date:   Tue Dec 10 18:14:28 2013 -0500

locks: fix posix lock range overflow handling

In the 32-bit case fcntl assigns f_pos and i_size to a 32-bit off_t, with probably incorrect results.

So instead let's return -EOVERFLOW as described in SUSv3, whenever "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."

While we're here, do some cleanup including consolidating code for the flock and flock64 cases.

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

fs/locks.c +5 -10

index 92a0f0a..39f2ca9 100644
@@ -344,48 +344,42 @@ 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;
-
 	switch (l->l_whence) {
 	case SEEK_SET:
-		start = 0;
+		fl->fl_start = 0;
 		break;
 	case SEEK_CUR:
-		start = filp->f_pos;
+		fl->fl_start = filp->f_pos;
 		break;
 	case SEEK_END:
-		start = i_size_read(file_inode(filp));
+		fl->fl_start = i_size_read(file_inode(filp));
 		break;
 	default:
 		return -EINVAL;
 	}

fs/locks.c +30 -58

 	/* 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 = fl->fl_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 +390,32 @@ 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,
+	};
+
+	/*
+	 * The use of OFFT_OFFSET_MAX here ensures we return -EOVERFLOW
+	 * if the start or end of the lock could not be represented as
+	 * an off_t, following SUSv3.
+	 */
+	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
 

include/uapi/asm-generic/fcntl.h +0 -3

index 95e46c8..36025f7 100644
@@ -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 */

FL_FILE_PVT locks are no longer tied to a particular pid, and are instead inheritable by child processes. Report a l_pid of '-1' for these sorts of locks since the pid is somewhat meaningless for them.

This precedent comes from FreeBSD. There, POSIX and flock() locks can conflict with one another. If fcntl(F_GETLK, ...) returns a lock set with flock() then the l_pid member cannot be a process ID because the lock is not held by a process as such.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/locks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/locks.c +1 -1

index e163a30..5372ddd 100644
@@ -1899,7 +1899,7 @@ EXPORT_SYMBOL_GPL(vfs_test_lock);
 
 static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl)
 {
-	flock->l_pid = fl->fl_pid;
+	flock->l_pid = IS_FILE_PVT(fl) ? -1 : fl->fl_pid;
 #if BITS_PER_LONG == 32
 	/*
 	 * Make sure we can represent the posix lock via

Quoted 29 lines · On Tue, 10 Dec 2013 14:17:34 -0500 Jeff Layton <[email protected]> wrote:

FL_FILE_PVT locks are no longer tied to a particular pid, and are instead inheritable by child processes. Report a l_pid of '-1' for these sorts of locks since the pid is somewhat meaningless for them.

This precedent comes from FreeBSD. There, POSIX and flock() locks can conflict with one another. If fcntl(F_GETLK, ...) returns a lock set with flock() then the l_pid member cannot be a process ID because the lock is not held by a process as such.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/locks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/locks.c +1 -1

index e163a30..5372ddd 100644
@@ -1899,7 +1899,7 @@ EXPORT_SYMBOL_GPL(vfs_test_lock);
 
 static int posix_lock_to_flock(struct flock *flock, struct file_lock
 *fl)  {
-	flock->l_pid = fl->fl_pid;
+	flock->l_pid = IS_FILE_PVT(fl) ? -1 : fl->fl_pid;
 #if BITS_PER_LONG == 32
 	/*
 	 * Make sure we can represent the posix lock via @@ -1921,7 +1921,7

@@ static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) #if BITS_PER_LONG == 32 static void

fs/locks.c static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) · line 1927

Quoted 6 lines

posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl) {

fs/locks.c +1 -1

-	flock->l_pid = fl->fl_pid;
+	flock->l_pid = IS_FILE_PVT(fl) ? -1 : fl->fl_pid;
 	flock->l_start = fl->fl_start;
 	flock->l_len = fl->fl_end == OFFSET_MAX ? 0 :
 		fl->fl_end - fl->fl_start + 1;

While I think this behavior is reasonable, I do wonder if we ought to

more changes to how F_GETLK works. Currently the F_GETLK code won't handle the new l_type values, but maybe it should...

For instance, if there is a conflicting lock, and the F_GETLK caller

F_RDLCKP or F_WRLCKP, might it make sense to report the l_type on the conflicting lock as F_RDLCKP or F_WRLCKP if that conflicting lock is also

type?

...or maybe we should consider a new F_GETLKP cmd value, and a new expanded struct flock that gives more info. The pid is already somewhat meaningless with this sort of lock. Perhaps we could obfuscate the

value and report that instead? What other sorts of info would be useful to programs that intend to use these new interfaces?

Quoted 32 lines

Quoted 29 lines · On Tue, 10 Dec 2013 14:17:34 -0500 Jeff Layton <[email protected]> wrote:

FL_FILE_PVT locks are no longer tied to a particular pid, and are instead inheritable by child processes. Report a l_pid of '-1' for these sorts of locks since the pid is somewhat meaningless for them.

This precedent comes from FreeBSD. There, POSIX and flock() locks can conflict with one another. If fcntl(F_GETLK, ...) returns a lock set with flock() then the l_pid member cannot be a process ID because the lock is not held by a process as such.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/locks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/locks.c +1 -1

index e163a30..5372ddd 100644
@@ -1899,7 +1899,7 @@ EXPORT_SYMBOL_GPL(vfs_test_lock);
 
 static int posix_lock_to_flock(struct flock *flock, struct file_lock
 *fl)  {
-	flock->l_pid = fl->fl_pid;
+	flock->l_pid = IS_FILE_PVT(fl) ? -1 : fl->fl_pid;
 #if BITS_PER_LONG == 32
 	/*
 	 * Make sure we can represent the posix lock via @@ -1921,7 +1921,7

@@ static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) #if BITS_PER_LONG == 32 static void

Quoted 8 lines

fs/locks.c static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) · line 1927

Quoted 6 lines

posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl) {

fs/locks.c +1 -1

-	flock->l_pid = fl->fl_pid;
+	flock->l_pid = IS_FILE_PVT(fl) ? -1 : fl->fl_pid;
 	flock->l_start = fl->fl_start;
 	flock->l_len = fl->fl_end == OFFSET_MAX ? 0 :
 		fl->fl_end - fl->fl_start + 1;

While I think this behavior is reasonable, I do wonder if we ought to

consider

Quoted 4 lines

more changes to how F_GETLK works. Currently the F_GETLK code won't handle the new l_type values, but maybe it should...

For instance, if there is a conflicting lock, and the F_GETLK caller

specified

Quoted 2 lines

F_RDLCKP or F_WRLCKP, might it make sense to report the l_type on the conflicting lock as F_RDLCKP or F_WRLCKP if that conflicting lock is also

a *P

Quoted 5 lines

type?

...or maybe we should consider a new F_GETLKP cmd value, and a new expanded struct flock that gives more info. The pid is already somewhat meaningless with this sort of lock. Perhaps we could obfuscate the

fl_owner

Quoted 2 lines

value and report that instead? What other sorts of info would be useful to programs that intend to use these new interfaces?

I always wonder if anyone actually uses the conflicting lock information...

I think returning the lock type as F_RDLCK or F_WRLCK is fine, but maybe an extended F_GETLKP could report the extended type. For Ganesha, we wouldn't use the extended type since the NFS protocol has no concept of separate private and non-private locks (all NFS locks are "private" to the specified owner in the request). The pid is also not that useful to Ganesha.

Due to some unfortunate history, POSIX locks have very strange and unhelpful semantics. The thing that usually catches people by surprise is that they are dropped whenever the process closes any file descriptor associated with the inode.

This is extremely problematic for people developing file servers that need to implement byte-range locks. Developers often need a "lock management" facility to ensure that file descriptors are not closed until all of the locks associated with the inode are finished.

Additionally, "classic" POSIX locks are owned by the process. Locks taken between threads within the same process won't conflict with one another, which renders classic POSIX locks useless for synchronization between threads.

This patchset adds a new type of lock that attempts to address these issues. These locks work just like classic POSIX read/write locks, but have semantics that are more like BSD locks with respect to inheritance and behavior on close.

This is implemented primarily by changing how fl_owner field is set for these locks. Instead of having them owned by the files_struct of the process, they are instead owned by the filp on which they were acquired. Thus, they are inherited across fork() and are only released when the last reference to a filp is put.

These new semantics prevent them from being merged with classic POSIX locks, even if they are acquired by the same process. These locks will also conflict with classic POSIX locks even if they are acquired by the same process or on the same file descriptor.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/locks.c                       | 32 ++++++++++++++++++++++++++++++--
 include/uapi/asm-generic/fcntl.h | 16 ++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

fs/locks.c +30 -2

index 5372ddd..98a503d 100644
@@ -367,7 +367,34 @@ flock_to_posix_lock_common(struct file_lock *fl, struct file *filp, short type)
 		break;
 	}
 
-	return assign_type(fl, type);
+	/*
+	 * FL_FILE_PVT locks are "owned" by the filp upon which they were
+	 * acquired, regardless of what task is dealing with them. Set the
+	 * fl_owner appropriately and flag them as private.
+	 */
+	switch(type) {
+	case F_RDLCKP:
+		fl->fl_owner = (fl_owner_t)fl->fl_file;
+		fl->fl_type = F_RDLCK;
+		fl->fl_flags |= FL_FILE_PVT;
+		break;
+	case F_WRLCKP:
+		fl->fl_owner = (fl_owner_t)fl->fl_file;
+		fl->fl_type = F_WRLCK;
+		fl->fl_flags |= FL_FILE_PVT;
+		break;
+	case F_UNLCKP:
+		fl->fl_owner = (fl_owner_t)fl->fl_file;
+		fl->fl_type = F_UNLCK;
+		fl->fl_flags |= FL_FILE_PVT;
+		break;
+	default:
+		/* Any other POSIX lock is owned by the file_struct */
+		fl->fl_owner = current->files;
+		return assign_type(fl, type);
+	}
+
+	return 0;
 }
 
 /* Verify a "struct flock" and copy it to a "struct file_lock" as a POSIX

@@ -2259,7 +2286,8 @@ void locks_remove_file(struct file *filp)
 
 	while ((fl = *before) != NULL) {
 		if (fl->fl_file == filp) {
-			if (IS_FLOCK(fl)) {
+			if (IS_FLOCK(fl) ||
+			    (IS_POSIX(fl) && IS_FILE_PVT(fl))) {
 				locks_delete_lock(before);
 				continue;
 			}

include/uapi/asm-generic/fcntl.h +9 -0

index 95e46c8..ed09fc5 100644
@@ -151,6 +151,22 @@ struct f_owner_ex {
 #define F_UNLCK		2
 #endif
 
+/*
+ * fd "private" POSIX locks.
+ *
+ * Usually POSIX locks held by a process are released on *any* close and are
+ * not inherited across a fork().
+ *
+ * These lock types will conflict with normal POSIX locks, but are "owned"
+ * by the opened file, not the process. This means that they are inherited
+ * across fork() like BSD (flock) locks, and they are only released

So, I think the above semantics are pretty clear, but now that I've had a go at sitting down to document this stuff for the POSIX spec and manpages, it's clear how convoluted the text in there is becoming.

That makes me wonder...would we be better off with a new set of cmd values here instead of new l_type values? IOW, we could add new:

F_GETLKP
F_SETLKP
F_SETLKPW

...and then just reuse the same F_RDLCK/F_WRLCK/F_UNLCK values? With that too, we could create a new equivalent to struct flock that has fixed length types instead of dealing with the off_t mess.

Quoted 10 lines · Jeff Layton

So, I think the above semantics are pretty clear, but now that I've had a go at sitting down to document this stuff for the POSIX spec and manpages, it's clear how convoluted the text in there is becoming.

That makes me wonder...would we be better off with a new set of cmd values here instead of new l_type values? IOW, we could add new:

F_GETLKP
F_SETLKP
F_SETLKPW

That seems a tad cleaner to me indeed.

Quoted 3 lines

...and then just reuse the same F_RDLCK/F_WRLCK/F_UNLCK values? With that too, we could create a new equivalent to struct flock that has fixed length types instead of dealing with the off_t mess.

For the Posix interface you'd need an off_t as that's what the whole API uses for file offsets. We could make sure to always use a off64_t for the kernel interface though.

What is the API you propose to posix? An new posix_lockf?

This patchset is the third posting of this set. Behavior between this set

the last should be more or less the same. Here is a summary of changes:

v3:
- more consolidation of common code between flock_to_posix_lock and flock_to_posix_lock64
- better bisectability by reordering changes, such that partial implementation won't be exposed
- s/filp/file/ s/FILP/FILE/ in symbol names

v2:
- inheritance semantics have been change to be more BSD-lock like
- patchset size has been reduced by changing how lock ownership is handled
- new F_UNLCKP l_type value has been added

Note too that I've gone ahead and opened a request for the POSIX folks to consider adding this to the POSIX spec once we have something mergeable. They seem amenable to the idea but don't want to enshrine it into the standard until there's a real implementation of it:

    http://austingroupbugs.net/view.php?id=768

I also owe them a better writeup of the semantics for these new locks, but have been holding off on doing that until they're a little more settled.

As a side note, I've also had a few other userland developers reach out to

as to the status of this work. There seems to be a lot of interest since

POSIX locks are such a pain to work with in threaded programs. Hopefully some will chime in on this posting...

Original cover letter from v1 posting follows. Comments and suggestions welcome.

-------------------------------[snip]------------------------------

At LSF this year, there was a discussion about the "wishlist" for userland

servers. One of the things brought up was the goofy and problematic behavior of POSIX locks when a file is closed. Boaz started a thread on it

    http://permalink.gmane.org/gmane.linux.file-systems/73364

Userland fileservers often need to maintain more than one open file descriptor on a file. The POSIX spec says:

"All locks associated with a file for a given process shall be removed

file descriptor for that file is closed by that process or the process

that file descriptor terminates."

This is problematic since you can't close any file descriptor without

all your POSIX locks. Most userland file servers therefore end up opening

file with more access than is really necessary, and keeping fd's open for longer than is necessary to work around this.

This patchset is a first stab at an approach to address this problem by

two new l_type values -- F_RDLCKP and F_WRLCKP (the 'P' is short for "private" -- I'm open to changing that if you have a better mnemonic).

For all intents and purposes these lock types act just like their "non-P" counterpart. The difference is that they are only implicitly released when

fd against which they were acquired is closed. As a side effect, these

cannot be merged with "non-P" locks since they have different semantics on close.

I've given this patchset some very basic smoke testing and it seems to do

right thing, but it is still pretty rough. If this looks reasonable I'll

some documentation updates and will take a stab at trying to get these new lock types added to the POSIX spec (as HCH recommended).

At this point, my main questions are:

1) does this look useful, particularly for fileserver implementors?

2) does this look OK API-wise? We could consider different "cmd" values or even different syscalls, but I figured this makes it clearer that "P" and "non-P" locks will still conflict with one another.

Jeff Layton (6):
locks: consolidate common code in the flock_to_posix_lock routines
locks: consolidate checks for compatible filp->f_mode values in setlk
handlers
locks: rename locks_remove_flock to locks_remove_file
locks: show private lock types in /proc/locks
locks: report l_pid as -1 for FL_FILE_PVT locks
locks: add new "private" lock type that is owned by the filp

 fs/file_table.c                  |   2 +-
 fs/locks.c                       | 122

 include/linux/fs.h               |   5 +-
 include/uapi/asm-generic/fcntl.h |  16 +++++
 4 files changed, 85 insertions(+), 60 deletions(-)

--
1.8.4.2

Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clk
trk
_______________________________________________
Nfs-ganesha-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel