* [PATCH 0/2] Fix write leases on overlayfs
@ 2019-06-08 13:57 Amir Goldstein
2019-06-08 13:57 ` [PATCH 1/2] vfs: replace i_readcount with a biased i_count Amir Goldstein
2019-06-08 13:57 ` [PATCH 2/2] locks: eliminate false positive conflicts for write lease Amir Goldstein
0 siblings, 2 replies; 9+ messages in thread
From: Amir Goldstein @ 2019-06-08 13:57 UTC (permalink / raw)
To: Miklos Szeredi
Cc: J . Bruce Fields, Jeff Layton, Mimi Zohar, Al Viro,
linux-fsdevel, linux-unionfs, linux-integrity
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] vfs: replace i_readcount with a biased i_count
2019-06-08 13:57 [PATCH 0/2] Fix write leases on overlayfs Amir Goldstein
@ 2019-06-08 13:57 ` Amir Goldstein
2019-06-09 19:36 ` Miklos Szeredi
2019-06-12 12:51 ` Mimi Zohar
2019-06-08 13:57 ` [PATCH 2/2] locks: eliminate false positive conflicts for write lease Amir Goldstein
1 sibling, 2 replies; 9+ messages in thread
From: Amir Goldstein @ 2019-06-08 13:57 UTC (permalink / raw)
To: Miklos Szeredi
Cc: J . Bruce Fields, Jeff Layton, Mimi Zohar, Al Viro,
linux-fsdevel, linux-unionfs, linux-integrity
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] locks: eliminate false positive conflicts for write lease
2019-06-08 13:57 [PATCH 0/2] Fix write leases on overlayfs Amir Goldstein
2019-06-08 13:57 ` [PATCH 1/2] vfs: replace i_readcount with a biased i_count Amir Goldstein
@ 2019-06-08 13:57 ` Amir Goldstein
1 sibling, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2019-06-08 13:57 UTC (permalink / raw)
To: Miklos Szeredi
Cc: J . Bruce Fields, Jeff Layton, Mimi Zohar, Al Viro,
linux-fsdevel, linux-unionfs, linux-integrity
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count
2019-06-08 13:57 ` [PATCH 1/2] vfs: replace i_readcount with a biased i_count Amir Goldstein
@ 2019-06-09 19:36 ` Miklos Szeredi
2019-06-12 12:51 ` Mimi Zohar
1 sibling, 0 replies; 9+ messages in thread
From: Miklos Szeredi @ 2019-06-09 19:36 UTC (permalink / raw)
To: Amir Goldstein
Cc: J . Bruce Fields, Jeff Layton, Mimi Zohar, Al Viro,
linux-fsdevel, overlayfs, linux-integrity
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count
2019-06-08 13:57 ` [PATCH 1/2] vfs: replace i_readcount with a biased i_count Amir Goldstein
2019-06-09 19:36 ` Miklos Szeredi
@ 2019-06-12 12:51 ` Mimi Zohar
2019-06-12 15:09 ` Amir Goldstein
1 sibling, 1 reply; 9+ messages in thread
From: Mimi Zohar @ 2019-06-12 12:51 UTC (permalink / raw)
To: Amir Goldstein, Miklos Szeredi
Cc: J . Bruce Fields, Jeff Layton, Al Viro, linux-fsdevel,
linux-unionfs, linux-integrity
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 */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count
2019-06-12 12:51 ` Mimi Zohar
@ 2019-06-12 15:09 ` Amir Goldstein
2019-06-12 15:29 ` J . Bruce Fields
2019-06-12 15:59 ` Mimi Zohar
0 siblings, 2 replies; 9+ messages in thread
From: Amir Goldstein @ 2019-06-12 15:09 UTC (permalink / raw)
To: Mimi Zohar
Cc: Miklos Szeredi, J . Bruce Fields, Jeff Layton, Al Viro,
linux-fsdevel, overlayfs, linux-integrity
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count
2019-06-12 15:09 ` Amir Goldstein
@ 2019-06-12 15:29 ` J . Bruce Fields
2019-06-12 15:35 ` Amir Goldstein
2019-06-12 15:59 ` Mimi Zohar
1 sibling, 1 reply; 9+ messages in thread
From: J . Bruce Fields @ 2019-06-12 15:29 UTC (permalink / raw)
To: Amir Goldstein
Cc: Mimi Zohar, Miklos Szeredi, Jeff Layton, Al Viro, linux-fsdevel,
overlayfs, linux-integrity
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count
2019-06-12 15:29 ` J . Bruce Fields
@ 2019-06-12 15:35 ` Amir Goldstein
0 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2019-06-12 15:35 UTC (permalink / raw)
To: J . Bruce Fields
Cc: Mimi Zohar, Miklos Szeredi, Jeff Layton, Al Viro, linux-fsdevel,
overlayfs, linux-integrity
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] vfs: replace i_readcount with a biased i_count
2019-06-12 15:09 ` Amir Goldstein
2019-06-12 15:29 ` J . Bruce Fields
@ 2019-06-12 15:59 ` Mimi Zohar
1 sibling, 0 replies; 9+ messages in thread
From: Mimi Zohar @ 2019-06-12 15:59 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, J . Bruce Fields, Jeff Layton, Al Viro,
linux-fsdevel, overlayfs, linux-integrity
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-12 15:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-08 13:57 [PATCH 0/2] Fix write leases on overlayfs Amir Goldstein
2019-06-08 13:57 ` [PATCH 1/2] vfs: replace i_readcount with a biased i_count Amir Goldstein
2019-06-09 19:36 ` Miklos Szeredi
2019-06-12 12:51 ` Mimi Zohar
2019-06-12 15:09 ` Amir Goldstein
2019-06-12 15:29 ` J . Bruce Fields
2019-06-12 15:35 ` Amir Goldstein
2019-06-12 15:59 ` Mimi Zohar
2019-06-08 13:57 ` [PATCH 2/2] locks: eliminate false positive conflicts for write lease Amir Goldstein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).