The original minix iomap conversion patches had a couple of issues exposed by syzbot: 1. NULL pointer dereferences in page_symlink. Since the iomap based minix_aops no longer used write_begin and write_end, when page_link attempted to call aops->write_begin there was a crash because there was no longer a pointer assigned to aops->write_begin. This was fixed with a custom function that writes the symlink target directly to a newly allocated data block acquired with minix_new_block and sb_getblk, thus bypassing the aops write path entirely. The symlink read path was in turn replaced with a new minix_get_link that reads the target from the first data block directly with sb_bread, similar to how ext4_get_link does it. 2. Truncate crashed. There was a conflict in minix's truncate() function between buffer_head and iomap where block_truncate_page was attaching buffer_heads to folios, but this ran up against how iomap works. Fixed this by using iomap_truncate_page for inodes using minix_aops and block_truncate_page for inodes with minix_dir_aops. Also, minix_symlink_inode_operations now has .setattr = minix_setattr so symlinks can be truncated more properly through iomap rather than using the default simple_setattr. Signed-off-by: Jeremy Bingham --- fs/minix/inode.c | 4 +- fs/minix/itree_common.c | 11 +++- fs/minix/minix.h | 5 +- fs/minix/namei.c | 137 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 152 insertions(+), 5 deletions(-) diff --git a/fs/minix/inode.c b/fs/minix/inode.c index 2ba6766fce51..b113c44764ff 100644 --- a/fs/minix/inode.c +++ b/fs/minix/inode.c @@ -573,8 +573,9 @@ static const struct address_space_operations minix_dir_aops = { }; static const struct inode_operations minix_symlink_inode_operations = { - .get_link = page_get_link, + .get_link = minix_get_link, .getattr = minix_getattr, + .setattr = minix_setattr, }; void minix_set_inode(struct inode *inode, dev_t rdev) @@ -838,4 +839,3 @@ module_init(init_minix_fs) module_exit(exit_minix_fs) MODULE_DESCRIPTION("Minix file system"); MODULE_LICENSE("GPL"); - diff --git a/fs/minix/itree_common.c b/fs/minix/itree_common.c index c3cd2c75af9c..5a8b73a7beda 100644 --- a/fs/minix/itree_common.c +++ b/fs/minix/itree_common.c @@ -311,7 +311,16 @@ static inline void truncate (struct inode * inode) long iblock; iblock = (inode->i_size + sb->s_blocksize -1) >> sb->s_blocksize_bits; - block_truncate_page(inode->i_mapping, inode->i_size, get_block); + + /* Depending on what address space operations are being used by the + * inode being truncated, we need to either call iomap_truncate_page or + * block_truncate_page. + */ + if (inode->i_mapping->a_ops == &minix_aops) + iomap_truncate_page(inode, inode->i_size, NULL, + minix_iomap_ops_ver(inode), NULL, NULL); + else + block_truncate_page(inode->i_mapping, inode->i_size, get_block); n = block_to_path(inode, iblock, offsets); if (!n) diff --git a/fs/minix/minix.h b/fs/minix/minix.h index 76718f789369..d1a890e96abe 100644 --- a/fs/minix/minix.h +++ b/fs/minix/minix.h @@ -57,7 +57,7 @@ int minix_new_block(struct inode *inode); void minix_free_block(struct inode *inode, unsigned long block); unsigned long minix_count_free_blocks(struct super_block *sb); int minix_getattr(struct mnt_idmap *, const struct path *, - struct kstat *, u32, unsigned int); + struct kstat *, u32, unsigned); int minix_setattr(struct mnt_idmap *idmap, struct dentry *dentry, struct iattr *attr); int minix_prepare_chunk(struct folio *folio, loff_t pos, unsigned len); @@ -82,6 +82,9 @@ int minix_make_empty(struct inode*, struct inode*); int minix_empty_dir(struct inode*); int minix_set_link(struct minix_dir_entry *de, struct folio *folio, struct inode *inode); +extern const char *minix_get_link(struct dentry *dentry, struct inode *inode, + struct delayed_call *callback); + struct minix_dir_entry *minix_dotdot(struct inode*, struct folio **); ino_t minix_inode_by_name(struct dentry*); diff --git a/fs/minix/namei.c b/fs/minix/namei.c index 263e4ba8b1c8..e245f55a68ff 100644 --- a/fs/minix/namei.c +++ b/fs/minix/namei.c @@ -6,6 +6,8 @@ */ #include "minix.h" +#include +#include static int add_nondir(struct dentry *dentry, struct inode *inode) { @@ -69,6 +71,101 @@ static int minix_create(struct mnt_idmap *idmap, struct inode *dir, return minix_mknod(&nop_mnt_idmap, dir, dentry, mode, 0); } +static inline u16 *v1_i_data(struct inode *inode) +{ + return (u16 *)minix_i(inode)->u.i1_data; +} + +static inline u32 *v2_i_data(struct inode *inode) +{ + return (u32 *)minix_i(inode)->u.i2_data; +} + +static inline u16 cpu_to_v1_block(sector_t n) +{ + return n; +} + +static inline u32 cpu_to_v2_block(sector_t n) +{ + return n; +} + +static inline sector_t v1_block_to_cpu(u16 n) +{ + return n; +} + +static inline sector_t v2_block_to_cpu(u32 n) +{ + return n; +} + +/* Reimplement page_symlink's general logic while avoiding using buffer head + * based aops operations like aops->write_begin so things behave better with + * the new regime of iomap based aops operations. Cribbing from page_symlink in + * fs/namei.c and ext4's ext4_init_symlink_block. + */ +static int __page_symlink(struct inode *inode, const char *symname, int len) +{ + struct super_block *sb = inode->i_sb; + struct buffer_head *bh; + char *kaddr; + int err = 0; + u16 *p16; /* v1 16 bit block */ + u32 *p32; /* v2/3 32 bit block */ + + sector_t phys; + + phys = minix_new_block(inode); + if (!phys) { + err = -ENOSPC; + goto ps_out; + } + + if (INODE_VERSION(inode) == MINIX_V1) { + p16 = v1_i_data(inode); + *p16 = cpu_to_v1_block(phys); + } else { + p32 = v2_i_data(inode); + *p32 = cpu_to_v2_block(phys); + } + + bh = sb_getblk(sb, phys); + if (!bh) { + err = -ENOMEM; + goto ps_fail; + } + + lock_buffer(bh); + kaddr = (char *)bh->b_data; + memset(kaddr, 0, sb->s_blocksize); + memcpy(kaddr, symname, len); + inode->i_size = len - 1; + set_buffer_uptodate(bh); + unlock_buffer(bh); + + mmb_mark_buffer_dirty(bh, &minix_i(inode)->i_metadata_bhs); + if (inode_needs_sync(inode)) { + sync_dirty_buffer(bh); + if (buffer_req(bh) && !buffer_uptodate(bh)) { + pr_err("i/o error syncing itable block"); + err = -EIO; + } + + } + + mark_inode_dirty(inode); + brelse(bh); + +ps_out: + return err; + +ps_fail: + minix_free_block(inode, phys); + goto ps_out; +} + static int minix_symlink(struct mnt_idmap *idmap, struct inode *dir, struct dentry *dentry, const char *symname) { @@ -84,7 +181,7 @@ static int minix_symlink(struct mnt_idmap *idmap, struct inode *dir, return PTR_ERR(inode); minix_set_inode(inode, 0); - err = page_symlink(inode, symname, i); + err = __page_symlink(inode, symname, i); if (unlikely(err)) { inode_dec_link_count(inode); iput(inode); @@ -273,6 +370,44 @@ static int minix_rename(struct mnt_idmap *idmap, return err; } +/* straight up thievery here; stolen verbatim from ext4_get_link */ +static void minix_free_link(void *bh) +{ + brelse(bh); +} + +/* Borrowing from ext4_get_link to a degree; since minix inodes and symlinks + * are significantly simpler, we don't need to do nearly as much as ext4 + * requires for old-timey ext4 slow links. + */ +const char *minix_get_link(struct dentry *dentry, struct inode *inode, + struct delayed_call *callback) +{ + struct super_block *sb = inode->i_sb; + struct buffer_head *bh; + sector_t blk; + + /* Get yon block, depending on what version of the minix fs this is. */ + if (INODE_VERSION(inode) == MINIX_V1) + blk = v1_block_to_cpu(*(v1_i_data(inode))); + else + blk = v2_block_to_cpu(*(v2_i_data(inode))); + + bh = sb_bread(sb, blk); + if (IS_ERR(bh)) + return ERR_CAST(bh); + if (!bh) { + pr_err("bad symlink on inode %llu", inode->i_ino); + return ERR_PTR(-EFSCORRUPTED); + } + + set_delayed_call(callback, minix_free_link, bh); + nd_terminate_link(bh->b_data, inode->i_size, + inode->i_sb->s_blocksize - 1); + + return bh->b_data; +} + /* * directories can handle most operations... */ -- 2.47.3