On Thu, Jun 13, 2019 at 4:22 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx> # v4.19 > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > --- > > > > 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@xxxxxxxxxx> > > 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.
