linux-kernel - Re: [PATCH RESEND 2/7] xfs: add support FALLOC_FL_COLLAPSE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 11 Oct 2013 08:23:26 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Namjae Jeon <linkinjeon@...il.com>
Cc:	viro@...iv.linux.org.uk, mtk.manpages@...il.com, tytso@....edu,
	adilger.kernel@...ger.ca, bpm@....com, elder@...nel.org,
	hch@...radead.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-ext4@...r.kernel.org,
	xfs@....sgi.com, a.sangwan@...sung.com,
	Namjae Jeon <namjae.jeon@...sung.com>
Subject: Re: [PATCH RESEND 2/7] xfs: add support FALLOC_FL_COLLAPSE_RANGE for
 fallocate

On Thu, Oct 10, 2013 at 04:00:13PM +0900, Namjae Jeon wrote:
> >
> > /*
> >  * Shift extent records to the left to cover a hole.
> >  *
> >  * The maximum number of extents to be shifted in a single operation
> >  * is @count, and @current_ext keeps track of the current extent
> >  * index we have shifted. If there is no hole to shift the extents
> >  * into, then we abort immediately.
> >  */
> Thanks for your help. I will change this comment instead of original one.
> 
> >> +int
> >> +xfs_bmap_shift_extents(
> >> +     struct xfs_trans        *tp,
> >> +     struct xfs_inode        *ip,
> >> +     int                     *done,
> >> +     xfs_fileoff_t           start_fsb,
> >> +     xfs_fileoff_t           shift,
> >
> > Shift means ...? Number of extents to shift, a length, a number of
> > block, or something else?
> Ah, yes, shift_len would be a more proper name

I'm not sure that's a lot better. What are we shifting? We are
shifting the offset of the blocks, right? And the unit is in fsb?
So perhaps offset_shift_fsb, and add that to the description of the
function above?

> >> +             /*
> >> +              * Before shifting extent into hole, make sure that the hole
> >> +              * is large enough to accomodate the shift.
> >> +              */
> >> +             if (*current_ext) {
> >> +                     state |= BMAP_LEFT_VALID;
> >> +                     xfs_bmbt_get_all(xfs_iext_get_ext(ifp,
> >> +                                             *current_ext - 1), &left);
> >> +
> >> +                     if (isnullstartblock(left.br_startblock))
> >> +                             state |= BMAP_LEFT_DELAY;
> >> +
> >> +                     if (startoff < left.br_startoff + left.br_blockcount)
> >> +                             error = XFS_ERROR(EFSCORRUPTED);
> >
> > Why is the filesystem corrupted if the shift we asked for is too
> > large for the hole in the file? I haven't seen any checks before
> > this that guarantee that the hole is big enough for the shift...
> 
> we call xfs_free_file_space to free enough blocks for shifting.
> If still the space is not big enough will it be considered as fs corrupted?
> What error could we return in this case?

Hole punching rounds inwards, and the amount of rounding is not
necessarily the nearest filesystem block. Again it's the block size
smaller than page size case that will trip you over here, as the
rounding  when punching holes will be done to page size, not
filesystem block size. Hence it's entirely possible that your
calculated shift start and lengths don't match the size of the hole
that was punched.

That doesn't mean there was a corruption - just that the hole wasn't
the size and shape that was expected....

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/