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: <YtgbKhYxbX4NPJts@debian-BULLSEYE-live-builder-AMD64> Date: Wed, 20 Jul 2022 11:11:38 -0400 From: Eric Whitney <enwlinux@...il.com> To: Eric Whitney <enwlinux@...il.com> Cc: Jan Kara <jack@...e.cz>, linux-ext4@...r.kernel.org, tytso@....edu Subject: Re: [PATCH] ext4: minor defrag code improvements * Eric Whitney <enwlinux@...il.com>: > * 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. And of course I botched my search - ETXTBSY, not ETXTBUSY. 50+ instances remain in the kernel. So, EOPNOTSUPP or EINVAL would be a clearer indication that the move extents ioctl is being applied to a file type it can't defrag. I can go with ETXTBSY if you really prefer - any of these is better than EBUSY in this case, IMHO. Eric > > 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