2019-08-01 07:28:23

by Sergei Turchanov

[permalink] [raw]
Subject: [BUG] lseek on /proc/meminfo is broken in 4.19.59

Hello!

(I sent this e-mail two weeks ago with no feedback. Does anyone care?
Wrong mailing list? Anything....?)

Seeking (to an offset within file size) in /proc/meminfo is broken in
4.19.59. It does seek to a desired position, but reading from that
position returns the remainder of file and then a whole copy of file.
This doesn't happen with /proc/vmstat or /proc/self/maps for example.

Seeking did work correctly in kernel 4.14.47. So it seems something
broke in the way.

Background: this kind of access pattern (seeking to /proc/meminfo) is
used by libvirt-lxc fuse driver for virtualized view of /proc/meminfo.
So that /proc/meminfo is broken in guests when running kernel 4.19.x.

$ ./test /proc/meminfo 0        # Works as expected

MemTotal:       394907728 kB
MemFree:        173738328 kB
...
DirectMap2M:    13062144 kB
DirectMap1G:    390070272 kB

-----------------------------------------------------------------------

$ ./test 1024                   # returns a copy of file after the remainder

Will seek to 1024


Data read at offset 1024
gePages:         0 kB
ShmemHugePages:        0 kB
ShmemPmdMapped:        0 kB
HugePages_Total:       0
HugePages_Free:        0
HugePages_Rsvd:        0
HugePages_Surp:        0
Hugepagesize:       2048 kB
Hugetlb:               0 kB
DirectMap4k:      245204 kB
DirectMap2M:    13062144 kB
DirectMap1G:    390070272 kB
MemTotal:       394907728 kB
MemFree:        173738328 kB
MemAvailable:   379989680 kB
Buffers:          355812 kB
Cached:         207216224 kB
...
DirectMap2M:    13062144 kB
DirectMap1G:    390070272 kB

As you see, after "DirectMap1G:" line, a whole copy of /proc/meminfo
returned by "read".

Test program:

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

#define SIZE 1024
char buf[SIZE + 1];

int main(int argc, char *argv[]) {
    int     fd;
    ssize_t rd;
    off_t   ofs = 0;

    if (argc < 2) {
        printf("Usage: test <file> [<offset>]\n");
        exit(1);
    }

    if (-1 == (fd = open(argv[1], O_RDONLY))) {
        perror("open failed");
        exit(1);
    }

    if (argc > 2) {
        ofs = atol(argv[2]);
    }
    printf("Will seek to %ld\n", ofs);

    if (-1 == (lseek(fd, ofs, SEEK_SET))) {
        perror("lseek failed");
        exit(1);
    }

    for (;; ofs += rd) {
        printf("\n\nData read at offset %ld\n", ofs);
        if (-1 == (rd = read(fd, buf, SIZE))) {
            perror("read failed");
            exit(1);
        }
        buf[rd] = '\0';
        printf(buf);
        if (rd < SIZE) {
            break;
        }
    }

    return 0;
}




2019-08-01 07:39:27

by Gao Xiang

[permalink] [raw]
Subject: Re: [BUG] lseek on /proc/meminfo is broken in 4.19.59

Hi,

I just took a glance, maybe due to
commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")

I simply reverted it just now and it seems fine... but I haven't digged into this commit.

Maybe you could Cc NeilBrown <[email protected]> for some more advice and
I have no idea whether it's an expected behavior or not...

Thanks,
Gao Xiang

