# gcc x.c
# ./a.out
lseek on -100000: -100000
write: File too large
Should kernel allow negative offsets for lseek/llseek?
--
H.J. Lu ([email protected])
---
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>
extern loff_t llseek (int fd, loff_t offset, int whence);
int
main ()
{
int fd = open ("/tmp/foo.out", O_CREAT | O_RDWR, 0600);
off_t res, pos;
loff_t lres, lpos;
char buffer [] = "negative offset";
if (fd < 0)
{
perror ("open");
return 1;
}
pos = -100000;
res = lseek (fd, pos, SEEK_SET);
if (res == (off_t) -1L)
{
perror ("lseek");
close (fd);
return 1;
}
printf ("lseek on %ld: %ld\n", pos, res);
if (write (fd, buffer, sizeof (buffer)) != sizeof (buffer))
{
perror ("write");
close (fd);
return 1;
}
lpos = -100000LL;
lres = llseek (fd, lpos, SEEK_SET);
if (lres == (loff_t) -1L)
{
perror ("llseek");
close (fd);
return 1;
}
printf ("llseek on %lld: %lld\n", lpos, lres);
if (write (fd, buffer, sizeof (buffer)) != sizeof (buffer))
{
perror ("write");
close (fd);
return 1;
}
close (fd);
return 0;
}
On Fri, Nov 17, 2000 at 03:59:13PM -0800, H . J . Lu wrote:
> # gcc x.c
> # ./a.out
> lseek on -100000: -100000
> write: File too large
>
> Should kernel allow negative offsets for lseek/llseek?
>
>
Never mind. I was running the wrong kernel.
H.J.
On Fri, Nov 17, 2000 at 04:09:00PM -0800, H . J . Lu wrote:
> On Fri, Nov 17, 2000 at 03:59:13PM -0800, H . J . Lu wrote:
> > # gcc x.c
> > # ./a.out
> > lseek on -100000: -100000
> > write: File too large
> >
> > Should kernel allow negative offsets for lseek/llseek?
> >
> >
>
> Never mind. I was running the wrong kernel.
With 2.2.18pre21aa2 this little proggy:
main()
{
int fd = creat("x", 0600);
lseek(fd, 0x80000000, 0);
}
get confused this way:
lseek(3, 2147483648, SEEK_SET) = -1 ERRNO_0 (Success)
_exit(-2147483648) = ?
I fixed it this way:
diff -urN 2.2.18pre21/fs/read_write.c lseek/fs/read_write.c
--- 2.2.18pre21/fs/read_write.c Tue Sep 5 02:28:49 2000
+++ lseek/fs/read_write.c Sat Nov 18 18:42:55 2000
@@ -53,6 +53,10 @@
struct dentry * dentry;
struct inode * inode;
+ retval = -EINVAL;
+ if (offset < 0)
+ goto out_nolock;
+
lock_kernel();
retval = -EBADF;
file = fget(fd);
@@ -69,6 +73,7 @@
fput(file);
bad:
unlock_kernel();
+out_nolock:
return retval;
}
@@ -83,6 +88,11 @@
struct inode * inode;
loff_t offset;
+ retval = -EINVAL;
+ offset = ((loff_t) offset_high << 32) | offset_low;
+ if (offset < 0)
+ goto out_nolock;
+
lock_kernel();
retval = -EBADF;
file = fget(fd);
@@ -96,8 +106,7 @@
if (origin > 2)
goto out_putf;
- offset = llseek(file, ((loff_t) offset_high << 32) | offset_low,
- origin);
+ offset = llseek(file, offset, origin);
retval = (int)offset;
if (offset >= 0) {
@@ -109,6 +118,7 @@
fput(file);
bad:
unlock_kernel();
+out_nolock:
return retval;
}
#endif
I've not tried yet in practice but by reading sources 2.4.x has the same bug
since doing the check internally to ext2 is too late to handle the 32bit
(non-lfs) lseek interface. After moving the checks into the vfs (that seems
the right thing to do to me) the check internally to ext2 can be removed of
course (it was superflous anyways because ext2 file size is limited to 4T
that's not negative value in 64bit signed math)
Andrea
On Sat, Nov 18, 2000 at 07:25:42PM +0100, Andrea Arcangeli wrote:
> On Fri, Nov 17, 2000 at 04:09:00PM -0800, H . J . Lu wrote:
> > On Fri, Nov 17, 2000 at 03:59:13PM -0800, H . J . Lu wrote:
> > > # gcc x.c
> > > # ./a.out
> > > lseek on -100000: -100000
> > > write: File too large
> > >
> > > Should kernel allow negative offsets for lseek/llseek?
> > >
> > >
> >
> > Never mind. I was running the wrong kernel.
>
> With 2.2.18pre21aa2 this little proggy:
>
> main()
> {
> int fd = creat("x", 0600);
> lseek(fd, 0x80000000, 0);
> }
>
> get confused this way:
>
> lseek(3, 2147483648, SEEK_SET) = -1 ERRNO_0 (Success)
> _exit(-2147483648) = ?
>
> I fixed it this way:
>
> diff -urN 2.2.18pre21/fs/read_write.c lseek/fs/read_write.c
> --- 2.2.18pre21/fs/read_write.c Tue Sep 5 02:28:49 2000
> +++ lseek/fs/read_write.c Sat Nov 18 18:42:55 2000
> @@ -53,6 +53,10 @@
> struct dentry * dentry;
> struct inode * inode;
>
> + retval = -EINVAL;
> + if (offset < 0)
> + goto out_nolock;
> +
offset shouldn't be < 0 to begin with. There may be a bug somewhere
else. In my case,
if (offset < 0)
return -EINVAL;
is missing from those FS lseek functions.
H.J.
On Sat, Nov 18, 2000 at 07:25:42PM +0100, Andrea Arcangeli wrote:
> I fixed it this way:
fix is plain wrong, it's still possible to have lseek return -1 -2 -3 -4
even when it should return -EINVAL.
Andrea
On Sun, Nov 19, 2000 at 01:45:12AM +0100, Andrea Arcangeli wrote:
> On Sat, Nov 18, 2000 at 07:25:42PM +0100, Andrea Arcangeli wrote:
> > I fixed it this way:
>
> fix is plain wrong, it's still possible to have lseek return -1 -2 -3 -4
> even when it should return -EINVAL.
>
Try this again 2.2.18pre21. It works for me.
--
H.J. Lu ([email protected])
---
--- linux/fs/ext2/file.c.lseek Sat Nov 18 17:18:49 2000
+++ linux/fs/ext2/file.c Sat Nov 18 17:19:28 2000
@@ -120,6 +120,8 @@ static long long ext2_file_lseek(
case 1:
offset += file->f_pos;
}
+ if (offset < 0)
+ return -EINVAL;
if (((unsigned long long) offset >> 32) != 0) {
#if BITS_PER_LONG < 64
return -EINVAL;
--- linux/fs/proc/mem.c.lseek Tue Jan 4 10:12:23 2000
+++ linux/fs/proc/mem.c Sat Nov 18 17:19:28 2000
@@ -196,14 +196,17 @@ static long long mem_lseek(struct file *
{
switch (orig) {
case 0:
- file->f_pos = offset;
- return file->f_pos;
+ break;
case 1:
- file->f_pos += offset;
- return file->f_pos;
+ offset += file->f_pos;
+ break;
default:
return -EINVAL;
}
+ if (offset < 0)
+ return -EINVAL;
+ file->f_pos = offset;
+ return offset;
}
/*
On Sat, Nov 18, 2000 at 05:20:34PM -0800, H . J . Lu wrote:
> Try this again 2.2.18pre21. It works for me.
>
>
> --
> H.J. Lu ([email protected])
> ---
> --- linux/fs/ext2/file.c.lseek Sat Nov 18 17:18:49 2000
> +++ linux/fs/ext2/file.c Sat Nov 18 17:19:28 2000
> @@ -120,6 +120,8 @@ static long long ext2_file_lseek(
> case 1:
> offset += file->f_pos;
> }
> + if (offset < 0)
> + return -EINVAL;
> if (((unsigned long long) offset >> 32) != 0) {
> #if BITS_PER_LONG < 64
> return -EINVAL;
It's not enough for 2.2.x (and you left the `>> 32' nosense check).
2.2.x vanilla (so non-lfs) is getting wrong both 32bit and 64bit:
1) 32bit can lseek over 2G and it can return bogus retval
main()
{
int fd = creat("x", 0600);
lseek(fd, 0x7fffffff, 1);
lseek(fd, 0x7fffffff, 1);
}
lseek(3, 2147483647, SEEK_CUR) = 2147483647
lseek(3, 2147483647, SEEK_CUR) = -1 ENOENT (No such file or directory)
(this isn't really -ENOENT but it's just trying to return a succesful -2 value)
2) 64bit can lseek over ext2_max_sizes and it can return bogus retvals
main()
{
int fd = creat("x", 0600);
lseek(fd, -2, 0);
}
open("x", O_WRONLY|O_CREAT|O_TRUNC, 0600) = 3
lseek(3, 18446744073709551614, SEEK_SET) = -1 ENOENT (No such file or directory)
(this isn't really -ENOENT but it's just trying to return a succesful -2 value)
This will cure 2.2.x vanilla:
--- 2.2.18pre21/fs/ext2/file.c.~1~ Sun Nov 12 00:45:42 2000
+++ 2.2.18pre21/fs/ext2/file.c Sun Nov 19 03:59:29 2000
@@ -120,14 +120,14 @@
case 1:
offset += file->f_pos;
}
- if (((unsigned long long) offset >> 32) != 0) {
#if BITS_PER_LONG < 64
+ if (offset >> 31)
return -EINVAL;
#else
- if (offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
- return -EINVAL;
+ if (offset < 0 ||
+ offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
+ return -EINVAL;
#endif
- }
if (offset != file->f_pos) {
file->f_pos = offset;
file->f_reada = 0;
and this will remove the nosense stuff from 2.4.x:
--- 2.4.0-test11-pre6/fs/ext2/file.c.~1~ Thu Nov 16 15:37:32 2000
+++ 2.4.0-test11-pre6/fs/ext2/file.c Sun Nov 19 04:03:27 2000
@@ -53,12 +53,9 @@
case 1:
offset += file->f_pos;
}
- if (offset<0)
+ if (offset < 0 ||
+ offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
return -EINVAL;
- if (((unsigned long long) offset >> 32) != 0) {
- if (offset > ext2_max_sizes[EXT2_BLOCK_SIZE_BITS(inode->i_sb)])
- return -EINVAL;
- }
if (offset != file->f_pos) {
file->f_pos = offset;
file->f_reada = 0;
(the above is also ok for 2.2.x-lfs and I'll fix it that way)
Andrea
On Sun, Nov 19, 2000 at 04:07:04AM +0100, Andrea Arcangeli wrote:
> On Sat, Nov 18, 2000 at 05:20:34PM -0800, H . J . Lu wrote:
> > Try this again 2.2.18pre21. It works for me.
> >
> >
> > --
> > H.J. Lu ([email protected])
> > ---
> > --- linux/fs/ext2/file.c.lseek Sat Nov 18 17:18:49 2000
> > +++ linux/fs/ext2/file.c Sat Nov 18 17:19:28 2000
> > @@ -120,6 +120,8 @@ static long long ext2_file_lseek(
> > case 1:
> > offset += file->f_pos;
> > }
> > + if (offset < 0)
> > + return -EINVAL;
> > if (((unsigned long long) offset >> 32) != 0) {
> > #if BITS_PER_LONG < 64
> > return -EINVAL;
>
> It's not enough for 2.2.x (and you left the `>> 32' nosense check).
>
It works for me on ia32. offset is a long long in 2.2.18pre21 on ia32.
I don't see anything wrong for
if (offset < 0)
return -EINVAL;
I got:
lseek(3, 4294967295, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 4294967294, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 4294967293, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 4294967292, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 4294967291, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 2147483647, SEEK_CUR) = -1 EOVERFLOW (Value too large for defined data type)
lseek(3, 2147483647, SEEK_CUR) = -1 EOVERFLOW (Value too large for defined data type)
_llseek(3, 18446744073709551614, 0xbffff980, SEEK_SET) = -1 EINVAL (Invalid argument)
lseek(3, 4294867296, SEEK_SET) = -1 EINVAL (Invalid argument)
My kernel has LFS patch.
H.J.
On Sat, Nov 18, 2000 at 05:20:34PM -0800, H . J . Lu wrote:
> --- linux/fs/proc/mem.c.lseek Tue Jan 4 10:12:23 2000
> +++ linux/fs/proc/mem.c Sat Nov 18 17:19:28 2000
> @@ -196,14 +196,17 @@ static long long mem_lseek(struct file *
> {
> switch (orig) {
> case 0:
> - file->f_pos = offset;
> - return file->f_pos;
> + break;
> case 1:
> - file->f_pos += offset;
> - return file->f_pos;
> + offset += file->f_pos;
> + break;
> default:
> return -EINVAL;
> }
> + if (offset < 0)
> + return -EINVAL;
> + file->f_pos = offset;
> + return offset;
> }
This looks fine to me.
Andrea