| Message ID | 20170925102303.8288-1-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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;
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(-)