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

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

[Darrick J. Wong]

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.

Inspired-by: Mateusz S <muttdini@gmail.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.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) {

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

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

On Jul 12, 2017, at 10:36 AM, Darrick J. Wong <darrick.wong@oracle.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?
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.  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@gmail.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Darrick J. Wong]

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@oracle.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@gmail.com>
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.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
> 
> 
> 
> 
>