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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