On 2019/8/1 14:16, Sergei Turchanov wrote:
> Hello!
>
> (I sent this e-mail two weeks ago with no feedback. Does anyone care? Wrong mailing list? Anything....?)
>
> Seeking (to an offset within file size) in /proc/meminfo is broken in 4.19.59. It does seek to a desired position, but reading from that position returns the remainder of file and then a whole copy of file. This doesn't happen with /proc/vmstat or /proc/self/maps for example.
>
> Seeking did work correctly in kernel 4.14.47. So it seems something broke in the way.
>
> Background: this kind of access pattern (seeking to /proc/meminfo) is used by libvirt-lxc fuse driver for virtualized view of /proc/meminfo. So that /proc/meminfo is broken in guests when running kernel 4.19.x.
>
> $ ./test /proc/meminfo 0        # Works as expected
>
> MemTotal:       394907728 kB
> MemFree:        173738328 kB
> ...
> DirectMap2M:    13062144 kB
> DirectMap1G:    390070272 kB
>
> -----------------------------------------------------------------------
>
> $ ./test 1024                   # returns a copy of file after the remainder
>
> Will seek to 1024
>
>
> Data read at offset 1024
> gePages:         0 kB
> ShmemHugePages:        0 kB
> ShmemPmdMapped:        0 kB
> HugePages_Total:       0
> HugePages_Free:        0
> HugePages_Rsvd:        0
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
> Hugetlb:               0 kB
> DirectMap4k:      245204 kB
> DirectMap2M:    13062144 kB
> DirectMap1G:    390070272 kB
> MemTotal:       394907728 kB
> MemFree:        173738328 kB
> MemAvailable:   379989680 kB
> Buffers:          355812 kB
> Cached:         207216224 kB
> ...
> DirectMap2M:    13062144 kB
> DirectMap1G:    390070272 kB
>
> As you see, after "DirectMap1G:" line, a whole copy of /proc/meminfo returned by "read".
>
> Test program:
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> #define SIZE 1024
> char buf[SIZE + 1];
>
> int main(int argc, char *argv[]) {
>     int     fd;
>     ssize_t rd;
>     off_t   ofs = 0;
>
>     if (argc < 2) {
>         printf("Usage: test <file> [<offset>]\n");
>         exit(1);
>     }
>
>     if (-1 == (fd = open(argv[1], O_RDONLY))) {
>         perror("open failed");
>         exit(1);
>     }
>
>     if (argc > 2) {
>         ofs = atol(argv[2]);
>     }
>     printf("Will seek to %ld\n", ofs);
>
>     if (-1 == (lseek(fd, ofs, SEEK_SET))) {
>         perror("lseek failed");
>         exit(1);
>     }
>
>     for (;; ofs += rd) {
>         printf("\n\nData read at offset %ld\n", ofs);
>         if (-1 == (rd = read(fd, buf, SIZE))) {
>             perror("read failed");
>             exit(1);
>         }
>         buf[rd] = '\0';
>         printf(buf);
>         if (rd < SIZE) {
>             break;
>         }
>     }
>
>     return 0;
> }
>
>
>

2019-08-01 07:55:55

by Sergei Turchanov

[permalink] [raw]
Subject: Re: [BUG] lseek on /proc/meminfo is broken in 4.19.59

Hi,

Thank you very much for your suggestion. Will certainly do that.

With best regards,
Sergei.

