lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <YtcjkMcYQCATlt0Y@debian-BULLSEYE-live-builder-AMD64> Date: Tue, 19 Jul 2022 17:35:12 -0400 From: Eric Whitney <enwlinux@...il.com> To: Jan Kara <jack@...e.cz> Cc: Eric Whitney <enwlinux@...il.com>, linux-ext4@...r.kernel.org, tytso@....edu Subject: Re: [PATCH] ext4: minor defrag code improvements * Jan Kara <jack@...e.cz>: > On Tue 21-06-22 10:33:40, Eric Whitney wrote: > > Modify two error paths returning EBUSY for bad argument file types to > > return EOPNOTSUPP instead. Move an extent tree search whose results are > > only occasionally required to the site always requiring them for > > improved efficiency. Address a few typos. > > > > Signed-off-by: Eric Whitney <enwlinux@...il.com> > > So why is EOPNOTSUPP better than EBUSY? Honestly we are rather inconsistent > with errors returned for various operations on swapfile - > read/write/fallocate/truncate return ETXTBSY, unlink returns EPERM, some > ext4 ioctls return EINVAL... I guess ETXTBSY is the most common return > value? > > Otherwise the patch looks good. > > Honza Hi Jan - thanks for your review. I think EOPNOTSUPP is better than EBUSY when EXT4_IOC_MOVE_EXT is applied to a swap file because it's a much more direct message indicating that ext4 doesn't support swap file defragmentation. With the exception of the two sites modified by this patch, all the other sites in the defrag code where checks are made for unsupported file types or file system configurations, EOPNOTSUPP is returned. Because EBUSY really doesn't seem to convey what's happening here very well - this isn't so much a case of a potentially temporarily busy resource as an attempt to perform an operation that will never succeed - another choice seemed more appropriate. I picked EOPNOTSUPP simply because it's used in those other sites in the defrag code, and was trying to be consistent. (The defrag code reports EBUSYs when it can't release pages with references on them.) EINVAL could be an alternative. It's not quite as direct but it does convey the idea that an argument to the ioctl is wrong. The defrag code uses it (a little inconsistently) when argument values are out of range or otherwise invalid. Is ETXTBUSY still reported by the kernel? I couldn't find it in a search after reading this: lwn.net/Articles/866493/ I didn't consider that because an executable wasn't involved - interesting that it was used for some operations applied to swap files. At any rate, I'm open to suggestions here - just trying to clean up a few things after a little code review. Thanks, Eric > > > --- > > fs/ext4/move_extent.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c > > index 701f1d6a217f..4e4b0452106e 100644 > > --- a/fs/ext4/move_extent.c > > +++ b/fs/ext4/move_extent.c > > @@ -472,19 +472,17 @@ mext_check_arguments(struct inode *orig_inode, > > if (IS_IMMUTABLE(donor_inode) || IS_APPEND(donor_inode)) > > return -EPERM; > > > > - /* Ext4 move extent does not support swapfile */ > > + /* Ext4 move extent does not support swap files */ > > if (IS_SWAPFILE(orig_inode) || IS_SWAPFILE(donor_inode)) { > > - ext4_debug("ext4 move extent: The argument files should " > > - "not be swapfile [ino:orig %lu, donor %lu]\n", > > + ext4_debug("ext4 move extent: The argument files should not be swap files [ino:orig %lu, donor %lu]\n", > > orig_inode->i_ino, donor_inode->i_ino); > > - return -EBUSY; > > + return -EOPNOTSUPP; > > } > > > > if (ext4_is_quota_file(orig_inode) && ext4_is_quota_file(donor_inode)) { > > - ext4_debug("ext4 move extent: The argument files should " > > - "not be quota files [ino:orig %lu, donor %lu]\n", > > + ext4_debug("ext4 move extent: The argument files should not be quota files [ino:orig %lu, donor %lu]\n", > > orig_inode->i_ino, donor_inode->i_ino); > > - return -EBUSY; > > + return -EOPNOTSUPP; > > } > > > > /* Ext4 move extent supports only extent based file */ > > @@ -631,11 +629,11 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, > > if (ret) > > goto out; > > ex = path[path->p_depth].p_ext; > > - next_blk = ext4_ext_next_allocated_block(path); > > cur_blk = le32_to_cpu(ex->ee_block); > > cur_len = ext4_ext_get_actual_len(ex); > > /* Check hole before the start pos */ > > if (cur_blk + cur_len - 1 < o_start) { > > + next_blk = ext4_ext_next_allocated_block(path); > > if (next_blk == EXT_MAX_BLOCKS) { > > ret = -ENODATA; > > goto out; > > @@ -663,7 +661,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, > > donor_page_index = d_start >> (PAGE_SHIFT - > > donor_inode->i_blkbits); > > offset_in_page = o_start % blocks_per_page; > > - if (cur_len > blocks_per_page- offset_in_page) > > + if (cur_len > blocks_per_page - offset_in_page) > > cur_len = blocks_per_page - offset_in_page; > > /* > > * Up semaphore to avoid following problems: > > -- > > 2.30.2 > > > -- > Jan Kara <jack@...e.com> > SUSE Labs, CR
Powered by blists - more mailing lists