[PATCH v2] locks: eliminate false positive conflicts for write lease

[PATCH v2] locks: eliminate false positive conflicts for write lease

[Amir Goldstein]

check_conflicting_open() is checking for existing fd's open for read or
for write before allowing to take a write lease.  The check that was
implemented using i_count and d_count is an approximation that has
several false positives.  For example, overlayfs since v4.19, takes an
extra reference on the dentry; An open with O_PATH takes a reference on
the dentry although the file cannot be read nor written.

Change the implementation to use i_readcount and i_writecount to
eliminate the false positive conflicts and allow a write lease to be
taken on an overlayfs file.

The change of behavior with existing fd's open with O_PATH is symmetric
w.r.t. current behavior of lease breakers - an open with O_PATH currently
does not break a write lease.

This increases the size of struct inode by 4 bytes on 32bit archs when
CONFIG_FILE_LOCKING is defined and CONFIG_IMA was not already
defined.

Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos, Jeff and Bruce,

This patch fixes a v4.19 overlayfs regression with taking write
leases. It also provides correct semantics w.r.t RDONLY open counter
that Bruce also needed for nfsd.

Since this is locks code that fixes an overlayfs regression which
is also needed for nfsd, it could go via either of your trees.
I didn't want to pick sides, so first one to grab the patch wins ;-)

I verified the changes using modified LTP F_SETLEASE tests [1],
which I ran over xfs and overlayfs.

Thanks,
Amir.

[1] https://github.com/amir73il/ltp/commits/overlayfs-devel

Changes since v1:
- Drop patch to fold i_readcount into i_count
- Make i_readcount depend on CONFIG_FILE_LOCKING

 fs/locks.c         | 33 ++++++++++++++++++++++-----------
 include/linux/fs.h |  4 ++--
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index ec1e4a5df629..28528b4fc53b 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1753,10 +1753,10 @@ int fcntl_getlease(struct file *filp)
 }
 
 /**
- * check_conflicting_open - see if the given dentry points to a file that has
+ * check_conflicting_open - see if the given file points to an inode that has
  *			    an existing open that would conflict with the
  *			    desired lease.
- * @dentry:	dentry to check
+ * @filp:	file to check
  * @arg:	type of lease that we're trying to acquire
  * @flags:	current lock flags
  *
@@ -1764,19 +1764,31 @@ int fcntl_getlease(struct file *filp)
  * conflict with the lease we're trying to set.
  */
 static int