On 01.08.2019 17:11, Gao Xiang wrote:
> Hi,
>
> I just took a glance, maybe due to
> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
>
> I simply reverted it just now and it seems fine... but I haven't digged into this commit.
>
> Maybe you could Cc NeilBrown <[email protected]> for some more advice and
> I have no idea whether it's an expected behavior or not...
>
> Thanks,
> Gao Xiang
>
> On 2019/8/1 14:16, Sergei Turchanov wrote:
>> Hello!
>>
>> (I sent this e-mail two weeks ago with no feedback. Does anyone care? Wrong mailing list? Anything....?)
>>
>> Seeking (to an offset within file size) in /proc/meminfo is broken in 4.19.59. It does seek to a desired position, but reading from that position returns the remainder of file and then a whole copy of file. This doesn't happen with /proc/vmstat or /proc/self/maps for example.
>>
>> Seeking did work correctly in kernel 4.14.47. So it seems something broke in the way.
>>
>> Background: this kind of access pattern (seeking to /proc/meminfo) is used by libvirt-lxc fuse driver for virtualized view of /proc/meminfo. So that /proc/meminfo is broken in guests when running kernel 4.19.x.
>>
>> $ ./test /proc/meminfo 0        # Works as expected
>>
>> MemTotal:       394907728 kB
>> MemFree:        173738328 kB
>> ...
>> DirectMap2M:    13062144 kB
>> DirectMap1G:    390070272 kB
>>
>> -----------------------------------------------------------------------
>>
>> $ ./test 1024                   # returns a copy of file after the remainder
>>
>> Will seek to 1024
>>
>>
>> Data read at offset 1024
>> gePages:         0 kB
>> ShmemHugePages:        0 kB
>> ShmemPmdMapped:        0 kB
>> HugePages_Total:       0
>> HugePages_Free:        0
>> HugePages_Rsvd:        0
>> HugePages_Surp:        0
>> Hugepagesize:       2048 kB
>> Hugetlb:               0 kB
>> DirectMap4k:      245204 kB
>> DirectMap2M:    13062144 kB
>> DirectMap1G:    390070272 kB
>> MemTotal:       394907728 kB
>> MemFree:        173738328 kB
>> MemAvailable:   379989680 kB
>> Buffers:          355812 kB
>> Cached:         207216224 kB
>> ...
>> DirectMap2M:    13062144 kB
>> DirectMap1G:    390070272 kB
>>
>> As you see, after "DirectMap1G:" line, a whole copy of /proc/meminfo returned by "read".
>>
>> Test program:
>>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>>
>> #define SIZE 1024
>> char buf[SIZE + 1];
>>
>> int main(int argc, char *argv[]) {
>>     int     fd;
>>     ssize_t rd;
>>     off_t   ofs = 0;
>>
>>     if (argc < 2) {
>>         printf("Usage: test <file> [<offset>]\n");
>>         exit(1);
>>     }
>>
>>     if (-1 == (fd = open(argv[1], O_RDONLY))) {
>>         perror("open failed");
>>         exit(1);
>>     }
>>
>>     if (argc > 2) {
>>         ofs = atol(argv[2]);
>>     }
>>     printf("Will seek to %ld\n", ofs);
>>
>>     if (-1 == (lseek(fd, ofs, SEEK_SET))) {
>>         perror("lseek failed");
>>         exit(1);
>>     }
>>
>>     for (;; ofs += rd) {
>>         printf("\n\nData read at offset %ld\n", ofs);
>>         if (-1 == (rd = read(fd, buf, SIZE))) {
>>             perror("read failed");
>>             exit(1);
>>         }
>>         buf[rd] = '\0';
>>         printf(buf);
>>         if (rd < SIZE) {
>>             break;
>>         }
>>     }
>>
>>     return 0;
>> }
>>
>>
>>

2019-08-01 09:34:26

by Sergei Turchanov

[permalink] [raw]
Subject: [BUG] lseek on /proc/meminfo is broken in 4.19.59 maybe due to commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")

Hello!

[
As suggested in previous discussion this behavior may be caused by your
commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
]

Original bug report:

Seeking (to an offset within file size) in /proc/meminfo is broken in 4.19.59. It does seek to a desired position, but reading from that position returns the remainder of file and then a whole copy of file. This doesn't happen with /proc/vmstat or /proc/self/maps for example.

Seeking did work correctly in kernel 4.14.47. So it seems something broke in the way.

Background: this kind of access pattern (seeking to /proc/meminfo) is used by libvirt-lxc fuse driver for virtualized view of /proc/meminfo. So that /proc/meminfo is broken in guests when running kernel 4.19.x.

> On 01.08.2019 17:11, Gao Xiang wrote:
> Hi,
>
> I just took a glance, maybe due to
> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
>
> I simply reverted it just now and it seems fine... but I haven't digged into this commit.
>
> Maybe you could Cc NeilBrown <[email protected]> for some more advice and
> I have no idea whether it's an expected behavior or not...
>
> Thanks,
> Gao Xiang
>
> On 2019/8/1 14:16, Sergei Turchanov wrote:


