[PATCH 0/2] Fix write leases on overlayfs

[PATCH 0/2] Fix write leases on overlayfs

[Amir Goldstein]

Miklos,

This series fixes a v4.19 regression.

It also provides correct semantics w.r.t RDONLY open counter
that Bruce also needed for nfsd.

I marked both patches for stable v4.19.

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

Thanks,
Amir.

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

Amir Goldstein (2):
  vfs: replace i_readcount with a biased i_count
  locks: eliminate false positive conflicts for write lease

 fs/locks.c                        | 26 ++++++++++++++----------
 include/linux/fs.h                | 33 +++++++++++++++++++------------
 security/integrity/ima/ima_main.c |  2 +-
 3 files changed, 37 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/2] vfs: replace i_readcount with a biased i_count

[Amir Goldstein]

Count struct files open RO together with inode reference count instead
of using a dedicated i_readcount field.  This will allow us to use the
RO count also when CONFIG_IMA is not defined and will reduce the size of
struct inode for 32bit archs when CONFIG_IMA is defined.

We need this RO count for posix leases code, which currently naively
checks i_count and d_count in an inaccurate manner.

Should regular i_count overflow into RO count bias by struct files
opened for write, it's not a big deal, as we mostly need the RO count
to be reliable when the first writer comes along.

Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fs.h                | 33 +++++++++++++++++++------------
 security/integrity/ima/ima_main.c |  2 +-
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index f7fdfe93e25d..504bf17967dd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -694,9 +694,6 @@ struct inode {
 	atomic_t		i_count;
 	atomic_t		i_dio_count;
 	atomic_t		i_writecount;
-#ifdef CONFIG_IMA
-	atomic_t		i_readcount; /* struct files open RO */
-#endif
 	union {
 		const struct file_operations	*i_fop;	/* former ->i_op->default_file_ops */
 		void (*free_inode)(struct inode *);
@@ -2890,26 +2887,36 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
 	return atomic_read(&inode->i_writecount) > 0;
 }
 
-#ifdef CONFIG_IMA
+/*
+ * Count struct files open RO together with inode rerefernce count.
+ * We need this count for IMA and for posix leases. The RO count should not
+ * include files opened RDWR nor files opened O_PATH and internal kernel
+ * inode references, like the ones taken by overlayfs and inotify.
+ * Should regular i_count overflow into I_RO_COUNT_BIAS by struct files
+ * opened for write, it's not a big deal, as we mostly need
+ * inode_is_open_rdonly() to be reliable when the first writer comes along.
+ */
+#define I_RO_COUNT_SHIFT 10
+#define I_RO_COUNT_BIAS	(1UL << I_RO_COUNT_SHIFT)
+
 static inline void i_readcount_dec(struct inode *inode)
 {
-	BUG_ON(!atomic_read(&inode->i_readcount));
-	atomic_dec(&inode->i_readcount);
+	WARN_ON(atomic_read(&inode->i_count) < I_RO_COUNT_BIAS);
+	atomic_sub(I_RO_COUNT_BIAS, &inode->i_count);
 }
 static inline void i_readcount_inc(struct inode *inode)
 {
-	atomic_inc(&inode->i_readcount);
+	atomic_add(I_RO_COUNT_BIAS, &inode->i_count);
 }
-#else
-static inline void i_readcount_dec(struct inode *inode)
+static inline int i_readcount_read(const struct inode *inode)
 {
-	return;
+	return atomic_read(&inode->i_count) >> I_RO_COUNT_SHIFT;
 }
-static inline void i_readcount_inc(struct inode *inode)
+static inline bool inode_is_open_rdonly(const struct inode *inode)
 {
-	return;
+	return atomic_read(&inode->i_count) > I_RO_COUNT_BIAS;
 }
-#endif
+
 extern int do_pipe_flags(int *, int);
 
 #define __kernel_read_file_id(id) \
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..766bac778d11 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -94,7 +94,7 @@ static void ima_rdwr_violation_check(struct file *file,
 	bool send_tomtou = false, send_writers = false;
 
 	if (mode & FMODE_WRITE) {
-		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
+		if (inode_is_open_rdonly(inode) && IS_IMA(inode)) {
 			if (!iint)
 				iint = integrity_iint_find(inode);
 			/* IMA_MEASURE is set from reader side */
-- 
2.17.1

[PATCH 2/2] 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 inode and dentry although the file cannot be read nor written.

Change the implementation to use inode_is_open_rdonly() 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.

Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/locks.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index ec1e4a5df629..4937cfdf611a 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,10 +1764,11 @@ 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;
@@ -1775,8 +1776,14 @@ check_conflicting_open(const struct dentry *dentry, const long arg, int flags)
 	if ((arg == F_RDLCK) && inode_is_open_for_write(inode))
 		return -EAGAIN;
 
-	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 */
+	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) && (i_readcount_read(inode) != self_rcount ||
+	     (atomic_read(&inode->i_writecount) != self_wcount)))
 		ret = -EAGAIN;
 
 	return ret;
@@ -1786,8 +1793,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 +1828,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 +1885,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;
-- 
2.17.1

Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count

[Miklos Szeredi]

On Sat, Jun 8, 2019 at 3:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Count struct files open RO together with inode reference count instead
> of using a dedicated i_readcount field.  This will allow us to use the
> RO count also when CONFIG_IMA is not defined and will reduce the size of
> struct inode for 32bit archs when CONFIG_IMA is defined.
>
> We need this RO count for posix leases code, which currently naively
> checks i_count and d_count in an inaccurate manner.
>
> Should regular i_count overflow into RO count bias by struct files
> opened for write, it's not a big deal, as we mostly need the RO count
> to be reliable when the first writer comes along.
>
> Cc: <stable@vger.kernel.org> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  include/linux/fs.h                | 33 +++++++++++++++++++------------
>  security/integrity/ima/ima_main.c |  2 +-
>  2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index f7fdfe93e25d..504bf17967dd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -694,9 +694,6 @@ struct inode {
>         atomic_t                i_count;
>         atomic_t                i_dio_count;
>         atomic_t                i_writecount;
> -#ifdef CONFIG_IMA
> -       atomic_t                i_readcount; /* struct files open RO */
> -#endif
>         union {
>                 const struct file_operations    *i_fop; /* former ->i_op->default_file_ops */
>                 void (*free_inode)(struct inode *);
> @@ -2890,26 +2887,36 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
>         return atomic_read(&inode->i_writecount) > 0;
>  }
>
> -#ifdef CONFIG_IMA
> +/*
> + * Count struct files open RO together with inode rerefernce count.
> + * We need this count for IMA and for posix leases. The RO count should not
> + * include files opened RDWR nor files opened O_PATH and internal kernel
> + * inode references, like the ones taken by overlayfs and inotify.
> + * Should regular i_count overflow into I_RO_COUNT_BIAS by struct files
> + * opened for write, it's not a big deal, as we mostly need
> + * inode_is_open_rdonly() to be reliable when the first writer comes along.

The bigger issue is allowing i_count to wrap around: all you need is a
file with 1025 hard links and 4194304 opens of said file.  Doable
without privileges?  Not sure, but it does seem pretty fragile.

BTW the current 32 bit i_readcount that IMA uses also has wraparound
issues, though that's not nearly as bad.

Going to a 64 bit i_count would fix these, I guess.

Thanks,
Miklos

Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count

[Mimi Zohar]

On Sat, 2019-06-08 at 16:57 +0300, Amir Goldstein wrote:
> Count struct files open RO together with inode reference count instead
> of using a dedicated i_readcount field.  This will allow us to use the
> RO count also when CONFIG_IMA is not defined and will reduce the size of
> struct inode for 32bit archs when CONFIG_IMA is defined.
> 
> We need this RO count for posix leases code, which currently naively
> checks i_count and d_count in an inaccurate manner.
> 
> Should regular i_count overflow into RO count bias by struct files
> opened for write, it's not a big deal, as we mostly need the RO count
> to be reliable when the first writer comes along.

"i_count" has been defined forever.  Has its meaning changed?  This
patch implies that "i_readcount" was never really needed.

Mimi

> 
> Cc: <stable@vger.kernel.org> # v4.19
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  include/linux/fs.h                | 33 +++++++++++++++++++------------
>  security/integrity/ima/ima_main.c |  2 +-
>  2 files changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index f7fdfe93e25d..504bf17967dd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -694,9 +694,6 @@ struct inode {
>  	atomic_t		i_count;
>  	atomic_t		i_dio_count;
>  	atomic_t		i_writecount;
> -#ifdef CONFIG_IMA
> -	atomic_t		i_readcount; /* struct files open RO */
> -#endif
>  	union {
>  		const struct file_operations	*i_fop;	/* former ->i_op->default_file_ops */
>  		void (*free_inode)(struct inode *);
> @@ -2890,26 +2887,36 @@ static inline bool inode_is_open_for_write(const struct inode *inode)
>  	return atomic_read(&inode->i_writecount) > 0;
>  }
>  
> -#ifdef CONFIG_IMA
> +/*
> + * Count struct files open RO together with inode rerefernce count.
> + * We need this count for IMA and for posix leases. The RO count should not
> + * include files opened RDWR nor files opened O_PATH and internal kernel
> + * inode references, like the ones taken by overlayfs and inotify.
> + * Should regular i_count overflow into I_RO_COUNT_BIAS by struct files
> + * opened for write, it's not a big deal, as we mostly need
> + * inode_is_open_rdonly() to be reliable when the first writer comes along.
> + */
> +#define I_RO_COUNT_SHIFT 10
> +#define I_RO_COUNT_BIAS	(1UL << I_RO_COUNT_SHIFT)
> +
>  static inline void i_readcount_dec(struct inode *inode)
>  {
> -	BUG_ON(!atomic_read(&inode->i_readcount));
> -	atomic_dec(&inode->i_readcount);
> +	WARN_ON(atomic_read(&inode->i_count) < I_RO_COUNT_BIAS);
> +	atomic_sub(I_RO_COUNT_BIAS, &inode->i_count);
>  }
>  static inline void i_readcount_inc(struct inode *inode)
>  {
> -	atomic_inc(&inode->i_readcount);
> +	atomic_add(I_RO_COUNT_BIAS, &inode->i_count);
>  }
> -#else
> -static inline void i_readcount_dec(struct inode *inode)
> +static inline int i_readcount_read(const struct inode *inode)
>  {
> -	return;
> +	return atomic_read(&inode->i_count) >> I_RO_COUNT_SHIFT;
>  }
> -static inline void i_readcount_inc(struct inode *inode)
> +static inline bool inode_is_open_rdonly(const struct inode *inode)
>  {
> -	return;
> +	return atomic_read(&inode->i_count) > I_RO_COUNT_BIAS;
>  }
> -#endif
> +
>  extern int do_pipe_flags(int *, int);
>  
>  #define __kernel_read_file_id(id) \
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 357edd140c09..766bac778d11 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -94,7 +94,7 @@ static void ima_rdwr_violation_check(struct file *file,
>  	bool send_tomtou = false, send_writers = false;
>  
>  	if (mode & FMODE_WRITE) {
> -		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
> +		if (inode_is_open_rdonly(inode) && IS_IMA(inode)) {
>  			if (!iint)
>  				iint = integrity_iint_find(inode);
>  			/* IMA_MEASURE is set from reader side */

Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count

[Amir Goldstein]

On Wed, Jun 12, 2019 at 3:52 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Sat, 2019-06-08 at 16:57 +0300, Amir Goldstein wrote:
> > Count struct files open RO together with inode reference count instead
> > of using a dedicated i_readcount field.  This will allow us to use the
> > RO count also when CONFIG_IMA is not defined and will reduce the size of
> > struct inode for 32bit archs when CONFIG_IMA is defined.
> >
> > We need this RO count for posix leases code, which currently naively
> > checks i_count and d_count in an inaccurate manner.
> >
> > Should regular i_count overflow into RO count bias by struct files
> > opened for write, it's not a big deal, as we mostly need the RO count
> > to be reliable when the first writer comes along.
>
> "i_count" has been defined forever.  Has its meaning changed?  This
> patch implies that "i_readcount" was never really needed.
>

Not really.
i_count is only used to know if object is referenced.
It does not matter if user takes 1 or more references on i_count
as long as user puts back all the references it took.

If user took i_readcount, i_count cannot be zero, so short of overflow,
we can describe i_readcount as a biased i_count.

But if I am following Miklos' suggestion to make i_count 64bit, inode
struct size is going to grow for 32bit arch when  CONFIG_IMA is not
defined, so to reduce impact, I will keep i_readcount as a separate
member and let it be defined also when BITS_PER_LONG == 64
and implement inode_is_open_rdonly() using d_count and i_count
when i_readcount is not defined.

Let's see what people will have to say about that...

Thanks,
Amir.

Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count

[J . Bruce Fields]

On Wed, Jun 12, 2019 at 06:09:59PM +0300, Amir Goldstein wrote:
> But if I am following Miklos' suggestion to make i_count 64bit, inode
> struct size is going to grow for 32bit arch when  CONFIG_IMA is not
> defined, so to reduce impact, I will keep i_readcount as a separate
> member and let it be defined also when BITS_PER_LONG == 64
> and implement inode_is_open_rdonly() using d_count and i_count
> when i_readcount is not defined.

How bad would it be just to let the inode be a little bigger?  How big
is it already on 32 bit architectures?  How much does this change e.g.
how many inodes you can cache per megabyte?

--b.

Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count

[Amir Goldstein]

On Wed, Jun 12, 2019 at 6:29 PM J . Bruce Fields <bfields@fieldses.org> wrote:
>
> On Wed, Jun 12, 2019 at 06:09:59PM +0300, Amir Goldstein wrote:
> > But if I am following Miklos' suggestion to make i_count 64bit, inode
> > struct size is going to grow for 32bit arch when  CONFIG_IMA is not
> > defined, so to reduce impact, I will keep i_readcount as a separate
> > member and let it be defined also when BITS_PER_LONG == 64
> > and implement inode_is_open_rdonly() using d_count and i_count
> > when i_readcount is not defined.
>
> How bad would it be just to let the inode be a little bigger?  How big
> is it already on 32 bit architectures?  How much does this change e.g.
> how many inodes you can cache per megabyte?
>

It's hard to answer how tiny changes like this impact users with different
configs, especially to IoT ones, so I do not like increasing size of inode
unconditionally, but I will go with:
-#ifdef CONFIG_IMA
+#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING)
        atomic_t                i_readcount; /* struct files open RO */
 #endif

So IoT guys can have an option to keep inode size the same
and not let the locks code worry about it.

OK?

Thanks,
Amir.

Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count

[Mimi Zohar]

On Wed, 2019-06-12 at 18:09 +0300, Amir Goldstein wrote:
> On Wed, Jun 12, 2019 at 3:52 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Sat, 2019-06-08 at 16:57 +0300, Amir Goldstein wrote:
> > > Count struct files open RO together with inode reference count instead
> > > of using a dedicated i_readcount field.  This will allow us to use the
> > > RO count also when CONFIG_IMA is not defined and will reduce the size of
> > > struct inode for 32bit archs when CONFIG_IMA is defined.
> > >
> > > We need this RO count for posix leases code, which currently naively
> > > checks i_count and d_count in an inaccurate manner.
> > >
> > > Should regular i_count overflow into RO count bias by struct files
> > > opened for write, it's not a big deal, as we mostly need the RO count
> > > to be reliable when the first writer comes along.
> >
> > "i_count" has been defined forever.  Has its meaning changed?  This
> > patch implies that "i_readcount" was never really needed.
> >
> 
> Not really.
> i_count is only used to know if object is referenced.
> It does not matter if user takes 1 or more references on i_count
> as long as user puts back all the references it took.
> 
> If user took i_readcount, i_count cannot be zero, so short of overflow,
> we can describe i_readcount as a biased i_count.

Having a count was originally to make sure we weren't missing
anything.  As long as we can detect if a file is opened for read, the
less IMA specific code there is, the better.

> 
> But if I am following Miklos' suggestion to make i_count 64bit, inode
> struct size is going to grow for 32bit arch when  CONFIG_IMA is not
> defined, so to reduce impact, I will keep i_readcount as a separate
> member and let it be defined also when BITS_PER_LONG == 64
> and implement inode_is_open_rdonly() using d_count and i_count
> when i_readcount is not defined.
> 
> Let's see what people will have to say about that...

Ok

Mimi