[PATCH] tmpfs: let lseek return ENXIO with a negative offset

[PATCH] tmpfs: let lseek return ENXIO with a negative offset

[Yufen Yu]

For now, the others filesystems, such as ext4, f2fs, ubifs,
all of them return ENXIO when lseek with a negative offset.
It is better to let tmpfs return ENXIO too. After that, tmpfs
can also pass generic/448.

Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 mm/shmem.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 0376c124..f37bf06 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2608,9 +2608,7 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
 	inode_lock(inode);
 	/* We're holding i_mutex so we can access i_size directly */
 
-	if (offset < 0)
-		offset = -EINVAL;
-	else if (offset >= inode->i_size)
+	if (offset < 0 || offset >= inode->i_size)
 		offset = -ENXIO;
 	else {
 		start = offset >> PAGE_SHIFT;
-- 
2.7.4

Re: [PATCH] tmpfs: let lseek return ENXIO with a negative offset

[William Kucharski]

> On Oct 24, 2018, at 8:22 PM, Yufen Yu <yuyufen@huawei.com> wrote:
> 
> For now, the others filesystems, such as ext4, f2fs, ubifs,
> all of them return ENXIO when lseek with a negative offset.
> It is better to let tmpfs return ENXIO too. After that, tmpfs
> can also pass generic/448.
> 
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> ---
> mm/shmem.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0376c124..f37bf06 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2608,9 +2608,7 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> 	inode_lock(inode);
> 	/* We're holding i_mutex so we can access i_size directly */
> 
> -	if (offset < 0)
> -		offset = -EINVAL;
> -	else if (offset >= inode->i_size)
> +	if (offset < 0 || offset >= inode->i_size)
> 		offset = -ENXIO;
> 	else {
> 		start = offset >> PAGE_SHIFT;
> --

It's not at all clear what the proper thing to do is if a negative offset is passed.

The man page for lseek(2) states:

       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).

This seems to indicate that if passed a negative offset, a whence of either SEEK_DATA
or SEEK_HOLE should operate the same as if passed an offset of 0.

ENXIO just seems to be the wrong error code to return for a passed negative offset in
these cases (also from lseek(2)):

       ENXIO  whence is SEEK_DATA or SEEK_HOLE, and the file offset is
              beyond the end of the file.

but EINVAL isn't technically appropriate either:

       EINVAL whence is not valid.  Or: the resulting file offset would be
              negative, or beyond the end of a seekable device.

At the very least it seems the man page should be updated to reflect that ENXIO may be
returned if whence is SEEK_DATA or SEEK_HOLE and the passed offset is negative.

    William Kucharski

Re: [PATCH] tmpfs: let lseek return ENXIO with a negative offset

[Andrew Morton]

On Thu, 25 Oct 2018 10:22:56 +0800 Yufen Yu <yuyufen@huawei.com> wrote:

> For now, the others filesystems, such as ext4, f2fs, ubifs,
> all of them return ENXIO when lseek with a negative offset.

When using SEEK_DATA and/or SEEK_HOLE, yes?

> It is better to let tmpfs return ENXIO too. After that, tmpfs
> can also pass generic/448.

generic/448 is, I assume, part of xfstests?

So I'll rewrite the changelog as follows.  Please review carefully.



Subject: tmpfs: make lseek(SEEK_DATA/SEK_HOLE) return ENXIO with a negative offset

Other filesystems such as ext4, f2fs and ubifs all return ENXIO when
lseek (SEEK_DATA or SEEK_HOLE) requests a negative offset.

man 2 lseek says

:      EINVAL whence  is  not  valid.   Or: the resulting file offset would be
:             negative, or beyond the end of a seekable device.
:
:      ENXIO  whence is SEEK_DATA or SEEK_HOLE, and the file offset is  beyond
:             the end of the file.


Make tmpfs return ENXIO under these circumstances as well.  After this,
tmpfs also passes xfstests's generic/448.

Re: [PATCH] tmpfs: let lseek return ENXIO with a negative offset

[William Kucharski]

> On Nov 7, 2018, at 4:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> man 2 lseek says
> 
> :      EINVAL whence  is  not  valid.   Or: the resulting file offset would be
> :             negative, or beyond the end of a seekable device.
> :
> :      ENXIO  whence is SEEK_DATA or SEEK_HOLE, and the file offset is  beyond
> :             the end of the file.
> 
> 
> Make tmpfs return ENXIO under these circumstances as well.  After this,
> tmpfs also passes xfstests's generic/448.

As I objected to last week, despite the fact that other file systems do this, is
this in fact the desired behavior?

I'll let you reread that message rather than repeat it in its entirety here, but
certainly a negative offset is not "beyond the end of the file," and the end
result is errno is set to ENXIO for a reason that does not match what the
lseek(2) man page describes.

I also mentioned if a negative offset is used with SEEK_CUR or SEEK_WHENCE,
arguably the negative offset should actually be treated as "0" given lseek(2)
also states:

      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).