$ ./test /proc/meminfo 0        # Works as expected

MemTotal:       394907728 kB
MemFree:        173738328 kB
...
DirectMap2M:    13062144 kB
DirectMap1G:    390070272 kB

-----------------------------------------------------------------------

$ ./test /proc/meminfo 1024     # returns a copy of file after the remainder

Will seek to 1024


Data read at offset 1024
gePages:         0 kB
ShmemHugePages:        0 kB
ShmemPmdMapped:        0 kB
HugePages_Total:       0
HugePages_Free:        0
HugePages_Rsvd:        0
HugePages_Surp:        0
Hugepagesize:       2048 kB
Hugetlb:               0 kB
DirectMap4k:      245204 kB
DirectMap2M:    13062144 kB
DirectMap1G:    390070272 kB
MemTotal:       394907728 kB
MemFree:        173738328 kB
MemAvailable:   379989680 kB
Buffers:          355812 kB
Cached:         207216224 kB
...
DirectMap2M:    13062144 kB
DirectMap1G:    390070272 kB

As you see, after "DirectMap1G:" line, a whole copy of /proc/meminfo returned by "read".

Test program:

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

#define SIZE 1024
char buf[SIZE + 1];

int main(int argc, char *argv[]) {
    int     fd;
    ssize_t rd;
    off_t   ofs = 0;

    if (argc < 2) {
        printf("Usage: test <file> [<offset>]\n");
        exit(1);
    }

    if (-1 == (fd = open(argv[1], O_RDONLY))) {
        perror("open failed");
        exit(1);
    }

    if (argc > 2) {
        ofs = atol(argv[2]);
    }
    printf("Will seek to %ld\n", ofs);

    if (-1 == (lseek(fd, ofs, SEEK_SET))) {
        perror("lseek failed");
        exit(1);
    }

    for (;; ofs += rd) {
        printf("\n\nData read at offset %ld\n", ofs);
        if (-1 == (rd = read(fd, buf, SIZE))) {
            perror("read failed");
            exit(1);
        }
        buf[rd] = '\0';
        printf(buf);
        if (rd < SIZE) {
            break;
        }
    }

    return 0;
}


2019-08-01 09:55:13

by NeilBrown

[permalink] [raw]
Subject: Re: [BUG] lseek on /proc/meminfo is broken in 4.19.59 maybe due to commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")

On Thu, Aug 01 2019, Sergei Turchanov wrote:

> Hello!
>
> [
> As suggested in previous discussion this behavior may be caused by your
> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
> ]

Yes.... I think I can see what happened.
removing:
- if (!m->count) {
- m->from = 0;
- m->index++;
- }

from seq_read meant that ->index didn't get updated in a case that it
needs to be.

Please confirm that the following patch fixes the problem.
I think it is correct, but I need to look it over more carefully in the
morning, and see if I can explain why it is correct.

Thanks for the report.
NeilBrown

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 04f09689cd6d..1600034a929b 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -119,6 +119,7 @@ static int traverse(struct seq_file *m, loff_t offset)
}
if (seq_has_overflowed(m))
goto Eoverflow;
+ p = m->op->next(m, p, &m->index);
if (pos + m->count > offset) {
m->from = offset - pos;
m->count -= m->from;
@@ -126,7 +127,6 @@ static int traverse(struct seq_file *m, loff_t offset)
}
pos += m->count;
m->count = 0;
- p = m->op->next(m, p, &m->index);
if (pos == offset)
break;
}


