[SECOND RESEND] vfs: Return -ENXIO for negative SEEK_HOLE / SEEK_DATA offsets

[SECOND RESEND] vfs: Return -ENXIO for negative SEEK_HOLE / SEEK_DATA offsets

[Andreas Gruenbacher]

Linus,

could you please merge the following VFS fix, sent to Al etc. on August
30 and resent on September 14, with no reaction?

Thanks,
Andreas

--

In generic_file_llseek_size, return -ENXIO for negative offsets as well
as offsets beyond EOF.  This affects filesystems which don't implement
SEEK_HOLE / SEEK_DATA internally, possibly because they don't support
holes.

Fixes xfstest generic/448.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Cc: stable@vger.kernel.org
---
 fs/read_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index a2b9a47235c5..f0d4b16873e8 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -112,7 +112,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 		 * In the generic case the entire file is data, so as long as
 		 * offset isn't at the end of the file then the offset is data.
 		 */
-		if (offset >= eof)
+		if ((unsigned long long)offset >= eof)
 			return -ENXIO;
 		break;
 	case SEEK_HOLE:
@@ -120,7 +120,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 		 * There is a virtual hole at the end of the file, so as long as
 		 * offset isn't i_size or larger, return i_size.
 		 */
-		if (offset >= eof)
+		if ((unsigned long long)offset >= eof)
 			return -ENXIO;
 		offset = eof;
 		break;
-- 
2.13.5

Re: [SECOND RESEND] vfs: Return -ENXIO for negative SEEK_HOLE / SEEK_DATA offsets

[Linus Torvalds]

On Mon, Sep 25, 2017 at 3:23 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
>
> could you please merge the following VFS fix, sent to Al etc. on August
> 30 and resent on September 14, with no reaction?

This fix seems wrong, or at least misleading.

We already error out for negative offsets in vfs_setpos(), except for
the special case of /proc/<pid>/mem, /dev/mem and /dev/kmem (which
have that FMODE_UNSIGNED_OFFSET special case).

Sure, the error is different (-EINVAL), but that doesn't seem wrong.

So my gut feel is that if xfstest generic/448 cares about EINVAL vs
ENXIO, then that test is just garbage. Because let's face it, EINVAL
is the *normal* error return for negative offsets.

Am I missing something?

                 Linus

Re: [SECOND RESEND] vfs: Return -ENXIO for negative SEEK_HOLE / SEEK_DATA offsets

[Andreas Gruenbacher]

On Tue, Sep 26, 2017 at 7:12 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Sep 25, 2017 at 3:23 AM, Andreas Gruenbacher
> <agruenba@redhat.com> wrote:
>>
>> could you please merge the following VFS fix, sent to Al etc. on August
>> 30 and resent on September 14, with no reaction?
>
> This fix seems wrong, or at least misleading.
>
> We already error out for negative offsets in vfs_setpos(), except for
> the special case of /proc/<pid>/mem, /dev/mem and /dev/kmem (which
> have that FMODE_UNSIGNED_OFFSET special case).
>
> Sure, the error is different (-EINVAL), but that doesn't seem wrong.
>
> So my gut feel is that if xfstest generic/448 cares about EINVAL vs
> ENXIO, then that test is just garbage. Because let's face it, EINVAL
> is the *normal* error return for negative offsets.

Returning -EINVAL instead of -ENXIO for negative offsets sounds reasonable. But:

> Am I missing something?

When whence == SEEK_HOLE and offset < 0, generic_file_llseek_size will
return the
file size instead of -EINVAL. That's at least very weird, and should be fixed.

In addition, on ext4 and xfs, SEEK_HOLE / SEEK_DATA with a negative offset
will currently return -ENXIO. For consistency, do we want to change the ext4 and
xfs behavior to return -EINVAL? Do we want generic_file_llseek_size to behave
like ext4 and xfs?

Thanks,
Andreas

Re: [SECOND RESEND] vfs: Return -ENXIO for negative SEEK_HOLE / SEEK_DATA offsets

[Linus Torvalds]

On Tue, Sep 26, 2017 at 11:48 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
>
> When whence == SEEK_HOLE and offset < 0, generic_file_llseek_size will
> return the
> file size instead of -EINVAL. That's at least very weird, and should be fixed.

Ahh, yes. I looked at the first part of your patch (SEEK_DATA) and
went "that's nonsensical", because that one just uses offset, and will
then error out in vfs_setpos().

But you're right, SEEK_HOLE will then use "eof" for offset, and an
originally negative offset will just be ignored.

Ho humm. My gut feeling is that "maxsize" and "eof" should have been
unsigned types to begin with, since signed values don't make sense for
those. That would have taken care of the issue too.

But ok, I'll apply the patch.  Changing the signature of the function
looks like much too much effort for something like this.

                  Linus