Re: [PATCH v1 net-next 01/13] fs/lock: Revive LOCK_MAND.

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2022-08-25 at 17:04 -0700, Kuniyuki Iwashima wrote:
> The commit 90f7d7a0d0d6 ("locks: remove LOCK_MAND flock lock support")
> removed LOCK_MAND support from the kernel because nothing checked the
> flag, nor was there no use case.  This patch revives LOCK_MAND to
> introduce a mandatory lock for read/write on /proc/sys.  Currently, it's
> the only use case, so we added two changes while reverting the commit.
> 
> First, we used to allow any f_mode for LOCK_MAND, but now we don't get
> it back.  Instead, we enforce being FMODE_READ|FMODE_WRITE as LOCK_SH
> and LOCK_EX.
> 
> Second, when f_ops->flock() was called with LOCK_MAND, each function
> returned -EOPNOTSUPP.  The following patch does not use f_ops->flock(),
> so we put the validation before calling f_ops->flock().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  fs/locks.c                       | 57 ++++++++++++++++++++------------
>  include/uapi/asm-generic/fcntl.h |  5 ---
>  2 files changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index c266cfdc3291..03ff10a3165e 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -421,6 +421,10 @@ static inline int flock_translate_cmd(int cmd) {
>  	case LOCK_UN:
>  		return F_UNLCK;
>  	}
> +
> +	if (cmd & LOCK_MAND)
> +		return cmd & (LOCK_MAND | LOCK_RW);
> +
>  	return -EINVAL;
>  }
>  
> @@ -879,6 +883,10 @@ static bool flock_locks_conflict(struct file_lock *caller_fl,
>  	if (caller_fl->fl_file == sys_fl->fl_file)
>  		return false;
>  
> +	if (caller_fl->fl_type & LOCK_MAND ||
> +	    sys_fl->fl_type & LOCK_MAND)
> +		return true;
> +
>  	return locks_conflict(caller_fl, sys_fl);
>  }
>  
> @@ -2077,9 +2085,7 @@ EXPORT_SYMBOL(locks_lock_inode_wait);
>   *	- %LOCK_SH -- a shared lock.
>   *	- %LOCK_EX -- an exclusive lock.
>   *	- %LOCK_UN -- remove an existing lock.
> - *	- %LOCK_MAND -- a 'mandatory' flock. (DEPRECATED)
> - *
> - *	%LOCK_MAND support has been removed from the kernel.
> + *	- %LOCK_MAND -- a 'mandatory' flock. (only supported on /proc/sys/)
>   */
>  SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  {
> @@ -2087,19 +2093,6 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  	struct file_lock fl;
>  	struct fd f;
>  
> -	/*
> -	 * LOCK_MAND locks were broken for a long time in that they never
> -	 * conflicted with one another and didn't prevent any sort of open,
> -	 * read or write activity.
> -	 *
> -	 * Just ignore these requests now, to preserve legacy behavior, but
> -	 * throw a warning to let people know that they don't actually work.
> -	 */
> -	if (cmd & LOCK_MAND) {
> -		pr_warn_once("Attempt to set a LOCK_MAND lock via flock(2). This support has been removed and the request ignored.\n");
> -		return 0;
> -	}
> -
>  	type = flock_translate_cmd(cmd & ~LOCK_NB);
>  	if (type < 0)
>  		return type;
> @@ -2109,6 +2102,7 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  	if (!f.file)
>  		return error;
>  
> +	/* LOCK_MAND supports only read/write on proc_sysctl for now */
>  	if (type != F_UNLCK && !(f.file->f_mode & (FMODE_READ | FMODE_WRITE)))
>  		goto out_putf;
>  
> @@ -2122,12 +2116,18 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  	if (can_sleep)
>  		fl.fl_flags |= FL_SLEEP;
>  
> -	if (f.file->f_op->flock)
> +	if (f.file->f_op->flock) {
> +		if (cmd & LOCK_MAND) {
> +			error = -EOPNOTSUPP;
> +			goto out_putf;
> +		}
> +
>  		error = f.file->f_op->flock(f.file,
>  					    (can_sleep) ? F_SETLKW : F_SETLK,
>  					    &fl);
> -	else
> +	} else {
>  		error = locks_lock_file_wait(f.file, &fl);
> +	}
>  
>   out_putf:
>  	fdput(f);
> @@ -2711,7 +2711,11 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  		seq_printf(f, " %s ",
>  			     (inode == NULL) ? "*NOINODE*" : "ADVISORY ");
>  	} else if (IS_FLOCK(fl)) {
> -		seq_puts(f, "FLOCK  ADVISORY  ");
> +		if (fl->fl_type & LOCK_MAND) {
> +			seq_puts(f, "FLOCK  MANDATORY ");
> +		} else {
> +			seq_puts(f, "FLOCK  ADVISORY  ");
> +		}
>  	} else if (IS_LEASE(fl)) {
>  		if (fl->fl_flags & FL_DELEG)
>  			seq_puts(f, "DELEG  ");
> @@ -2727,10 +2731,19 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  	} else {
>  		seq_puts(f, "UNKNOWN UNKNOWN  ");
>  	}
> -	type = IS_LEASE(fl) ? target_leasetype(fl) : fl->fl_type;
>  
> -	seq_printf(f, "%s ", (type == F_WRLCK) ? "WRITE" :
> -			     (type == F_RDLCK) ? "READ" : "UNLCK");
> +	if (fl->fl_type & LOCK_MAND) {
> +		seq_printf(f, "%s ",
> +			   (fl->fl_type & LOCK_READ)
> +			   ? (fl->fl_type & LOCK_WRITE) ? "RW   " : "READ "
> +			   : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE ");
> +	} else {
> +		type = IS_LEASE(fl) ? target_leasetype(fl) : fl->fl_type;
> +
> +		seq_printf(f, "%s ", (type == F_WRLCK) ? "WRITE" :
> +			   (type == F_RDLCK) ? "READ" : "UNLCK");
> +	}
> +
>  	if (inode) {
>  		/* userspace relies on this representation of dev_t */
>  		seq_printf(f, "%d %02x:%02x:%lu ", fl_pid,
> diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> index 1ecdb911add8..94fb8c6fd543 100644
> --- a/include/uapi/asm-generic/fcntl.h
> +++ b/include/uapi/asm-generic/fcntl.h
> @@ -180,11 +180,6 @@ struct f_owner_ex {
>  #define LOCK_NB		4	/* or'd with one of the above to prevent
>  				   blocking */
>  #define LOCK_UN		8	/* remove lock */
> -
> -/*
> - * LOCK_MAND support has been removed from the kernel. We leave the symbols
> - * here to not break legacy builds, but these should not be used in new code.
> - */
>  #define LOCK_MAND	32	/* This is a mandatory flock ... */
>  #define LOCK_READ	64	/* which allows concurrent read operations */
>  #define LOCK_WRITE	128	/* which allows concurrent write operations */

NACK.

This may break legacy userland code that sets LOCK_MAND on flock calls
(e.g. old versions of samba).

If you want to add a new mechanism that does something similar with a
new flag, then that may be possible, but please don't overload old flags
that could still be used in the field with new meanings.

If you do decide to use flock for this functionality (and I'm not sure
this is a good idea), then I'd also like to see a clear description of
the semantics this provides.
-- 
Jeff Layton <jlayton@kernel.org>