>
> Original bug report:
>
> Seeking (to an offset within file size) in /proc/meminfo is broken in 4.19.59. It does seek to a desired position, but reading from that position returns the remainder of file and then a whole copy of file. This doesn't happen with /proc/vmstat or /proc/self/maps for example.
>
> Seeking did work correctly in kernel 4.14.47. So it seems something broke in the way.
>
> Background: this kind of access pattern (seeking to /proc/meminfo) is used by libvirt-lxc fuse driver for virtualized view of /proc/meminfo. So that /proc/meminfo is broken in guests when running kernel 4.19.x.
>
> > On 01.08.2019 17:11, Gao Xiang wrote:
>> Hi,
>>
>> I just took a glance, maybe due to
>> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
>>
>> I simply reverted it just now and it seems fine... but I haven't digged into this commit.
>>
>> Maybe you could Cc NeilBrown <[email protected]> for some more advice and
>> I have no idea whether it's an expected behavior or not...
>>
>> Thanks,
>> Gao Xiang
>>
>> On 2019/8/1 14:16, Sergei Turchanov wrote:
>
>
> $ ./test /proc/meminfo 0        # Works as expected
>
> MemTotal:       394907728 kB
> MemFree:        173738328 kB
> ...
> DirectMap2M:    13062144 kB
> DirectMap1G:    390070272 kB
>
> -----------------------------------------------------------------------
>
> $ ./test /proc/meminfo 1024     # returns a copy of file after the remainder
>
> Will seek to 1024
>
>
> Data read at offset 1024
> gePages:         0 kB
> ShmemHugePages:        0 kB
> ShmemPmdMapped:        0 kB
> HugePages_Total:       0
> HugePages_Free:        0
> HugePages_Rsvd:        0
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
> Hugetlb:               0 kB
> DirectMap4k:      245204 kB
> DirectMap2M:    13062144 kB
> DirectMap1G:    390070272 kB
> MemTotal:       394907728 kB
> MemFree:        173738328 kB
> MemAvailable:   379989680 kB
> Buffers:          355812 kB
> Cached:         207216224 kB
> ...
> DirectMap2M:    13062144 kB
> DirectMap1G:    390070272 kB
>
> As you see, after "DirectMap1G:" line, a whole copy of /proc/meminfo returned by "read".
>
> Test program:
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> #define SIZE 1024
> char buf[SIZE + 1];
>
> int main(int argc, char *argv[]) {
>     int     fd;
>     ssize_t rd;
>     off_t   ofs = 0;
>
>     if (argc < 2) {
>         printf("Usage: test <file> [<offset>]\n");
>         exit(1);
>     }
>
>     if (-1 == (fd = open(argv[1], O_RDONLY))) {
>         perror("open failed");
>         exit(1);
>     }
>
>     if (argc > 2) {
>         ofs = atol(argv[2]);
>     }
>     printf("Will seek to %ld\n", ofs);
>
>     if (-1 == (lseek(fd, ofs, SEEK_SET))) {
>         perror("lseek failed");
>         exit(1);
>     }
>
>     for (;; ofs += rd) {
>         printf("\n\nData read at offset %ld\n", ofs);
>         if (-1 == (rd = read(fd, buf, SIZE))) {
>             perror("read failed");
>             exit(1);
>         }
>         buf[rd] = '\0';
>         printf(buf);
>         if (rd < SIZE) {
>             break;
>         }
>     }
>
>     return 0;
> }


Attachments:
signature.asc (847.00 B)

2019-08-02 02:31:31

by Sergei Turchanov

[permalink] [raw]
Subject: Re: [BUG] lseek on /proc/meminfo is broken in 4.19.59 maybe due to commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")

Hello!

Yes, your patch fixed this bug.
Thank you very much!

With best regards,
Sergei.

