On Sun, Aug 14, 2022 at 8:57 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Sun, Aug 14, 2022 at 06:23:22PM +0300, Amir Goldstein wrote:
> > Thread A trying to acquire a write lease checks the value of i_readcount
> > and i_writecount in check_conflicting_open() to verify that its own fd
> > is the only fd referencing the file.
> >
> > Thread B trying to open the file for read will call break_lease() in
> > do_dentry_open() before incrementing i_readcount, which leaves a small
> > window where thread A can acquire the write lease and then thread B
> > completes the open of the file for read without breaking the write lease
> > that was acquired by thread A.
> >
> > Fix this race by incrementing i_readcount before checking for existing
> > leases, same as the case with i_writecount.
> >
> > Use a helper put_file_access() to decrement i_readcount or i_writecount
> > in do_dentry_open() and __fput().
> >
> > Fixes: 387e3746d01c ("locks: eliminate false positive conflicts for write lease")
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> Looks sane; I'd probably collapsed cleanup_file and cleanup_all while we are
> at it, but then I can do that in a followup as well.
>
Not sure how you envision that cleanup, so I'll let you do it.
> > +static inline void put_file_access(struct file *file)
> > +{
> > + if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
> > + i_readcount_dec(file->f_inode);
> > + } else if (file->f_mode & FMODE_WRITER) {
> > + put_write_access(file->f_inode);
> > + __mnt_drop_write(file->f_path.mnt);
> > + }
> > +}
>
> What's the point of having it in linux/fs.h instead of internal.h?
No reason. Overlooked.
Do you need me to re-send or will you move it to internal.h yourself?
Thanks,
Amir.
