| Message ID | 20220816140818.708431-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] locks: fix TOCTOU race when granting write lease | expand |
On Tue, 2022-08-16 at 17:08 +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@gmail.com> > --- > > Hi Al, > > Per your request, here is v2 with moved helper. > > Jeff has suggested that I add kerneldoc, but I couldn't > come up with a good description and didn't think it was very > important, so I left it as is. > It's not terribly important. > Thanks, > Amir. > > Changes since v1: > - Move helper to internal.h > > fs/file_table.c | 7 +------ > fs/internal.h | 10 ++++++++++ > fs/open.c | 11 ++++------- > 3 files changed, 15 insertions(+), 13 deletions(-) > > diff --git a/fs/file_table.c b/fs/file_table.c > index 99c6796c9f28..dd88701e54a9 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -324,12 +324,7 @@ static void __fput(struct file *file) > } > fops_put(file->f_op); > put_pid(file->f_owner.pid); > - if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) > - i_readcount_dec(inode); > - if (mode & FMODE_WRITER) { > - put_write_access(inode); > - __mnt_drop_write(mnt); > - } > + put_file_access(file); > dput(dentry); > if (unlikely(mode & FMODE_NEED_UNMOUNT)) > dissolve_on_fput(mnt); > diff --git a/fs/internal.h b/fs/internal.h > index 87e96b9024ce..31861e6c3eff 100644 > --- a/fs/internal.h > +++ b/fs/internal.h > @@ -101,6 +101,16 @@ extern void chroot_fs_refs(const struct path *, const struct path *); > extern struct file *alloc_empty_file(int, const struct cred *); > extern struct file *alloc_empty_file_noaccount(int, const struct cred *); > > +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); > + } > +} > + > /* > * super.c > */ > diff --git a/fs/open.c b/fs/open.c > index 8a813fa5ca56..a98572585815 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -840,7 +840,9 @@ static int do_dentry_open(struct file *f, > return 0; > } > > - if (f->f_mode & FMODE_WRITE && !special_file(inode->i_mode)) { > + if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) { > + i_readcount_inc(inode); > + } else if (f->f_mode & FMODE_WRITE && !special_file(inode->i_mode)) { > error = get_write_access(inode); > if (unlikely(error)) > goto cleanup_file; > @@ -880,8 +882,6 @@ static int do_dentry_open(struct file *f, > goto cleanup_all; > } > f->f_mode |= FMODE_OPENED; > - if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) > - i_readcount_inc(inode); > if ((f->f_mode & FMODE_READ) && > likely(f->f_op->read || f->f_op->read_iter)) > f->f_mode |= FMODE_CAN_READ; > @@ -935,10 +935,7 @@ static int do_dentry_open(struct file *f, > if (WARN_ON_ONCE(error > 0)) > error = -EINVAL; > fops_put(f->f_op); > - if (f->f_mode & FMODE_WRITER) { > - put_write_access(inode); > - __mnt_drop_write(f->f_path.mnt); > - } > + put_file_access(f); > cleanup_file: > path_put(&f->f_path); > f->f_path.mnt = NULL; Let me know if you need me to take this via my tree. Since Al responded, I'm assuming he'll pick this one up. Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/file_table.c b/fs/file_table.c index 99c6796c9f28..dd88701e54a9 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -324,12 +324,7 @@ static void __fput(struct file *file) } fops_put(file->f_op); put_pid(file->f_owner.pid); - if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) - i_readcount_dec(inode); - if (mode & FMODE_WRITER) { - put_write_access(inode); - __mnt_drop_write(mnt); - } + put_file_access(file); dput(dentry); if (unlikely(mode & FMODE_NEED_UNMOUNT)) dissolve_on_fput(mnt); diff --git a/fs/internal.h b/fs/internal.h index 87e96b9024ce..31861e6c3eff 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -101,6 +101,16 @@ extern void chroot_fs_refs(const struct path *, const struct path *); extern struct file *alloc_empty_file(int, const struct cred *); extern struct file *alloc_empty_file_noaccount(int, const struct cred *); +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); + } +} + /* * super.c */ diff --git a/fs/open.c b/fs/open.c index 8a813fa5ca56..a98572585815 100644 --- a/fs/open.c +++ b/fs/open.c @@ -840,7 +840,9 @@ static int do_dentry_open(struct file *f, return 0; } - if (f->f_mode & FMODE_WRITE && !special_file(inode->i_mode)) { + if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) { + i_readcount_inc(inode); + } else if (f->f_mode & FMODE_WRITE && !special_file(inode->i_mode)) { error = get_write_access(inode); if (unlikely(error)) goto cleanup_file; @@ -880,8 +882,6 @@ static int do_dentry_open(struct file *f, goto cleanup_all; } f->f_mode |= FMODE_OPENED; - if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) - i_readcount_inc(inode); if ((f->f_mode & FMODE_READ) && likely(f->f_op->read || f->f_op->read_iter)) f->f_mode |= FMODE_CAN_READ; @@ -935,10 +935,7 @@ static int do_dentry_open(struct file *f, if (WARN_ON_ONCE(error > 0)) error = -EINVAL; fops_put(f->f_op); - if (f->f_mode & FMODE_WRITER) { - put_write_access(inode); - __mnt_drop_write(f->f_path.mnt); - } + put_file_access(f); cleanup_file: path_put(&f->f_path); f->f_path.mnt = NULL;
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@gmail.com> --- Hi Al, Per your request, here is v2 with moved helper. Jeff has suggested that I add kerneldoc, but I couldn't come up with a good description and didn't think it was very important, so I left it as is. Thanks, Amir. Changes since v1: - Move helper to internal.h fs/file_table.c | 7 +------ fs/internal.h | 10 ++++++++++ fs/open.c | 11 ++++------- 3 files changed, 15 insertions(+), 13 deletions(-)