linux-ext4 - Re: [PATCH] vfs: in iomap seek_{hole,data}, return -ENXIO for negative offsets

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] [day] [month] [year] [list]

Message-ID: <20170713171026.GB4151@magnolia>
Date:   Thu, 13 Jul 2017 10:10:26 -0700
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        xfs <linux-xfs@...r.kernel.org>,
        linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] vfs: in iomap seek_{hole,data}, return -ENXIO for
 negative offsets

On Wed, Jul 12, 2017 at 11:08:51PM -0700, Andreas Dilger wrote:
> On Jul 12, 2017, at 10:36 AM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> > 
> > In the iomap implementations of SEEK_HOLE and SEEK_DATA, make sure we
> > return -ENXIO for negative offsets instead of badgering the iomap
> > provider with garbage requests.
> 
> Hmm, wouldn't it be useful to be able to seek N bytes before the hole
> or data?

Yes, but you can do that with a first lseek SEEK_DATA call to position
us at the start of data followed by a second lseek SEEK_CUR to position
us at the desired before-data offset.  The offset argument to SEEK DATA
tells us where to start looking for the next hole/not-hole.

> It would make more sense to verify the result after finding the hole
> or data and adding the relative offset. It should only be an error if
> the resulting offset is before the start or after the actual file end.

In any case, the manpage says:

"SEEK_DATA
Adjust the file offset to the next location in the file greater than or
equal to offset containing data.  If offset points to data, then the
file offset is set to offset.

"SEEK_HOLE
Adjust the file offset to the next hole in the file greater than or
equal to offset.  If offset points into the middle of a hole, then the
file offset is set to offset.  If there is no hole past offset, then the
file offset is adjusted to the end of the file (i.e., there is an
implicit hole at the end of any file)."

...which to my mind means that offset has to be an absolute file offset.
Therefore, negative offsets aren't allowed, and we're stuck with that.

Regrettably the ERRORS section doesn't specify the behavior when offset
is negative, so this patch merely fixes XFS to behave the same way it
did up to 4.12 (which fwiw matches btrfs behavior).

--D

> Of course, if the passed offset > size from the start then
> there is no way it can get better and that would be grounds for
> returning early.
> 
> Cheers, Andreas
> 
> > Inspired-by: Mateusz S <muttdini@...il.com>
> > Signed-off-by: Darrick J. Wong <darrick.wong@...cle.com>
> > ---
> > fs/iomap.c |    8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/iomap.c b/fs/iomap.c
> > index 432eed8..16f5c074 100644
> > --- a/fs/iomap.c
> > +++ b/fs/iomap.c
> > @@ -610,8 +610,8 @@ iomap_seek_hole(struct inode *inode, loff_t offset, const struct iomap_ops *ops)
> > 	loff_t length = size - offset;
> > 	loff_t ret;
> > 
> > -	/* Nothing to be found beyond the end of the file. */
> > -	if (offset >= size)
> > +	/* Nothing to be found before or beyond the end of the file. */
> > +	if (offset < 0 || offset >= size)
> > 		return -ENXIO;
> > 
> > 	while (length > 0) {
> > @@ -656,8 +656,8 @@ iomap_seek_data(struct inode *inode, loff_t offset, const struct iomap_ops *ops)
> > 	loff_t length = size - offset;
> > 	loff_t ret;
> > 
> > -	/* Nothing to be found beyond the end of the file. */
> > -	if (offset >= size)
> > +	/* Nothing to be found before or beyond the end of the file. */
> > +	if (offset < 0 || offset >= size)
> > 		return -ENXIO;
> > 
> > 	while (length > 0) {
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
>