| Message ID | 20221123104336.1030702-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: lock inode unconditionally in fuse_fallocate() | expand |
On Wed, Nov 23, 2022 at 11:43:36AM +0100, Miklos Szeredi wrote: > file_modified() must be called with inode lock held. fuse_fallocate() > didn't lock the inode in case of just FALLOC_KEEP_SIZE flags value, which > resulted in a kernel Warning in notify_change(). > > Lock the inode unconditionally, like all other fallocate implementations > do. > > Reported-by: Pengfei Xu <pengfei.xu@intel.com> > Reported-and-tested-by: syzbot+462da39f0667b357c4b6@syzkaller.appspotmail.com > Fixes: 4a6f278d4827 ("fuse: add file_modified() to fallocate") > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/fuse/file.c | 37 ++++++++++++++++--------------------- > 1 file changed, 16 insertions(+), 21 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 71bfb663aac5..89f4741728ba 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -2963,11 +2963,9 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset, > .mode = mode > }; > int err; > - bool lock_inode = !(mode & FALLOC_FL_KEEP_SIZE) || > - (mode & (FALLOC_FL_PUNCH_HOLE | > - FALLOC_FL_ZERO_RANGE)); > - > - bool block_faults = FUSE_IS_DAX(inode) && lock_inode; > + bool block_faults = FUSE_IS_DAX(inode) && > + (!(mode & FALLOC_FL_KEEP_SIZE) || > + (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE))); Hi Miklos, I think that we probably don't need above tests for "block_faults". Initially, ideas was that I wanted to synchronize truncate/punch_hole with fault handler path. Fuse did not seem to do it, so I decided to add it only for fuse DAX code to begin with. Details are in following commit. commit 6ae330cad6ef22ab8347ea9e0707dc56a7c7363f Author: Vivek Goyal <vgoyal@redhat.com> Date: Wed Aug 19 18:19:54 2020 -0400 virtiofs: serialize truncate/punch_hole and dax fault path I wanted to take following two locks for it to work. inode_lock(inode) down_write(&fi->i_mmap_sem); And that's why following condition. bool block_faults = FUSE_IS_DAX(inode) && lock_inode; Given that we will now always take inode lock, we could just do. bool block_faults = FUSE_IS_DAX(inode); instead. Not sure is there really a benefit in going through different "mode" flags and make this condition even narrower. I see that now fi->i_mmap_sem has been replaced by filemap_invalidate_lock(inode->i_mapping). At some point of time we should review fuse fault code and see if that code need to serialize with truncate/punch_hole path as well. If answer is yes, then we could get rid of this "block_faults" altogether and always take inode->i_mapping lock in truncate path. IOW, I think we should be able to simplify above to just for now. bool block_faults = FUSE_IS_DAX(inode); Thanks Vivek > > if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | > FALLOC_FL_ZERO_RANGE)) > @@ -2976,22 +2974,20 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset, > if (fm->fc->no_fallocate) > return -EOPNOTSUPP; > > - if (lock_inode) { > - inode_lock(inode); > - if (block_faults) { > - filemap_invalidate_lock(inode->i_mapping); > - err = fuse_dax_break_layouts(inode, 0, 0); > - if (err) > - goto out; > - } > + inode_lock(inode); > + if (block_faults) { > + filemap_invalidate_lock(inode->i_mapping); > + err = fuse_dax_break_layouts(inode, 0, 0); > + if (err) > + goto out; > + } > > - if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) { > - loff_t endbyte = offset + length - 1; > + if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) { > + loff_t endbyte = offset + length - 1; > > - err = fuse_writeback_range(inode, offset, endbyte); > - if (err) > - goto out; > - } > + err = fuse_writeback_range(inode, offset, endbyte); > + if (err) > + goto out; > } > > if (!(mode & FALLOC_FL_KEEP_SIZE) && > @@ -3039,8 +3035,7 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset, > if (block_faults) > filemap_invalidate_unlock(inode->i_mapping); > > - if (lock_inode) > - inode_unlock(inode); > + inode_unlock(inode); > > fuse_flush_time_update(inode); > > -- > 2.38.1 >
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 71bfb663aac5..89f4741728ba 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -2963,11 +2963,9 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset, .mode = mode }; int err; - bool lock_inode = !(mode & FALLOC_FL_KEEP_SIZE) || - (mode & (FALLOC_FL_PUNCH_HOLE | - FALLOC_FL_ZERO_RANGE)); - - bool block_faults = FUSE_IS_DAX(inode) && lock_inode; + bool block_faults = FUSE_IS_DAX(inode) && + (!(mode & FALLOC_FL_KEEP_SIZE) || + (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE))); if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) @@ -2976,22 +2974,20 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset, if (fm->fc->no_fallocate) return -EOPNOTSUPP; - if (lock_inode) { - inode_lock(inode); - if (block_faults) { - filemap_invalidate_lock(inode->i_mapping); - err = fuse_dax_break_layouts(inode, 0, 0); - if (err) - goto out; - } + inode_lock(inode); + if (block_faults) { + filemap_invalidate_lock(inode->i_mapping); + err = fuse_dax_break_layouts(inode, 0, 0); + if (err) + goto out; + } - if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) { - loff_t endbyte = offset + length - 1; + if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) { + loff_t endbyte = offset + length - 1; - err = fuse_writeback_range(inode, offset, endbyte); - if (err) - goto out; - } + err = fuse_writeback_range(inode, offset, endbyte); + if (err) + goto out; } if (!(mode & FALLOC_FL_KEEP_SIZE) && @@ -3039,8 +3035,7 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset, if (block_faults) filemap_invalidate_unlock(inode->i_mapping); - if (lock_inode) - inode_unlock(inode); + inode_unlock(inode); fuse_flush_time_update(inode);
file_modified() must be called with inode lock held. fuse_fallocate() didn't lock the inode in case of just FALLOC_KEEP_SIZE flags value, which resulted in a kernel Warning in notify_change(). Lock the inode unconditionally, like all other fallocate implementations do. Reported-by: Pengfei Xu <pengfei.xu@intel.com> Reported-and-tested-by: syzbot+462da39f0667b357c4b6@syzkaller.appspotmail.com Fixes: 4a6f278d4827 ("fuse: add file_modified() to fallocate") Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/fuse/file.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)