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.
> +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?