Since the "next location" or "next hole" will never be at a negative offset, the
"greater than" clause of both descriptions would mean the resulting offset should
be treated as if it were passed as zero.

However, if xfstest-compliant behavior is desired, the lseek(2) man page
description for ENXIO should be updated to something like:

       ENXIO  whence is SEEK_DATA or SEEK_HOLE, and the file offset is negative or
              beyond the end of the file.

I don't mean to be pedantic, but I also know how frustrating it can be when a system
call returns with errno set for a reason that doesn't correspond to the man page.

    Thanks,
         William Kucharski

Re: [PATCH] tmpfs: let lseek return ENXIO with a negative offset

[Andrew Morton]

On Thu, 8 Nov 2018 03:46:35 -0700 William Kucharski <william.kucharski@oracle.com> wrote:

> 
> 
> > On Nov 7, 2018, at 4:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > man 2 lseek says
> > 
> > :      EINVAL whence  is  not  valid.   Or: the resulting file offset would be
> > :             negative, or beyond the end of a seekable device.
> > :
> > :      ENXIO  whence is SEEK_DATA or SEEK_HOLE, and the file offset is  beyond
> > :             the end of the file.
> > 
> > 
> > Make tmpfs return ENXIO under these circumstances as well.  After this,
> > tmpfs also passes xfstests's generic/448.
> 
> As I objected to last week, despite the fact that other file systems do this, is
> this in fact the desired behavior?
> 
> I'll let you reread that message rather than repeat it in its entirety here, but
> certainly a negative offset is not "beyond the end of the file," and the end
> result is errno is set to ENXIO for a reason that does not match what the
> lseek(2) man page describes.
> 
> I also mentioned if a negative offset is used with SEEK_CUR or SEEK_WHENCE,
> arguably the negative offset should actually be treated as "0" given lseek(2)
> also states:
> 
>       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).
> 
> Since the "next location" or "next hole" will never be at a negative offset, the
> "greater than" clause of both descriptions would mean the resulting offset should
> be treated as if it were passed as zero.
> 
> However, if xfstest-compliant behavior is desired, the lseek(2) man page
> description for ENXIO should be updated to something like:
> 
>        ENXIO  whence is SEEK_DATA or SEEK_HOLE, and the file offset is negative or
>               beyond the end of the file.
> 
> I don't mean to be pedantic, but I also know how frustrating it can be when a system
> call returns with errno set for a reason that doesn't correspond to the man page.

I think that at this stage we should make tmpfs behaviour match the
other filesystems.

If the manpage doesn't match the kernel's behaviour for this
linux-specific feature(?) then we should fix the manpage.

If we find that the behaviour should actually change (and there's a way
of doing that in a reasonably back-compatible manner) then let's change
all filesystems and the manpage.

OK?

Re: [PATCH] tmpfs: let lseek return ENXIO with a negative offset

[William Kucharski]

> On Nov 8, 2018, at 4:07 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> I think that at this stage we should make tmpfs behaviour match the
> other filesystems.
> 
> If the manpage doesn't match the kernel's behaviour for this
> linux-specific feature(?) then we should fix the manpage.
> 
> If we find that the behaviour should actually change (and there's a way
> of doing that in a reasonably back-compatible manner) then let's change
> all filesystems and the manpage.
> 
> OK?

I did a little research, and according to lseek(2):

       SEEK_DATA and SEEK_HOLE are nonstandard extensions also present in
       Solaris, FreeBSD, and DragonFly BSD; they are proposed for inclusion
       in the next POSIX revision (Issue 8).
 
I wrote a brief test program that operated on a file of 1024 zeroes and found
that on an ext4 file system, lseek(2) matches Solaris' behavior on ZFS:

Solaris/ZFS
===========
lseek(3, -1, SEEK_HOLE) error, errno 6 (ENXIO)
lseek(3, 0, SEEK_HOLE) returned 1024
lseek(3, 1, SEEK_HOLE) returned 1024

lseek(3, -1, SEEK_DATA) error, errno 6 (ENXIO)
lseek(3, 0, SEEK_DATA) returned 0
lseek(3, 1, SEEK_DATA) returned 1

Linux/ext4
==========
lseek(3, -1, SEEK_HOLE) error, errno 6 (ENXIO)
lseek(3, 0, SEEK_HOLE) returned 1024
lseek(3, 1, SEEK_HOLE) returned 1024

lseek(3, -1, SEEK_DATA) error, errno 6 (ENXIO)
lseek(3, 0, SEEK_DATA) returned 0
lseek(3, 1, SEEK_DATA) returned 1

That validates the xfstest expectations that a negative passed offset should
return ENXIO.

I suggest the man page wording be changed to read:

       ENXIO  whence is SEEK_DATA or SEEK_HOLE, and the file offset is negative or
              beyond the end of the file.

unless you'd prefer an alternative statement.

Thanks!!