On 01.08.2019 19:14, NeilBrown wrote:
> On Thu, Aug 01 2019, Sergei Turchanov wrote:
>
>> Hello!
>>
>> [
>> As suggested in previous discussion this behavior may be caused by your
>> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
>> ]
> Yes.... I think I can see what happened.
> removing:
> - if (!m->count) {
> - m->from = 0;
> - m->index++;
> - }
>
> from seq_read meant that ->index didn't get updated in a case that it
> needs to be.
>
> Please confirm that the following patch fixes the problem.
> I think it is correct, but I need to look it over more carefully in the
> morning, and see if I can explain why it is correct.
>
> Thanks for the report.
> NeilBrown
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 04f09689cd6d..1600034a929b 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -119,6 +119,7 @@ static int traverse(struct seq_file *m, loff_t offset)
> }
> if (seq_has_overflowed(m))
> goto Eoverflow;
> + p = m->op->next(m, p, &m->index);
> if (pos + m->count > offset) {
> m->from = offset - pos;
> m->count -= m->from;
> @@ -126,7 +127,6 @@ static int traverse(struct seq_file *m, loff_t offset)
> }
> pos += m->count;
> m->count = 0;
> - p = m->op->next(m, p, &m->index);
> if (pos == offset)
> break;
> }
>
>
>> Original bug report:
>>
>> Seeking (to an offset within file size) in /proc/meminfo is broken in 4.19.59. It does seek to a desired position, but reading from that position returns the remainder of file and then a whole copy of file. This doesn't happen with /proc/vmstat or /proc/self/maps for example.
>>
>> Seeking did work correctly in kernel 4.14.47. So it seems something broke in the way.
>>
>> Background: this kind of access pattern (seeking to /proc/meminfo) is used by libvirt-lxc fuse driver for virtualized view of /proc/meminfo. So that /proc/meminfo is broken in guests when running kernel 4.19.x.
>>
>> > On 01.08.2019 17:11, Gao Xiang wrote:
>>> Hi,
>>>
>>> I just took a glance, maybe due to
>>> commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
>>>
>>> I simply reverted it just now and it seems fine... but I haven't digged into this commit.
>>>
>>> Maybe you could Cc NeilBrown <[email protected]> for some more advice and
>>> I have no idea whether it's an expected behavior or not...
>>>
>>> Thanks,
>>> Gao Xiang
>>>
>>> On 2019/8/1 14:16, Sergei Turchanov wrote:
>>
>> $ ./test /proc/meminfo 0        # Works as expected
>>
>> MemTotal:       394907728 kB
>> MemFree:        173738328 kB
>> ...
>> DirectMap2M:    13062144 kB
>> DirectMap1G:    390070272 kB
>>
>> -----------------------------------------------------------------------
>>
>> $ ./test /proc/meminfo 1024     # returns a copy of file after the remainder
>>
>> Will seek to 1024
>>
>>
>> Data read at offset 1024
>> gePages:         0 kB
>> ShmemHugePages:        0 kB
>> ShmemPmdMapped:        0 kB
>> HugePages_Total:       0
>> HugePages_Free:        0
>> HugePages_Rsvd:        0
>> HugePages_Surp:        0
>> Hugepagesize:       2048 kB
>> Hugetlb:               0 kB
>> DirectMap4k:      245204 kB
>> DirectMap2M:    13062144 kB
>> DirectMap1G:    390070272 kB
>> MemTotal:       394907728 kB
>> MemFree:        173738328 kB
>> MemAvailable:   379989680 kB
>> Buffers:          355812 kB
>> Cached:         207216224 kB
>> ...
>> DirectMap2M:    13062144 kB
>> DirectMap1G:    390070272 kB
>>
>> As you see, after "DirectMap1G:" line, a whole copy of /proc/meminfo returned by "read".
>>
>> Test program:
>>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>>
>> #define SIZE 1024
>> char buf[SIZE + 1];
>>
>> int main(int argc, char *argv[]) {
>>     int     fd;
>>     ssize_t rd;
>>     off_t   ofs = 0;
>>
>>     if (argc < 2) {
>>         printf("Usage: test <file> [<offset>]\n");
>>         exit(1);
>>     }
>>
>>     if (-1 == (fd = open(argv[1], O_RDONLY))) {
>>         perror("open failed");
>>         exit(1);
>>     }
>>
>>     if (argc > 2) {
>>         ofs = atol(argv[2]);
>>     }
>>     printf("Will seek to %ld\n", ofs);
>>
>>     if (-1 == (lseek(fd, ofs, SEEK_SET))) {
>>         perror("lseek failed");
>>         exit(1);
>>     }
>>
>>     for (;; ofs += rd) {
>>         printf("\n\nData read at offset %ld\n", ofs);
>>         if (-1 == (rd = read(fd, buf, SIZE))) {
>>             perror("read failed");
>>             exit(1);
>>         }
>>         buf[rd] = '\0';
>>         printf(buf);
>>         if (rd < SIZE) {
>>             break;
>>         }
>>     }
>>
>>     return 0;
>> }


