Re: [PATCH v3 1/6] locks: consolidate common code in theflock_to_posix_lock routines
From: Jeff Layton
Date: Wed Dec 11 2013 - 11:52:02 EST
On Wed, 11 Dec 2013 10:19:31 -0500
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Wed, Dec 11, 2013 at 09:37:24AM -0500, J. Bruce Fields wrote:
> > 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 <bfields@xxxxxxxxxx>
> 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 <bfields@xxxxxxxxxx>
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 92a0f0a..b70486a 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -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
>
> 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 */
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 :).
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/