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

Jeff Layton jlayton at redhat.com
Thu Dec 12 03:44:06 MST 2013


On Wed, 11 Dec 2013 17:56:16 -0500
"J. Bruce Fields" <bfields at fieldses.org> wrote:

> On Wed, Dec 11, 2013 at 02:07:41PM -0500, Jeff Layton wrote:
> > On Wed, 11 Dec 2013 10:19:31 -0500
> > "J. Bruce Fields" <bfields at fieldses.org> wrote:
> ...
> > > +	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.
> 
> 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 <bfields at redhat.com>
> 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 <bfields at redhat.com>
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 92a0f0a..39f2ca9 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -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;
>  	}
> +	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)
> +		return -EOVERFLOW;
> +	if (fl->fl_start + l->l_len < 0)
> +		return -EINVAL;
>  


Well spotted. The change looks correct to me.

>  	/* 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
>  
> 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 */


-- 
Jeff Layton <jlayton at redhat.com>


More information about the samba-technical mailing list