| 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