From 2251588143f65636cf3f3f12beb009084fa2d5d7 Mon Sep 17 00:00:00 2001 From: Shigeru Yoshida Date: Mon, 21 Aug 2023 13:33:12 +0900 Subject: reiserfs: Replace 1-element array with C99 style flex-array UBSAN found the following issue: ================================================================================ UBSAN: array-index-out-of-bounds in fs/reiserfs/journal.c:4166:22 index 1 is out of range for type '__le32 [1]' This is because struct reiserfs_journal_desc uses 1-element array for dynamically sized array member, j_realblock. This patch fixes this issue by replacing the 1-element array member with C99 style flex-array. This patch also fixes the same issue in struct reiserfs_journal_commit as the same manner. Fixes: f466c6fdb3b1 ("move private bits of reiserfs_fs.h to fs/reiserfs/reiserfs.h") Signed-off-by: Shigeru Yoshida Message-Id: <20230821043312.1444068-1-syoshida@redhat.com> Signed-off-by: Christian Brauner --- fs/reiserfs/reiserfs.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/reiserfs/reiserfs.h b/fs/reiserfs/reiserfs.h index b81749492ef9..7d12b8c5b2fa 100644 --- a/fs/reiserfs/reiserfs.h +++ b/fs/reiserfs/reiserfs.h @@ -2699,7 +2699,7 @@ struct reiserfs_iget_args { #define get_journal_desc_magic(bh) (bh->b_data + bh->b_size - 12) #define journal_trans_half(blocksize) \ - ((blocksize - sizeof (struct reiserfs_journal_desc) + sizeof (__u32) - 12) / sizeof (__u32)) + ((blocksize - sizeof(struct reiserfs_journal_desc) - 12) / sizeof(__u32)) /* journal.c see journal.c for all the comments here */ @@ -2711,7 +2711,7 @@ struct reiserfs_journal_desc { __le32 j_len; __le32 j_mount_id; /* mount id of this trans */ - __le32 j_realblock[1]; /* real locations for each block */ + __le32 j_realblock[]; /* real locations for each block */ }; #define get_desc_trans_id(d) le32_to_cpu((d)->j_trans_id) @@ -2726,7 +2726,7 @@ struct reiserfs_journal_desc { struct reiserfs_journal_commit { __le32 j_trans_id; /* must match j_trans_id from the desc block */ __le32 j_len; /* ditto */ - __le32 j_realblock[1]; /* real locations for each block */ + __le32 j_realblock[]; /* real locations for each block */ }; #define get_commit_trans_id(c) le32_to_cpu((c)->j_trans_id) -- cgit v1.2.3 From 8287474aa5ffb41df52552c4ae4748e791d2faf2 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 13 Sep 2023 18:28:15 +0200 Subject: direct_write_fallback(): on error revert the ->ki_pos update from buffered write If we fail filemap_write_and_wait_range() on the range the buffered write went into, we only report the "number of bytes which we direct-written", to quote the comment in there. Which is fine, but buffered write has already advanced iocb->ki_pos, so we need to roll that back. Otherwise we end up with e.g. write(2) advancing position by more than the amount it reports having written. Fixes: 182c25e9c157 "filemap: update ki_pos in generic_perform_write" Signed-off-by: Al Viro Message-Id: <20230827214518.GU3390869@ZenIV> Signed-off-by: Christian Brauner --- fs/libfs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/libfs.c b/fs/libfs.c index a4eb12757886..37f2d34ee090 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1903,6 +1903,7 @@ ssize_t direct_write_fallback(struct kiocb *iocb, struct iov_iter *iter, * We don't know how much we wrote, so just return the number of * bytes which were direct-written */ + iocb->ki_pos -= buffered_written; if (direct_written) return direct_written; return err; -- cgit v1.2.3 From db7fcc884d8a1e8265a87306e728c3d3239b0ca2 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 15 Sep 2023 13:14:14 -0700 Subject: aio: Annotate struct kioctx_table with __counted_by Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by for struct kioctx_table. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Cc: Benjamin LaHaise Cc: Alexander Viro Cc: Christian Brauner Cc: linux-aio@kvack.org Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Kees Cook Reviewed-by: "Gustavo A. R. Silva" Message-Id: <20230915201413.never.881-kees@kernel.org> Signed-off-by: Christian Brauner --- fs/aio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/aio.c b/fs/aio.c index a4c2a6bac72c..f8589caef9c1 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -80,7 +80,7 @@ struct aio_ring { struct kioctx_table { struct rcu_head rcu; unsigned nr; - struct kioctx __rcu *table[]; + struct kioctx __rcu *table[] __counted_by(nr); }; struct kioctx_cpu { -- cgit v1.2.3 From be049c3a088d512187407b7fd036cecfab46d565 Mon Sep 17 00:00:00 2001 From: Chunhai Guo Date: Fri, 15 Sep 2023 22:51:31 -0600 Subject: fs-writeback: do not requeue a clean inode having skipped pages When writing back an inode and performing an fsync on it concurrently, a deadlock issue may arise as shown below. In each writeback iteration, a clean inode is requeued to the wb->b_dirty queue due to non-zero pages_skipped, without anything actually being written. This causes an infinite loop and prevents the plug from being flushed, resulting in a deadlock. We now avoid requeuing the clean inode to prevent this issue. wb_writeback fsync (inode-Y) blk_start_plug(&plug) for (;;) { iter i-1: some reqs with page-X added into plug->mq_list // f2fs node page-X with PG_writeback filemap_fdatawrite __filemap_fdatawrite_range // write inode-Y with sync_mode WB_SYNC_ALL do_writepages f2fs_write_data_pages __f2fs_write_data_pages // wb_sync_req[DATA]++ for WB_SYNC_ALL f2fs_write_cache_pages f2fs_write_single_data_page f2fs_do_write_data_page f2fs_outplace_write_data f2fs_update_data_blkaddr f2fs_wait_on_page_writeback wait_on_page_writeback // wait for f2fs node page-X iter i: progress = __writeback_inodes_wb(wb, work) . writeback_sb_inodes . __writeback_single_inode // write inode-Y with sync_mode WB_SYNC_NONE . . do_writepages . . f2fs_write_data_pages . . . __f2fs_write_data_pages // skip writepages due to (wb_sync_req[DATA]>0) . . . wbc->pages_skipped += get_dirty_pages(inode) // wbc->pages_skipped = 1 . if (!(inode->i_state & I_DIRTY_ALL)) // i_state = I_SYNC | I_SYNC_QUEUED . total_wrote++; // total_wrote = 1 . requeue_inode // requeue inode-Y to wb->b_dirty queue due to non-zero pages_skipped if (progress) // progress = 1 continue; iter i+1: queue_io // similar process with iter i, infinite for-loop ! } blk_finish_plug(&plug) // flush plug won't be called Signed-off-by: Chunhai Guo Reviewed-by: Jan Kara Message-Id: <20230916045131.957929-1-guochunhai@vivo.com> Signed-off-by: Christian Brauner --- fs/fs-writeback.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 969ce991b0b0..c1af01b2c42d 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -1535,10 +1535,15 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb, if (wbc->pages_skipped) { /* - * writeback is not making progress due to locked - * buffers. Skip this inode for now. + * Writeback is not making progress due to locked buffers. + * Skip this inode for now. Although having skipped pages + * is odd for clean inodes, it can happen for some + * filesystems so handle that gracefully. */ - redirty_tail_locked(inode, wb); + if (inode->i_state & I_DIRTY_ALL) + redirty_tail_locked(inode, wb); + else + inode_cgwb_move_to_attached(inode, wb); return; } -- cgit v1.2.3 From ae81711c1edd769b7d9952dde40a579dceca4815 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 19 Sep 2023 09:40:44 +0200 Subject: fs/pipe: remove duplicate "offset" initializer This code duplication was introduced by commit a194dfe6e6f6 ("pipe: Rearrange sequence in pipe_write() to preallocate slot"), but since the pipe's mutex is locked, nobody else can modify the value meanwhile. Signed-off-by: Max Kellermann Message-Id: <20230919074045.1066796-1-max.kellermann@ionos.com> Signed-off-by: Christian Brauner --- fs/pipe.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/pipe.c b/fs/pipe.c index 6c1a9b1db907..139190165a1c 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -537,7 +537,6 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) break; } ret += copied; - buf->offset = 0; buf->len = copied; if (!iov_iter_count(from)) -- cgit v1.2.3 From 2ba0dd6562f2c42ef1ae61145bdfc882fc7a6f79 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 15 Sep 2023 16:01:02 +0200 Subject: porting: document new block device opening order We've changed the order of opening block devices and superblock handling. Let's document this so filesystem and vfs developers have a proper digital paper trail. Signed-off-by: Christian Brauner --- Documentation/filesystems/porting.rst | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst index deac4e973ddc..787d10b9e8b5 100644 --- a/Documentation/filesystems/porting.rst +++ b/Documentation/filesystems/porting.rst @@ -949,3 +949,29 @@ mmap_lock held. All in-tree users have been audited and do not seem to depend on the mmap_lock being held, but out of tree users should verify for themselves. If they do need it, they can return VM_FAULT_RETRY to be called with the mmap_lock held. + +--- + +**mandatory** + +The order of opening block devices and matching or creating superblocks has +changed. + +The old logic opened block devices first and then tried to find a +suitable superblock to reuse based on the block device pointer. + +The new logic tries to find a suitable superblock first based on the device +number, and opening the block device afterwards. + +Since opening block devices cannot happen under s_umount because of lock +ordering requirements s_umount is now dropped while opening block devices and +reacquired before calling fill_super(). + +In the old logic concurrent mounters would find the superblock on the list of +superblocks for the filesystem type. Since the first opener of the block device +would hold s_umount they would wait until the superblock became either born or +was discarded due to initialization failure. + +Since the new logic drops s_umount concurrent mounters could grab s_umount and +would spin. Instead they are now made to wait using an explicit wait-wake +mechanism without having to hold s_umount. -- cgit v1.2.3 From 060e6c7d179ed2f2088a23ceedf60d63320e9311 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 15 Sep 2023 16:01:40 +0200 Subject: porting: document superblock as block device holder We've changed the holder of the block device which has consequences. Document this clearly and in detail so filesystem and vfs developers have a proper digital paper trail. Signed-off-by: Christian Brauner --- Documentation/filesystems/porting.rst | 70 +++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst index 787d10b9e8b5..4d05b9862451 100644 --- a/Documentation/filesystems/porting.rst +++ b/Documentation/filesystems/porting.rst @@ -975,3 +975,73 @@ was discarded due to initialization failure. Since the new logic drops s_umount concurrent mounters could grab s_umount and would spin. Instead they are now made to wait using an explicit wait-wake mechanism without having to hold s_umount. + +--- + +**mandatory** + +The holder of a block device is now the superblock. + +The holder of a block device used to be the file_system_type which wasn't +particularly useful. It wasn't possible to go from block device to owning +superblock without matching on the device pointer stored in the superblock. +This mechanism would only work for a single device so the block layer couldn't +find the owning superblock of any additional devices. + +In the old mechanism reusing or creating a superblock for a racing mount(2) and +umount(2) relied on the file_system_type as the holder. This was severly +underdocumented however: + +(1) Any concurrent mounter that managed to grab an active reference on an + existing superblock was made to wait until the superblock either became + ready or until the superblock was removed from the list of superblocks of + the filesystem type. If the superblock is ready the caller would simple + reuse it. + +(2) If the mounter came after deactivate_locked_super() but before + the superblock had been removed from the list of superblocks of the + filesystem type the mounter would wait until the superblock was shutdown, + reuse the block device and allocate a new superblock. + +(3) If the mounter came after deactivate_locked_super() and after + the superblock had been removed from the list of superblocks of the + filesystem type the mounter would reuse the block device and allocate a new + superblock (the bd_holder point may still be set to the filesystem type). + +Because the holder of the block device was the file_system_type any concurrent +mounter could open the block devices of any superblock of the same +file_system_type without risking seeing EBUSY because the block device was +still in use by another superblock. + +Making the superblock the owner of the block device changes this as the holder +is now a unique superblock and thus block devices associated with it cannot be +reused by concurrent mounters. So a concurrent mounter in (2) could suddenly +see EBUSY when trying to open a block device whose holder was a different +superblock. + +The new logic thus waits until the superblock and the devices are shutdown in +->kill_sb(). Removal of the superblock from the list of superblocks of the +filesystem type is now moved to a later point when the devices are closed: + +(1) Any concurrent mounter managing to grab an active reference on an existing + superblock is made to wait until the superblock is either ready or until + the superblock and all devices are shutdown in ->kill_sb(). If the + superblock is ready the caller will simply reuse it. + +(2) If the mounter comes after deactivate_locked_super() but before + the superblock has been removed from the list of superblocks of the + filesystem type the mounter is made to wait until the superblock and the + devices are shut down in ->kill_sb() and the superblock is removed from the + list of superblocks of the filesystem type. The mounter will allocate a new + superblock and grab ownership of the block device (the bd_holder pointer of + the block device will be set to the newly allocated superblock). + +(3) This case is now collapsed into (2) as the superblock is left on the list + of superblocks of the filesystem type until all devices are shutdown in + ->kill_sb(). In other words, if the superblock isn't on the list of + superblock of the filesystem type anymore then it has given up ownership of + all associated block devices (the bd_holder pointer is NULL). + +As this is a VFS level change it has no practical consequences for filesystems +other than that all of them must use one of the provided kill_litter_super(), +kill_anon_super(), or kill_block_super() helpers. -- cgit v1.2.3 From 2d1b3bbc3dd56fc8364350eb93e5d67a05cb2c23 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Mon, 25 Sep 2023 00:21:35 -0600 Subject: ovl: disable IOCB_DIO_CALLER_COMP overlayfs copies the kiocb flags when it sets up a new kiocb to handle a write, but it doesn't properly support dealing with the deferred caller completions of the kiocb. This means it doesn't get the final write completion value, and hence will complete the write with '0' as the result. We could support the caller completions in overlayfs, but for now let's just disable them in the generated write kiocb. Reported-by: Zorro Lang Link: https://lore.kernel.org/io-uring/20230924142754.ejwsjen5pvyc32l4@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/ Fixes: 8c052fb3002e ("iomap: support IOCB_DIO_CALLER_COMP") Signed-off-by: Jens Axboe Message-Id: <71897125-e570-46ce-946a-d4729725e28f@kernel.dk> Signed-off-by: Christian Brauner --- fs/overlayfs/file.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 3b4cc633d763..ca18db79daf4 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -393,6 +393,12 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter) if (!ovl_should_sync(OVL_FS(inode->i_sb))) ifl &= ~(IOCB_DSYNC | IOCB_SYNC); + /* + * Overlayfs doesn't support deferred completions, don't copy + * this property in case it is set by the issuer. + */ + ifl &= ~IOCB_DIO_CALLER_COMP; + old_cred = ovl_override_creds(file_inode(file)->i_sb); if (is_sync_kiocb(iocb)) { file_start_write(real.file); -- cgit v1.2.3 From 493c71926c20309226b6d73f6b661a9813de5f0b Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Mon, 25 Sep 2023 13:08:52 +0200 Subject: ntfs3: put resources during ntfs_fill_super() During ntfs_fill_super() some resources are allocated that we need to cleanup in ->put_super() such as additional inodes. When ntfs_fill_super() fails these resources need to be cleaned up as well. Reported-by: syzbot+2751da923b5eb8307b0b@syzkaller.appspotmail.com Fixes: 78a06688a4d4 ("ntfs3: drop inode references in ntfs_put_super()") Signed-off-by: Christian Brauner --- fs/ntfs3/super.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c index cfec5e0c7f66..5661a363005e 100644 --- a/fs/ntfs3/super.c +++ b/fs/ntfs3/super.c @@ -1562,6 +1562,7 @@ load_root: put_inode_out: iput(inode); out: + ntfs3_put_sbi(sbi); kfree(boot2); return err; } -- cgit v1.2.3 From 03dbab3bba5f009d053635c729d1244f2c8bad38 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Wed, 13 Sep 2023 09:33:12 -0400 Subject: overlayfs: set ctime when setting mtime and atime Nathan reported that he was seeing the new warning in setattr_copy_mgtime pop when starting podman containers. Overlayfs is trying to set the atime and mtime via notify_change without also setting the ctime. POSIX states that when the atime and mtime are updated via utimes() that we must also update the ctime to the current time. The situation with overlayfs copy-up is analogies, so add ATTR_CTIME to the bitmask. notify_change will fill in the value. Reported-by: Nathan Chancellor Signed-off-by: Jeff Layton Tested-by: Nathan Chancellor Acked-by: Christian Brauner Acked-by: Amir Goldstein Message-Id: <20230913-ctime-v1-1-c6bc509cbc27@kernel.org> Signed-off-by: Christian Brauner --- fs/overlayfs/copy_up.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index bae404a1bad4..557e46bc4361 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -337,7 +337,7 @@ static int ovl_set_timestamps(struct ovl_fs *ofs, struct dentry *upperdentry, { struct iattr attr = { .ia_valid = - ATTR_ATIME | ATTR_MTIME | ATTR_ATIME_SET | ATTR_MTIME_SET, + ATTR_ATIME | ATTR_MTIME | ATTR_ATIME_SET | ATTR_MTIME_SET | ATTR_CTIME, .ia_atime = stat->atime, .ia_mtime = stat->mtime, }; -- cgit v1.2.3