2019-08-05 04:27:07

by NeilBrown

[permalink] [raw]
Subject: [PATCH] seq_file: fix problem when seeking mid-record.


If you use lseek or similar (e.g. pread) to access
a location in a seq_file file that is within a record,
rather than at a record boundary, then the first read
will return the remainder of the record, and the second
read will return the whole of that same record (instead
of the next record).
Whnn seeking to a record boundary, the next record is
correctly returned.

This bug was introduced by a recent patch (identified below)
Before that patch, seq_read() would increment m->index when
the last of the buffer was returned (m->count == 0).
After that patch, we rely on ->next to increment m->index
after filling the buffer - but there was one place where that
didn't happen.

Link: https://lkml.kernel.org/lkml/[email protected]/
Reported-by-tested-by: Sergei Turchanov <[email protected]>
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
and interface")
Cc: [email protected] (v4.19+)
Signed-off-by: NeilBrown <[email protected]>
---

Hi Andrew: as you applied the offending patch for me, maybe you could
queue up this fix too.
Thanks,
NeilBrown

fs/seq_file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 04f09689cd6d..1600034a929b 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -119,6 +119,7 @@ static int traverse(struct seq_file *m, loff_t offset)
}
if (seq_has_overflowed(m))
goto Eoverflow;
+ p = m->op->next(m, p, &m->index);
if (pos + m->count > offset) {
m->from = offset - pos;
m->count -= m->from;
@@ -126,7 +127,6 @@ static int traverse(struct seq_file *m, loff_t offset)
}
pos += m->count;
m->count = 0;
- p = m->op->next(m, p, &m->index);
if (pos == offset)
break;
}
--
2.14.0.rc0.dirty


Attachments:
signature.asc (847.00 B)

2019-08-05 09:17:21

by Markus Elfring

[permalink] [raw]
Subject: Re: seq_file: fix problem when seeking mid-record.

> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
> and interface")

Please do not split this tag across multiple lines in the final commit description.

Regards,
Markus

2019-08-05 10:10:48

by NeilBrown

[permalink] [raw]
Subject: Re: seq_file: fix problem when seeking mid-record.

On Mon, Aug 05 2019, Markus Elfring wrote:

>> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
>> and interface")
>
> Please do not split this tag across multiple lines in the final commit description.

I tend to agree...
I had previously seen
"Possible unwrapped commit description (prefer a maximum 75 chars per line)\n"
warnings from checkpatch, but one closer look that doesn't apply to
Fixes: lines (among other special cases).

Maybe Andrew will fix it up for me when it applies .... (please!)

Thanks,
NeilBrown


Attachments:
signature.asc (847.00 B)

2019-08-06 22:43:31

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] seq_file: fix problem when seeking mid-record.

On Mon, 05 Aug 2019 14:26:08 +1000 NeilBrown <[email protected]> wrote:

> If you use lseek or similar (e.g. pread) to access
> a location in a seq_file file that is within a record,
> rather than at a record boundary, then the first read
> will return the remainder of the record, and the second
> read will return the whole of that same record (instead
> of the next record).
> Whnn seeking to a record boundary, the next record is
> correctly returned.

ouch. I'm surprised it took this long to be noticed.

Maybe we need a seqfile-basher in tools/testing/selftests/proc or
somewhere.