| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220826164802.95813-1-kuniyu@amazon.com>
Date: Fri, 26 Aug 2022 09:48:02 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <jlayton@...nel.org>
CC: <chuck.lever@...cle.com>, <davem@...emloft.net>,
<edumazet@...gle.com>, <keescook@...omium.org>, <kuba@...nel.org>,
<kuni1840@...il.com>, <kuniyu@...zon.com>,
<linux-fsdevel@...r.kernel.org>, <mcgrof@...nel.org>,
<netdev@...r.kernel.org>, <pabeni@...hat.com>, <yzaikin@...gle.com>
Subject: Re: [PATCH v1 net-next 01/13] fs/lock: Revive LOCK_MAND.
From: Jeff Layton <jlayton@...nel.org>
Date: Fri, 26 Aug 2022 06:02:44 -0400
> 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@...zon.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.
Exactly, that makes sense.
Thanks for feedback!
> If you do decide to use flock for this functionality (and I'm not sure
> this is a good idea),
Actually, the patch 1-2 were experimental to show all available options
(flock()'s latency vs unshare()'s memory cost), and I like unshare().
If both of them were unacceptable, I would have added clone() BPF hook.
But it seems unshare() works at least, I'll drop this part in the next
spin.
Thank you.
> then I'd also like to see a clear description of
> the semantics this provides.
> --
> Jeff Layton <jlayton@...nel.org>
Powered by blists - more mailing lists