-check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
+check_conflicting_open(struct file *filp, const long arg, int flags)
 {
 	int ret = 0;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = locks_inode(filp);
+	int wcount = atomic_read(&inode->i_writecount);
+	int self_wcount = 0, self_rcount = 0;
 
 	if (flags & FL_LAYOUT)
 		return 0;
 
-	if ((arg == F_RDLCK) && inode_is_open_for_write(inode))
+	if (arg == F_RDLCK && wcount > 0)
 		return -EAGAIN;
 
-	if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
-	    (atomic_read(&inode->i_count) > 1)))
+	/* Eliminate deny writes from actual writers count */
+	if (wcount < 0)
+		wcount = 0;
+
+	/* Make sure that only read/write count is from lease requestor */
+	if (filp->f_mode & FMODE_WRITE)
+		self_wcount = 1;
+	else if ((filp->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		self_rcount = 1;
+
+	if (arg == F_WRLCK && (wcount != self_wcount ||
+	    atomic_read(&inode->i_readcount) != self_rcount))
 		ret = -EAGAIN;
 
 	return ret;
@@ -1786,8 +1798,7 @@ static int
 generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
 {
 	struct file_lock *fl, *my_fl = NULL, *lease;
-	struct dentry *dentry = filp->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = locks_inode(filp);
 	struct file_lock_context *ctx;
 	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
 	int error;
@@ -1822,7 +1833,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	percpu_down_read(&file_rwsem);
 	spin_lock(&ctx->flc_lock);
 	time_out_leases(inode, &dispose);
-	error = check_conflicting_open(dentry, arg, lease->fl_flags);
+	error = check_conflicting_open(filp, arg, lease->fl_flags);
 	if (error)
 		goto out;
 
@@ -1879,7 +1890,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	 * precedes these checks.
 	 */
 	smp_mb();
-	error = check_conflicting_open(dentry, arg, lease->fl_flags);
+	error = check_conflicting_open(filp, arg, lease->fl_flags);
 	if (error) {
 		locks_unlink_lock_ctx(lease);
 		goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 79ffa2958bd8..2d55f1b64014 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -694,7 +694,7 @@ struct inode {
 	atomic_t		i_count;
 	atomic_t		i_dio_count;
 	atomic_t		i_writecount;
-#ifdef CONFIG_IMA
+#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
 	atomic_t		i_readcount; /* struct files open RO */
 #endif
 	union {
@@ -2895,7 +2895,7 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
 	return atomic_read(&inode->i_writecount) > 0;
 }
 
-#ifdef CONFIG_IMA
+#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
 static inline void i_readcount_dec(struct inode *inode)
 {
 	BUG_ON(!atomic_read(&inode->i_readcount));
-- 
2.17.1

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[J . Bruce Fields]

How do opens for execute work?  I guess they create a struct file with
FMODE_EXEC and FMODE_RDONLY set and they decrement i_writecount.  Do
they also increment i_readcount?  Reading do_open_execat and alloc_file,
looks like it does, so, good, they should conflict with write leases,
which sounds right.

Looks good to me.--b.

On Wed, Jun 12, 2019 at 08:24:08PM +0300, Amir Goldstein wrote:
> check_conflicting_open() is checking for existing fd's open for read or
> for write before allowing to take a write lease.  The check that was
> implemented using i_count and d_count is an approximation that has
> several false positives.  For example, overlayfs since v4.19, takes an
> extra reference on the dentry; An open with O_PATH takes a reference on
> the dentry although the file cannot be read nor written.
> 
> Change the implementation to use i_readcount and i_writecount to
> eliminate the false positive conflicts and allow a write lease to be
> taken on an overlayfs file.
> 
> The change of behavior with existing fd's open with O_PATH is symmetric
> w.r.t. current behavior of lease breakers - an open with O_PATH currently
> does not break a write lease.
> 
> This increases the size of struct inode by 4 bytes on 32bit archs when
> CONFIG_FILE_LOCKING is defined and CONFIG_IMA was not already
> defined.
> 
> Cc: <stable@vger.kernel.org> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Miklos, Jeff and Bruce,
> 
> This patch fixes a v4.19 overlayfs regression with taking write
> leases. It also provides correct semantics w.r.t RDONLY open counter
> that Bruce also needed for nfsd.
> 
> Since this is locks code that fixes an overlayfs regression which
> is also needed for nfsd, it could go via either of your trees.
> I didn't want to pick sides, so first one to grab the patch wins ;-)
> 
> I verified the changes using modified LTP F_SETLEASE tests [1],
> which I ran over xfs and overlayfs.
> 
> Thanks,
> Amir.
> 
> [1] https://github.com/amir73il/ltp/commits/overlayfs-devel
> 
> Changes since v1:
> - Drop patch to fold i_readcount into i_count
> - Make i_readcount depend on CONFIG_FILE_LOCKING
> 
>  fs/locks.c         | 33 ++++++++++++++++++++++-----------
>  include/linux/fs.h |  4 ++--
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index ec1e4a5df629..28528b4fc53b 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1753,10 +1753,10 @@ int fcntl_getlease(struct file *filp)
>  }
>  
>  /**
> - * check_conflicting_open - see if the given dentry points to a file that has
> + * check_conflicting_open - see if the given file points to an inode that has
>   *			    an existing open that would conflict with the
>   *			    desired lease.
> - * @dentry:	dentry to check
> + * @filp:	file to check
>   * @arg:	type of lease that we're trying to acquire
>   * @flags:	current lock flags
>   *
> @@ -1764,19 +1764,31 @@ int fcntl_getlease(struct file *filp)
>   * conflict with the lease we're trying to set.
>   */
>  static int
> -check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
> +check_conflicting_open(struct file *filp, const long arg, int flags)
>  {
>  	int ret = 0;
> -	struct inode *inode = dentry->d_inode;
> +	struct inode *inode = locks_inode(filp);
> +	int wcount = atomic_read(&inode->i_writecount);
> +	int self_wcount = 0, self_rcount = 0;
>  
>  	if (flags & FL_LAYOUT)
>  		return 0;
>  
> -	if ((arg == F_RDLCK) && inode_is_open_for_write(inode))
> +	if (arg == F_RDLCK && wcount > 0)
>  		return -EAGAIN;
>  
> -	if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
> -	    (atomic_read(&inode->i_count) > 1)))
> +	/* Eliminate deny writes from actual writers count */
> +	if (wcount < 0)
> +		wcount = 0;
> +
> +	/* Make sure that only read/write count is from lease requestor */
> +	if (filp->f_mode & FMODE_WRITE)
> +		self_wcount = 1;
> +	else if ((filp->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
> +		self_rcount = 1;
> +
> +	if (arg == F_WRLCK && (wcount != self_wcount ||
> +	    atomic_read(&inode->i_readcount) != self_rcount))
>  		ret = -EAGAIN;
>  
>  	return ret;
> @@ -1786,8 +1798,7 @@ static int
>  generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
>  {
>  	struct file_lock *fl, *my_fl = NULL, *lease;
> -	struct dentry *dentry = filp->f_path.dentry;
> -	struct inode *inode = dentry->d_inode;
> +	struct inode *inode = locks_inode(filp);
>  	struct file_lock_context *ctx;
>  	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
>  	int error;
> @@ -1822,7 +1833,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>  	percpu_down_read(&file_rwsem);
>  	spin_lock(&ctx->flc_lock);
>  	time_out_leases(inode, &dispose);
> -	error = check_conflicting_open(dentry, arg, lease->fl_flags);
> +	error = check_conflicting_open(filp, arg, lease->fl_flags);
>  	if (error)
>  		goto out;
>  
> @@ -1879,7 +1890,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>  	 * precedes these checks.
>  	 */
>  	smp_mb();
> -	error = check_conflicting_open(dentry, arg, lease->fl_flags);
> +	error = check_conflicting_open(filp, arg, lease->fl_flags);
>  	if (error) {
>  		locks_unlink_lock_ctx(lease);
>  		goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 79ffa2958bd8..2d55f1b64014 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -694,7 +694,7 @@ struct inode {
>  	atomic_t		i_count;
>  	atomic_t		i_dio_count;
>  	atomic_t		i_writecount;
> -#ifdef CONFIG_IMA
> +#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
>  	atomic_t		i_readcount; /* struct files open RO */
>  #endif
>  	union {
> @@ -2895,7 +2895,7 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
>  	return atomic_read(&inode->i_writecount) > 0;
>  }
>  
> -#ifdef CONFIG_IMA
> +#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
>  static inline void i_readcount_dec(struct inode *inode)
>  {
>  	BUG_ON(!atomic_read(&inode->i_readcount));
> -- 
> 2.17.1

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Miklos Szeredi]

On Wed, Jun 12, 2019 at 8:31 PM J . Bruce Fields <bfields@fieldses.org> wrote:
>
> How do opens for execute work?  I guess they create a struct file with
> FMODE_EXEC and FMODE_RDONLY set and they decrement i_writecount.  Do
> they also increment i_readcount?  Reading do_open_execat and alloc_file,
> looks like it does, so, good, they should conflict with write leases,
> which sounds right.

Right, but then why this:

> > +     /* Eliminate deny writes from actual writers count */
> > +     if (wcount < 0)
> > +             wcount = 0;

It's basically a no-op, as you say.  And it doesn't make any sense
logically, since denying writes *should* deny write leases as well...

Thanks,
Miklos

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[J . Bruce Fields]

On Thu, Jun 13, 2019 at 04:13:15PM +0200, Miklos Szeredi wrote:
> On Wed, Jun 12, 2019 at 8:31 PM J . Bruce Fields <bfields@fieldses.org> wrote:
> >
> > How do opens for execute work?  I guess they create a struct file with
> > FMODE_EXEC and FMODE_RDONLY set and they decrement i_writecount.  Do
> > they also increment i_readcount?  Reading do_open_execat and alloc_file,
> > looks like it does, so, good, they should conflict with write leases,
> > which sounds right.
> 
> Right, but then why this:
> 
> > > +     /* Eliminate deny writes from actual writers count */
> > > +     if (wcount < 0)
> > > +             wcount = 0;
> 
> It's basically a no-op, as you say.  And it doesn't make any sense
> logically, since denying writes *should* deny write leases as well...

Yes.  I feel like the negative writecount case is a little nonobvious,
so maybe replace that by a comment, something like this?:

--b.

diff --git a/fs/locks.c b/fs/locks.c
index 2056595751e8..379829b913c1 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1772,11 +1772,12 @@ check_conflicting_open(struct file *filp, const long arg, int flags)
 	if (arg == F_RDLCK && wcount > 0)
 		return -EAGAIN;
 
-	/* Eliminate deny writes from actual writers count */
-	if (wcount < 0)
-		wcount = 0;
-
-	/* Make sure that only read/write count is from lease requestor */
+	/*
+	 * Make sure that only read/write count is from lease requestor.
+	 * Note that this will result in denying write leases when wcount
+	 * is negative, which is what we want.  (We shouldn't grant
+	 * write leases on files open for execution.)
+	 */
 	if (filp->f_mode & FMODE_WRITE)
 		self_wcount = 1;
 	else if (filp->f_mode & FMODE_READ)

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

On Thu, Jun 13, 2019 at 5:32 PM J . Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Jun 13, 2019 at 04:13:15PM +0200, Miklos Szeredi wrote:
> > On Wed, Jun 12, 2019 at 8:31 PM J . Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > How do opens for execute work?  I guess they create a struct file with
> > > FMODE_EXEC and FMODE_RDONLY set and they decrement i_writecount.  Do
> > > they also increment i_readcount?  Reading do_open_execat and alloc_file,
> > > looks like it does, so, good, they should conflict with write leases,
> > > which sounds right.
> >
> > Right, but then why this:
> >
> > > > +     /* Eliminate deny writes from actual writers count */
> > > > +     if (wcount < 0)
> > > > +             wcount = 0;
> >
> > It's basically a no-op, as you say.  And it doesn't make any sense
> > logically, since denying writes *should* deny write leases as well...
>
> Yes.  I feel like the negative writecount case is a little nonobvious,
> so maybe replace that by a comment, something like this?:
>
> --b.
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 2056595751e8..379829b913c1 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1772,11 +1772,12 @@ check_conflicting_open(struct file *filp, const long arg, int flags)
>         if (arg == F_RDLCK && wcount > 0)
>                 return -EAGAIN;
>
> -       /* Eliminate deny writes from actual writers count */
> -       if (wcount < 0)
> -               wcount = 0;
> -
> -       /* Make sure that only read/write count is from lease requestor */
> +       /*
> +        * Make sure that only read/write count is from lease requestor.
> +        * Note that this will result in denying write leases when wcount
> +        * is negative, which is what we want.  (We shouldn't grant
> +        * write leases on files open for execution.)
> +        */
>         if (filp->f_mode & FMODE_WRITE)
>                 self_wcount = 1;
>         else if (filp->f_mode & FMODE_READ)

I'm fine with targeting 5.3 and I'm fine with all suggested changes
and adding some of my own. At this point we no longer need wcount
variable and code becomes more readable without it.
See attached patch (also tested).

Thanks,
Amir.

[-- Attachment #2: v3-0001-locks-eliminate-false-positive-conflicts-for-writ.patch.txt --]
[-- Type: text/plain, Size: 4980 bytes --]

From d1b029a778c9100bf1a295c7035319913ef25333 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 7 Jun 2019 17:24:38 +0300
Subject: [PATCH v3] locks: eliminate false positive conflicts for write lease

check_conflicting_open() is checking for existing fd's open for read or
for write before allowing to take a write lease.  The check that was
implemented using i_count and d_count is an approximation that has
several false positives.  For example, overlayfs since v4.19, takes an
extra reference on the dentry; An open with O_PATH takes a reference on
the dentry although the file cannot be read nor written.

Change the implementation to use i_readcount and i_writecount to
eliminate the false positive conflicts and allow a write lease to be
taken on an overlayfs file.

The change of behavior with existing fd's open with O_PATH is symmetric
w.r.t. current behavior of lease breakers - an open with O_PATH currently
does not break a write lease.

This increases the size of struct inode by 4 bytes on 32bit archs when
CONFIG_FILE_LOCKING is defined and CONFIG_IMA was not already
defined.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/locks.c         | 37 +++++++++++++++++++++++++------------
 include/linux/fs.h |  4 ++--
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index ec1e4a5df629..633512100842 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1753,10 +1753,10 @@ int fcntl_getlease(struct file *filp)
 }
 
 /**
- * check_conflicting_open - see if the given dentry points to a file that has
+ * check_conflicting_open - see if the given file points to an inode that has
  *			    an existing open that would conflict with the
  *			    desired lease.
- * @dentry:	dentry to check
+ * @filp:	file to check
  * @arg:	type of lease that we're trying to acquire
  * @flags:	current lock flags
  *
@@ -1764,19 +1764,33 @@ int fcntl_getlease(struct file *filp)
  * conflict with the lease we're trying to set.
  */
 static int
-check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
+check_conflicting_open(struct file *filp, const long arg, int flags)
 {
 	int ret = 0;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = locks_inode(filp);
+	int self_wcount = 0, self_rcount = 0;
 
 	if (flags & FL_LAYOUT)
 		return 0;
 
-	if ((arg == F_RDLCK) && inode_is_open_for_write(inode))
-		return -EAGAIN;
+	if (arg == F_RDLCK)
+		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
+	else if (arg != F_WRLCK)
+		return 0;
 
-	if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
-	    (atomic_read(&inode->i_count) > 1)))
+	/*
+	 * Make sure that only read/write count is from lease requestor.
+	 * Note that this will result in denying write leases when i_writecount
+	 * is negative, which is what we want.  (We shouldn't grant write leases
+	 * on files open for execution.)
+	 */
+	if (filp->f_mode & FMODE_WRITE)
+		self_wcount = 1;
+	else if (filp->f_mode & FMODE_READ)
+		self_rcount = 1;
+
+	if (atomic_read(&inode->i_writecount) != self_wcount ||
+	    atomic_read(&inode->i_readcount) != self_rcount)
 		ret = -EAGAIN;
 
 	return ret;
@@ -1786,8 +1800,7 @@ static int
 generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
 {
 	struct file_lock *fl, *my_fl = NULL, *lease;
-	struct dentry *dentry = filp->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = locks_inode(filp);
 	struct file_lock_context *ctx;
 	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
 	int error;
@@ -1822,7 +1835,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	percpu_down_read(&file_rwsem);
 	spin_lock(&ctx->flc_lock);
 	time_out_leases(inode, &dispose);
-	error = check_conflicting_open(dentry, arg, lease->fl_flags);
+	error = check_conflicting_open(filp, arg, lease->fl_flags);
 	if (error)
 		goto out;
 
@@ -1879,7 +1892,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	 * precedes these checks.
 	 */
 	smp_mb();
-	error = check_conflicting_open(dentry, arg, lease->fl_flags);
+	error = check_conflicting_open(filp, arg, lease->fl_flags);
 	if (error) {
 		locks_unlink_lock_ctx(lease);
 		goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 79ffa2958bd8..2d55f1b64014 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -694,7 +694,7 @@ struct inode {
 	atomic_t		i_count;
 	atomic_t		i_dio_count;
 	atomic_t		i_writecount;
-#ifdef CONFIG_IMA
+#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
 	atomic_t		i_readcount; /* struct files open RO */
 #endif
 	union {
@@ -2895,7 +2895,7 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
 	return atomic_read(&inode->i_writecount) > 0;
 }
 
-#ifdef CONFIG_IMA
+#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
 static inline void i_readcount_dec(struct inode *inode)
 {
 	BUG_ON(!atomic_read(&inode->i_readcount));
-- 
2.17.1

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Jeff Layton]

On Thu, 2019-06-13 at 18:47 +0300, Amir Goldstein wrote:
> On Thu, Jun 13, 2019 at 5:32 PM J . Bruce Fields <bfields@fieldses.org> wrote:
> > On Thu, Jun 13, 2019 at 04:13:15PM +0200, Miklos Szeredi wrote:
> > > On Wed, Jun 12, 2019 at 8:31 PM J . Bruce Fields <bfields@fieldses.org> wrote:
> > > > How do opens for execute work?  I guess they create a struct file with
> > > > FMODE_EXEC and FMODE_RDONLY set and they decrement i_writecount.  Do
> > > > they also increment i_readcount?  Reading do_open_execat and alloc_file,
> > > > looks like it does, so, good, they should conflict with write leases,
> > > > which sounds right.
> > > 
> > > Right, but then why this:
> > > 
> > > > > +     /* Eliminate deny writes from actual writers count */
> > > > > +     if (wcount < 0)
> > > > > +             wcount = 0;
> > > 
> > > It's basically a no-op, as you say.  And it doesn't make any sense
> > > logically, since denying writes *should* deny write leases as well...
> > 
> > Yes.  I feel like the negative writecount case is a little nonobvious,
> > so maybe replace that by a comment, something like this?:
> > 
> > --b.
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 2056595751e8..379829b913c1 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -1772,11 +1772,12 @@ check_conflicting_open(struct file *filp, const long arg, int flags)
> >         if (arg == F_RDLCK && wcount > 0)
> >                 return -EAGAIN;
> > 
> > -       /* Eliminate deny writes from actual writers count */
> > -       if (wcount < 0)
> > -               wcount = 0;
> > -
> > -       /* Make sure that only read/write count is from lease requestor */
> > +       /*
> > +        * Make sure that only read/write count is from lease requestor.
> > +        * Note that this will result in denying write leases when wcount
> > +        * is negative, which is what we want.  (We shouldn't grant
> > +        * write leases on files open for execution.)
> > +        */
> >         if (filp->f_mode & FMODE_WRITE)
> >                 self_wcount = 1;
> >         else if (filp->f_mode & FMODE_READ)
> 
> I'm fine with targeting 5.3 and I'm fine with all suggested changes
> and adding some of my own. At this point we no longer need wcount
> variable and code becomes more readable without it.
> See attached patch (also tested).
> 
> Thanks,
> Amir.

Thanks Amir. In that case, I'll go ahead and pick this up for v5.3, and
will get it into linux-next soon.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

> > I'm fine with targeting 5.3 and I'm fine with all suggested changes
> > and adding some of my own. At this point we no longer need wcount
> > variable and code becomes more readable without it.
> > See attached patch (also tested).
> >
> > Thanks,
> > Amir.
>
> Thanks Amir. In that case, I'll go ahead and pick this up for v5.3, and
> will get it into linux-next soon.
>

While we are polishing the patch to perfection, could also get rid of
ret variable...
Not a must.

Thanks,
Amir.

[-- Attachment #2: v4-0001-locks-eliminate-false-positive-conflicts-for-writ.patch.txt --]
[-- Type: text/plain, Size: 5000 bytes --]

From 1cebe862fab4b4eab05353b20cea2441fe1787ca Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 7 Jun 2019 17:24:38 +0300
Subject: [PATCH v4] locks: eliminate false positive conflicts for write lease

check_conflicting_open() is checking for existing fd's open for read or
for write before allowing to take a write lease.  The check that was
implemented using i_count and d_count is an approximation that has
several false positives.  For example, overlayfs since v4.19, takes an
extra reference on the dentry; An open with O_PATH takes a reference on
the dentry although the file cannot be read nor written.

Change the implementation to use i_readcount and i_writecount to
eliminate the false positive conflicts and allow a write lease to be
taken on an overlayfs file.

The change of behavior with existing fd's open with O_PATH is symmetric
w.r.t. current behavior of lease breakers - an open with O_PATH currently
does not break a write lease.

This increases the size of struct inode by 4 bytes on 32bit archs when
CONFIG_FILE_LOCKING is defined and CONFIG_IMA was not already
defined.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/locks.c         | 42 +++++++++++++++++++++++++++---------------
 include/linux/fs.h |  4 ++--
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index ec1e4a5df629..2e86dff71f3a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1753,10 +1753,10 @@ int fcntl_getlease(struct file *filp)
 }
 
 /**
- * check_conflicting_open - see if the given dentry points to a file that has
+ * check_conflicting_open - see if the given file points to an inode that has
  *			    an existing open that would conflict with the
  *			    desired lease.
- * @dentry:	dentry to check
+ * @filp:	file to check
  * @arg:	type of lease that we're trying to acquire
  * @flags:	current lock flags
  *
@@ -1764,30 +1764,42 @@ int fcntl_getlease(struct file *filp)
  * conflict with the lease we're trying to set.
  */
 static int
-check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
+check_conflicting_open(struct file *filp, const long arg, int flags)
 {
-	int ret = 0;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = locks_inode(filp);
+	int self_wcount = 0, self_rcount = 0;
 
 	if (flags & FL_LAYOUT)
 		return 0;
 
-	if ((arg == F_RDLCK) && inode_is_open_for_write(inode))
-		return -EAGAIN;
+	if (arg == F_RDLCK)
+		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
+	else if (arg != F_WRLCK)
+		return 0;
+
+	/*
+	 * Make sure that only read/write count is from lease requestor.
+	 * Note that this will result in denying write leases when i_writecount
+	 * is negative, which is what we want.  (We shouldn't grant write leases
+	 * on files open for execution.)
+	 */
+	if (filp->f_mode & FMODE_WRITE)
+		self_wcount = 1;
+	else if (filp->f_mode & FMODE_READ)
+		self_rcount = 1;
 
-	if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
-	    (atomic_read(&inode->i_count) > 1)))
-		ret = -EAGAIN;
+	if (atomic_read(&inode->i_writecount) != self_wcount ||
+	    atomic_read(&inode->i_readcount) != self_rcount)
+		return -EAGAIN;
 
-	return ret;
+	return 0;
 }
 
 static int
 generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
 {
 	struct file_lock *fl, *my_fl = NULL, *lease;
-	struct dentry *dentry = filp->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = locks_inode(filp);
 	struct file_lock_context *ctx;
 	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
 	int error;
@@ -1822,7 +1834,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	percpu_down_read(&file_rwsem);
 	spin_lock(&ctx->flc_lock);
 	time_out_leases(inode, &dispose);
-	error = check_conflicting_open(dentry, arg, lease->fl_flags);
+	error = check_conflicting_open(filp, arg, lease->fl_flags);
 	if (error)
 		goto out;
 
@@ -1879,7 +1891,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	 * precedes these checks.
 	 */
 	smp_mb();
-	error = check_conflicting_open(dentry, arg, lease->fl_flags);
+	error = check_conflicting_open(filp, arg, lease->fl_flags);
 	if (error) {
 		locks_unlink_lock_ctx(lease);
 		goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 79ffa2958bd8..2d55f1b64014 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -694,7 +694,7 @@ struct inode {
 	atomic_t		i_count;
 	atomic_t		i_dio_count;
 	atomic_t		i_writecount;
-#ifdef CONFIG_IMA
+#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
 	atomic_t		i_readcount; /* struct files open RO */
 #endif
 	union {
@@ -2895,7 +2895,7 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
 	return atomic_read(&inode->i_writecount) > 0;
 }
 
-#ifdef CONFIG_IMA
+#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
 static inline void i_readcount_dec(struct inode *inode)
 {
 	BUG_ON(!atomic_read(&inode->i_readcount));
-- 
2.17.1

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Jeff Layton]

On Wed, 2019-06-12 at 20:24 +0300, Amir Goldstein wrote:
> check_conflicting_open() is checking for existing fd's open for read or
> for write before allowing to take a write lease.  The check that was
> implemented using i_count and d_count is an approximation that has
> several false positives.  For example, overlayfs since v4.19, takes an
> extra reference on the dentry; An open with O_PATH takes a reference on
> the dentry although the file cannot be read nor written.
> 
> Change the implementation to use i_readcount and i_writecount to
> eliminate the false positive conflicts and allow a write lease to be
> taken on an overlayfs file.
> 
> The change of behavior with existing fd's open with O_PATH is symmetric
> w.r.t. current behavior of lease breakers - an open with O_PATH currently
> does not break a write lease.
> 
> This increases the size of struct inode by 4 bytes on 32bit archs when
> CONFIG_FILE_LOCKING is defined and CONFIG_IMA was not already
> defined.
> 
> Cc: <stable@vger.kernel.org> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Miklos, Jeff and Bruce,
> 
> This patch fixes a v4.19 overlayfs regression with taking write
> leases. It also provides correct semantics w.r.t RDONLY open counter
> that Bruce also needed for nfsd.
> 
> Since this is locks code that fixes an overlayfs regression which
> is also needed for nfsd, it could go via either of your trees.
> I didn't want to pick sides, so first one to grab the patch wins ;-)
> 
> I verified the changes using modified LTP F_SETLEASE tests [1],
> which I ran over xfs and overlayfs.
> 
> Thanks,
> Amir.
> 
> [1] https://github.com/amir73il/ltp/commits/overlayfs-devel
> 
> Changes since v1:
> - Drop patch to fold i_readcount into i_count
> - Make i_readcount depend on CONFIG_FILE_LOCKING
> 
>  fs/locks.c         | 33 ++++++++++++++++++++++-----------
>  include/linux/fs.h |  4 ++--
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index ec1e4a5df629..28528b4fc53b 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1753,10 +1753,10 @@ int fcntl_getlease(struct file *filp)
>  }
>  
>  /**
> - * check_conflicting_open - see if the given dentry points to a file that has
> + * check_conflicting_open - see if the given file points to an inode that has
>   *			    an existing open that would conflict with the
>   *			    desired lease.
> - * @dentry:	dentry to check
> + * @filp:	file to check
>   * @arg:	type of lease that we're trying to acquire
>   * @flags:	current lock flags
>   *
> @@ -1764,19 +1764,31 @@ int fcntl_getlease(struct file *filp)
>   * conflict with the lease we're trying to set.
>   */
>  static int
> -check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
> +check_conflicting_open(struct file *filp, const long arg, int flags)
>  {
>  	int ret = 0;
> -	struct inode *inode = dentry->d_inode;
> +	struct inode *inode = locks_inode(filp);
> +	int wcount = atomic_read(&inode->i_writecount);
> +	int self_wcount = 0, self_rcount = 0;
>  
>  	if (flags & FL_LAYOUT)
>  		return 0;
>  
> -	if ((arg == F_RDLCK) && inode_is_open_for_write(inode))
> +	if (arg == F_RDLCK && wcount > 0)
>  		return -EAGAIN;
>  
> -	if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
> -	    (atomic_read(&inode->i_count) > 1)))
> +	/* Eliminate deny writes from actual writers count */
> +	if (wcount < 0)
> +		wcount = 0;
> +
> +	/* Make sure that only read/write count is from lease requestor */
> +	if (filp->f_mode & FMODE_WRITE)
> +		self_wcount = 1;
> +	else if ((filp->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)

nit: you already checked for FMODE_WRITE and you know that it's not set
here, so this is equivalent to:

    else if (filp->f_mode & FMODE_READ)

> +		self_rcount = 1;
> +
> +	if (arg == F_WRLCK && (wcount != self_wcount ||
> +	    atomic_read(&inode->i_readcount) != self_rcount))
>  		ret = -EAGAIN;
>  
>  	return ret;
> @@ -1786,8 +1798,7 @@ static int
>  generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
>  {
>  	struct file_lock *fl, *my_fl = NULL, *lease;
> -	struct dentry *dentry = filp->f_path.dentry;
> -	struct inode *inode = dentry->d_inode;
> +	struct inode *inode = locks_inode(filp);
>  	struct file_lock_context *ctx;
>  	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
>  	int error;
> @@ -1822,7 +1833,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>  	percpu_down_read(&file_rwsem);
>  	spin_lock(&ctx->flc_lock);
>  	time_out_leases(inode, &dispose);
> -	error = check_conflicting_open(dentry, arg, lease->fl_flags);
> +	error = check_conflicting_open(filp, arg, lease->fl_flags);
>  	if (error)
>  		goto out;
>  
> @@ -1879,7 +1890,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>  	 * precedes these checks.
>  	 */
>  	smp_mb();
> -	error = check_conflicting_open(dentry, arg, lease->fl_flags);
> +	error = check_conflicting_open(filp, arg, lease->fl_flags);
>  	if (error) {
>  		locks_unlink_lock_ctx(lease);
>  		goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 79ffa2958bd8..2d55f1b64014 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -694,7 +694,7 @@ struct inode {
>  	atomic_t		i_count;
>  	atomic_t		i_dio_count;
>  	atomic_t		i_writecount;
> -#ifdef CONFIG_IMA
> +#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
>  	atomic_t		i_readcount; /* struct files open RO */
>  #endif
>  	union {
> @@ -2895,7 +2895,7 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
>  	return atomic_read(&inode->i_writecount) > 0;
>  }
>  
> -#ifdef CONFIG_IMA
> +#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
>  static inline void i_readcount_dec(struct inode *inode)
>  {
>  	BUG_ON(!atomic_read(&inode->i_readcount));


Looks good to me. Aside from the minor nit above:

    Reviewed-by: Jeff Layton <jlayton@kernel.org>

I have one file locking patch queued up for v5.3 so far, but nothing for
v5.2. Miklos or Bruce, if either of you have anything to send to Linus
for v5.2 would you mind taking this one too?

If not I can queue it up and get it to him after letting it soak in
linux-next for a bit.

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Amir Goldstein]

On Thu, Jun 13, 2019 at 4:22 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2019-06-12 at 20:24 +0300, Amir Goldstein wrote:
> > check_conflicting_open() is checking for existing fd's open for read or
> > for write before allowing to take a write lease.  The check that was
> > implemented using i_count and d_count is an approximation that has
> > several false positives.  For example, overlayfs since v4.19, takes an
> > extra reference on the dentry; An open with O_PATH takes a reference on
> > the dentry although the file cannot be read nor written.
> >
> > Change the implementation to use i_readcount and i_writecount to
> > eliminate the false positive conflicts and allow a write lease to be
> > taken on an overlayfs file.
> >
> > The change of behavior with existing fd's open with O_PATH is symmetric
> > w.r.t. current behavior of lease breakers - an open with O_PATH currently
> > does not break a write lease.
> >
> > This increases the size of struct inode by 4 bytes on 32bit archs when
> > CONFIG_FILE_LOCKING is defined and CONFIG_IMA was not already
> > defined.
> >
> > Cc: <stable@vger.kernel.org> # v4.19
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Miklos, Jeff and Bruce,
> >
> > This patch fixes a v4.19 overlayfs regression with taking write
> > leases. It also provides correct semantics w.r.t RDONLY open counter
> > that Bruce also needed for nfsd.
> >
> > Since this is locks code that fixes an overlayfs regression which
> > is also needed for nfsd, it could go via either of your trees.
> > I didn't want to pick sides, so first one to grab the patch wins ;-)
> >
> > I verified the changes using modified LTP F_SETLEASE tests [1],
> > which I ran over xfs and overlayfs.
> >
> > Thanks,
> > Amir.
> >
> > [1] https://github.com/amir73il/ltp/commits/overlayfs-devel
> >
> > Changes since v1:
> > - Drop patch to fold i_readcount into i_count
> > - Make i_readcount depend on CONFIG_FILE_LOCKING
> >
> >  fs/locks.c         | 33 ++++++++++++++++++++++-----------
> >  include/linux/fs.h |  4 ++--
> >  2 files changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/locks.c b/fs/locks.c
> > index ec1e4a5df629..28528b4fc53b 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -1753,10 +1753,10 @@ int fcntl_getlease(struct file *filp)
> >  }
> >
> >  /**
> > - * check_conflicting_open - see if the given dentry points to a file that has
> > + * check_conflicting_open - see if the given file points to an inode that has
> >   *                       an existing open that would conflict with the
> >   *                       desired lease.
> > - * @dentry:  dentry to check
> > + * @filp:    file to check
> >   * @arg:     type of lease that we're trying to acquire
> >   * @flags:   current lock flags
> >   *
> > @@ -1764,19 +1764,31 @@ int fcntl_getlease(struct file *filp)
> >   * conflict with the lease we're trying to set.
> >   */
> >  static int
> > -check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
> > +check_conflicting_open(struct file *filp, const long arg, int flags)
> >  {
> >       int ret = 0;
> > -     struct inode *inode = dentry->d_inode;
> > +     struct inode *inode = locks_inode(filp);
> > +     int wcount = atomic_read(&inode->i_writecount);
> > +     int self_wcount = 0, self_rcount = 0;
> >
> >       if (flags & FL_LAYOUT)
> >               return 0;
> >
> > -     if ((arg == F_RDLCK) && inode_is_open_for_write(inode))
> > +     if (arg == F_RDLCK && wcount > 0)
> >               return -EAGAIN;
> >
> > -     if ((arg == F_WRLCK) && ((d_count(dentry) > 1) ||
> > -         (atomic_read(&inode->i_count) > 1)))
> > +     /* Eliminate deny writes from actual writers count */
> > +     if (wcount < 0)
> > +             wcount = 0;
> > +
> > +     /* Make sure that only read/write count is from lease requestor */
> > +     if (filp->f_mode & FMODE_WRITE)
> > +             self_wcount = 1;
> > +     else if ((filp->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
>
> nit: you already checked for FMODE_WRITE and you know that it's not set
> here, so this is equivalent to:
>
>     else if (filp->f_mode & FMODE_READ)
>

Right... I guess you or Miklos could fix this up on commit.
Let me know if you want a re-post.

> > +             self_rcount = 1;
> > +
> > +     if (arg == F_WRLCK && (wcount != self_wcount ||
> > +         atomic_read(&inode->i_readcount) != self_rcount))
> >               ret = -EAGAIN;
> >
> >       return ret;
> > @@ -1786,8 +1798,7 @@ static int
> >  generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv)
> >  {
> >       struct file_lock *fl, *my_fl = NULL, *lease;
> > -     struct dentry *dentry = filp->f_path.dentry;
> > -     struct inode *inode = dentry->d_inode;
> > +     struct inode *inode = locks_inode(filp);
> >       struct file_lock_context *ctx;
> >       bool is_deleg = (*flp)->fl_flags & FL_DELEG;
> >       int error;
> > @@ -1822,7 +1833,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
> >       percpu_down_read(&file_rwsem);
> >       spin_lock(&ctx->flc_lock);
> >       time_out_leases(inode, &dispose);
> > -     error = check_conflicting_open(dentry, arg, lease->fl_flags);
> > +     error = check_conflicting_open(filp, arg, lease->fl_flags);
> >       if (error)
> >               goto out;
> >
> > @@ -1879,7 +1890,7 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
> >        * precedes these checks.
> >        */
> >       smp_mb();
> > -     error = check_conflicting_open(dentry, arg, lease->fl_flags);
> > +     error = check_conflicting_open(filp, arg, lease->fl_flags);
> >       if (error) {
> >               locks_unlink_lock_ctx(lease);
> >               goto out;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 79ffa2958bd8..2d55f1b64014 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -694,7 +694,7 @@ struct inode {
> >       atomic_t                i_count;
> >       atomic_t                i_dio_count;
> >       atomic_t                i_writecount;
> > -#ifdef CONFIG_IMA
> > +#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
> >       atomic_t                i_readcount; /* struct files open RO */
> >  #endif
> >       union {
> > @@ -2895,7 +2895,7 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
> >       return atomic_read(&inode->i_writecount) > 0;
> >  }
> >
> > -#ifdef CONFIG_IMA
> > +#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
> >  static inline void i_readcount_dec(struct inode *inode)
> >  {
> >       BUG_ON(!atomic_read(&inode->i_readcount));
>
>
> Looks good to me. Aside from the minor nit above:
>
>     Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
> I have one file locking patch queued up for v5.3 so far, but nothing for
> v5.2. Miklos or Bruce, if either of you have anything to send to Linus
> for v5.2 would you mind taking this one too?
>

Well. I did send a fix patch to Miklos for a bug introduced in v5.2-rc4,
so...

> If not I can queue it up and get it to him after letting it soak in
> linux-next for a bit.
>

Thanks for review!
Amir.

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[J . Bruce Fields]

On Thu, Jun 13, 2019 at 04:28:49PM +0300, Amir Goldstein wrote:
> On Thu, Jun 13, 2019 at 4:22 PM Jeff Layton <jlayton@kernel.org> wrote:
> > Looks good to me. Aside from the minor nit above:
> >
> >     Reviewed-by: Jeff Layton <jlayton@kernel.org>
> >
> > I have one file locking patch queued up for v5.3 so far, but nothing for
> > v5.2. Miklos or Bruce, if either of you have anything to send to Linus
> > for v5.2 would you mind taking this one too?
> >
> 
> Well. I did send a fix patch to Miklos for a bug introduced in v5.2-rc4,
> so...

I could take it.  I've modified it as below.

I'm very happy with the patch, but not so much with the idea of 5.2 and
stable.

It seems like a subtle change with some possibility of unintended side
effects.  (E.g. I don't think this is true any more, but my memory is
that for a long time the only thing stopping nfsd from giving out
(probably broken) write delegations was an extra reference that it held
during processing.) And if the overlayfs bug's been there since 4.19,
then waiting a little longer seems OK?

--b.

diff --git a/fs/locks.c b/fs/locks.c
index c7912b0fdeea..2056595751e8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1779,7 +1779,7 @@ check_conflicting_open(struct file *filp, const long arg, int flags)
 	/* Make sure that only read/write count is from lease requestor */
 	if (filp->f_mode & FMODE_WRITE)
 		self_wcount = 1;
-	else if ((filp->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+	else if (filp->f_mode & FMODE_READ)
 		self_rcount = 1;
 
 	if (arg == F_WRLCK && (wcount != self_wcount ||

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Amir Goldstein]

On Thu, Jun 13, 2019 at 5:08 PM J . Bruce Fields <bfields@fieldses.org> wrote:
>
> On Thu, Jun 13, 2019 at 04:28:49PM +0300, Amir Goldstein wrote:
> > On Thu, Jun 13, 2019 at 4:22 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > Looks good to me. Aside from the minor nit above:
> > >
> > >     Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > >
> > > I have one file locking patch queued up for v5.3 so far, but nothing for
> > > v5.2. Miklos or Bruce, if either of you have anything to send to Linus
> > > for v5.2 would you mind taking this one too?
> > >
> >
> > Well. I did send a fix patch to Miklos for a bug introduced in v5.2-rc4,
> > so...
>
> I could take it.  I've modified it as below.
>
> I'm very happy with the patch, but not so much with the idea of 5.2 and
> stable.
>
> It seems like a subtle change with some possibility of unintended side
> effects.  (E.g. I don't think this is true any more, but my memory is
> that for a long time the only thing stopping nfsd from giving out
> (probably broken) write delegations was an extra reference that it held
> during processing.) And if the overlayfs bug's been there since 4.19,
> then waiting a little longer seems OK?
>

Getting back to this now that the patch is on its way to Linus.
Bruce, I was fine with waiting to 5.3 and I also removed CC: stable,
but did you mean that patch is not appropriate for stable or just that
we'd better wait a bit and let it soak in master before forwarding it to stable?

Thanks,
Amir.

Re: [PATCH v2] locks: eliminate false positive conflicts for write lease

[Jeff Layton]

On Mon, 2019-07-08 at 19:09 +0300, Amir Goldstein wrote:
> On Thu, Jun 13, 2019 at 5:08 PM J . Bruce Fields <bfields@fieldses.org> wrote:
> > On Thu, Jun 13, 2019 at 04:28:49PM +0300, Amir Goldstein wrote:
> > > On Thu, Jun 13, 2019 at 4:22 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > Looks good to me. Aside from the minor nit above:
> > > > 
> > > >     Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > > > 
> > > > I have one file locking patch queued up for v5.3 so far, but nothing for
> > > > v5.2. Miklos or Bruce, if either of you have anything to send to Linus
> > > > for v5.2 would you mind taking this one too?
> > > > 
> > > 
> > > Well. I did send a fix patch to Miklos for a bug introduced in v5.2-rc4,
> > > so...
> > 
> > I could take it.  I've modified it as below.
> > 
> > I'm very happy with the patch, but not so much with the idea of 5.2 and
> > stable.
> > 
> > It seems like a subtle change with some possibility of unintended side
> > effects.  (E.g. I don't think this is true any more, but my memory is
> > that for a long time the only thing stopping nfsd from giving out
> > (probably broken) write delegations was an extra reference that it held
> > during processing.) And if the overlayfs bug's been there since 4.19,
> > then waiting a little longer seems OK?
> > 
> 
> Getting back to this now that the patch is on its way to Linus.
> Bruce, I was fine with waiting to 5.3 and I also removed CC: stable,
> but did you mean that patch is not appropriate for stable or just that
> we'd better wait a bit and let it soak in master before forwarding it to stable?
> 

With NFS and SMB, oplocks/leases/delegations are optimizations and
you're never guaranteed to get one in the face of competing access.

stable-kernel-rules.rst says:

- It must fix a problem that causes a build error (but not for
things marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
security issue, or some "oh, that's not good" issue.  In short,
something critical.

I'm not sure this clears that bar.
-- 
Jeff Layton <jlayton@kernel.org>