* 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90
@ 2008-07-30 19:54 Alexey Dobriyan
2008-07-30 20:09 ` Linus Torvalds
0 siblings, 1 reply; 7+ messages in thread
From: Alexey Dobriyan @ 2008-07-30 19:54 UTC (permalink / raw)
To: torvalds, akpm, npiggin; +Cc: linux-kernel
Steps to reproduce:
# while true; do ./ftest03; done
ftest03 from LTP 20080603
BUG: unable to handle kernel paging request at ffff88017c72a008
IP: [<ffffffff8026190e>] iov_iter_advance+0x2e/0x90
PGD 202063 PUD b067 PMD 17def8163 PTE 800000017c72a160
Oops: 0000 [1] PREEMPT SMP DEBUG_PAGEALLOC
CPU 0
Modules linked in: af_packet ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 xt_state iptable_filter iptable_raw ip_tables x_tables nf_conntrack_irc nf_conntrack fuse usblp usbcore
Pid: 3546, comm: ftest03 Not tainted 2.6.27-rc1 #2
RIP: 0010:[<ffffffff8026190e>] [<ffffffff8026190e>] iov_iter_advance+0x2e/0x90
RSP: 0018:ffff88017c75fad8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000800 RCX: 0000000000000000
RDX: 0000000000000080 RSI: 0000000000000000 RDI: ffff88017c75fb78
RBP: ffff88017c75fad8 R08: ffff88017c72a000 R09: 0000000000000000
R10: 0000000000000004 R11: 0000000000000001 R12: 0000000000000800
R13: 000000000006e800 R14: ffff88017f6b7ac8 R15: 0000000000000800
FS: 00007f490298d6f0(0000) GS:ffffffff8051f780(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: ffff88017c72a008 CR3: 000000017c631000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process ftest03 (pid: 3546, threadinfo ffff88017c75e000, task ffff88017c51e540)
Stack: ffff88017c75fbd8 ffffffff80263452 000000004890c442 0000000000000246
000000007faae000 ffff88017c75fd98 000000000006e800 ffff88017c75fd18
ffff88017efefe00 ffff88017f6b7ac8 ffffffff80422fc0 ffff88017f6b78e0
Call Trace:
[<ffffffff80263452>] generic_file_buffered_write+0x1e2/0x710
[<ffffffff8040cfd0>] ? _spin_unlock+0x30/0x60
[<ffffffff80263e0f>] __generic_file_aio_write_nolock+0x29f/0x450
[<ffffffff80264026>] generic_file_aio_write+0x66/0xd0
[<ffffffff802c9506>] ext3_file_write+0x26/0xc0
[<ffffffff80264250>] ? generic_file_aio_read+0x0/0x670
[<ffffffff802c94e0>] ? ext3_file_write+0x0/0xc0
[<ffffffff8028921b>] do_sync_readv_writev+0xeb/0x130
[<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff802449c0>] ? autoremove_wake_function+0x0/0x40
[<ffffffff80289055>] ? rw_copy_check_uvector+0x95/0x130
[<ffffffff80289953>] do_readv_writev+0xc3/0x120
[<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff802527b5>] ? trace_hardirqs_on_caller+0xd5/0x160
[<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff802899e9>] vfs_writev+0x39/0x60
[<ffffffff80289d60>] sys_writev+0x50/0x90
[<ffffffff8020b65b>] system_call_fastpath+0x16/0x1b
Code: 77 18 48 89 e5 72 11 48 83 7f 08 01 75 11 48 01 77 10 48 29 77 18 c9 c3 0f 0b 0f 1f 00 eb fb 4c 8b 07 48 8b 4f 10 48 85 f6 75 17 <49> 83 78 08 00 75 07 48 83 7f 18 00 75 09 4c 89 07 48 89 4f 10
RIP [<ffffffff8026190e>] iov_iter_advance+0x2e/0x90
RSP <ffff88017c75fad8>
CR2: ffff88017c72a008
0xffffffff8026190e is in iov_iter_advance (mm/filemap.c:1882).
1877
1878 /*
1879 * The !iov->iov_len check ensures we skip over unlikely
1880 * zero-length segments (without overruning the iovec).
1881 */
1882 ===> while (bytes || unlikely(!iov->iov_len && i->count)) {
1883 int copy;
1884
1885 copy = min(bytes, iov->iov_len - base);
1886 BUG_ON(!i->count || i->count < copy);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90
2008-07-30 19:54 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90 Alexey Dobriyan
@ 2008-07-30 20:09 ` Linus Torvalds
2008-07-30 21:37 ` Alexey Dobriyan
2008-07-31 2:29 ` Nick Piggin
0 siblings, 2 replies; 7+ messages in thread
From: Linus Torvalds @ 2008-07-30 20:09 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: akpm, Nick Piggin, Linux Kernel Mailing List
On Wed, 30 Jul 2008, Alexey Dobriyan wrote:
>
> Steps to reproduce:
>
> # while true; do ./ftest03; done
>
> ftest03 from LTP 20080603
Hmm. The oops disassembles to
-12: 4c 8b 07 mov (%rdi),%r8
-9: 48 8b 4f 10 mov 0x10(%rdi),%rcx
-5: 48 85 f6 test %rsi,%rsi
-2: 75 17 jne 0x42
0: 49 83 78 08 00 cmpq $0x0,0x8(%r8) <---
5: 75 07 jne 0xe
7: 48 83 7f 18 00 cmpq $0x0,0x18(%rdi)
c: 75 09 jne 0x17
So it looks like we just overflowed %r8 to a new page and you presumably
have DEBUG_PAGEALLOC on.
(And yes, I see in the oops that you do)
> RIP [<ffffffff8026190e>] iov_iter_advance+0x2e/0x90
> Call Trace:
> [<ffffffff80263452>] generic_file_buffered_write+0x1e2/0x710
> [<ffffffff8040cfd0>] ? _spin_unlock+0x30/0x60
> [<ffffffff80263e0f>] __generic_file_aio_write_nolock+0x29f/0x450
> [<ffffffff80264026>] generic_file_aio_write+0x66/0xd0
> [<ffffffff802c9506>] ext3_file_write+0x26/0xc0
> [<ffffffff80264250>] ? generic_file_aio_read+0x0/0x670
> [<ffffffff802c94e0>] ? ext3_file_write+0x0/0xc0
> [<ffffffff8028921b>] do_sync_readv_writev+0xeb/0x130
> [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffff802449c0>] ? autoremove_wake_function+0x0/0x40
> [<ffffffff80289055>] ? rw_copy_check_uvector+0x95/0x130
> [<ffffffff80289953>] do_readv_writev+0xc3/0x120
> [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffff802527b5>] ? trace_hardirqs_on_caller+0xd5/0x160
> [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffff802899e9>] vfs_writev+0x39/0x60
> [<ffffffff80289d60>] sys_writev+0x50/0x90
> [<ffffffff8020b65b>] system_call_fastpath+0x16/0x1b
>
> 0xffffffff8026190e is in iov_iter_advance (mm/filemap.c:1882).
> 1877
> 1878 /*
> 1879 * The !iov->iov_len check ensures we skip over unlikely
> 1880 * zero-length segments (without overruning the iovec).
> 1881 */
> 1882 ===> while (bytes || unlikely(!iov->iov_len && i->count)) {
And yes, that oopsing op would be the one that loads 'iov->iov_len'.
So it very much looks like iov_iter_advance() advances past the end of the
iov array. We've had issues like that before. And I bet it's due to a
combination of Nick's commit f7009264c519603b8ec67c881bd368a56703cfc9
("iov_iter_advance() fix") and 124d3b7041f9a0ca7c43a6293e1cae4576c32fd5
("fix writev regression: pan hanging unkillable and un-straceable").
It's simply _not_ acceptable to look at iov->iov_len when 'bytes' has gone
down to zero, because there may be no 'iov' left!
Nick?
That said, I do think that we have another issue with iovec's - I think we
should strive to always pass in the number of iovec's when we pass a
pointer to an iovec, in addition to the bytes. The sad part is that
'iov_iter_advance' actually -has- the count, but it's the byte count
remaining, not the iovec's remaining.
In this particular case, the trivial fix _may_ be to just change the order
of testing iov->iov_len && i->count, but I really think we should also
count actual iov entries and pass them around (and keep them updated).
So does this (hacky, ugly) patch fix it for you?
Linus
---
mm/filemap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index 42bbc69..d97d1ad 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1879,7 +1879,7 @@ void iov_iter_advance(struct iov_iter *i, size_t bytes)
* The !iov->iov_len check ensures we skip over unlikely
* zero-length segments (without overruning the iovec).
*/
- while (bytes || unlikely(!iov->iov_len && i->count)) {
+ while (bytes || unlikely(i->count && !iov->iov_len)) {
int copy;
copy = min(bytes, iov->iov_len - base);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90
2008-07-30 20:09 ` Linus Torvalds
@ 2008-07-30 21:37 ` Alexey Dobriyan
2008-07-30 21:44 ` Linus Torvalds
2008-07-31 2:29 ` Nick Piggin
1 sibling, 1 reply; 7+ messages in thread
From: Alexey Dobriyan @ 2008-07-30 21:37 UTC (permalink / raw)
To: Linus Torvalds; +Cc: akpm, Nick Piggin, Linux Kernel Mailing List
On Wed, Jul 30, 2008 at 01:09:21PM -0700, Linus Torvalds wrote:
> On Wed, 30 Jul 2008, Alexey Dobriyan wrote:
> >
> > Steps to reproduce:
> >
> > # while true; do ./ftest03; done
> >
> > ftest03 from LTP 20080603
>
> Hmm. The oops disassembles to
>
> -12: 4c 8b 07 mov (%rdi),%r8
> -9: 48 8b 4f 10 mov 0x10(%rdi),%rcx
> -5: 48 85 f6 test %rsi,%rsi
> -2: 75 17 jne 0x42
> 0: 49 83 78 08 00 cmpq $0x0,0x8(%r8) <---
> 5: 75 07 jne 0xe
> 7: 48 83 7f 18 00 cmpq $0x0,0x18(%rdi)
> c: 75 09 jne 0x17
>
> So it looks like we just overflowed %r8 to a new page and you presumably
> have DEBUG_PAGEALLOC on.
>
> (And yes, I see in the oops that you do)
>
> > RIP [<ffffffff8026190e>] iov_iter_advance+0x2e/0x90
> > Call Trace:
> > [<ffffffff80263452>] generic_file_buffered_write+0x1e2/0x710
> > [<ffffffff8040cfd0>] ? _spin_unlock+0x30/0x60
> > [<ffffffff80263e0f>] __generic_file_aio_write_nolock+0x29f/0x450
> > [<ffffffff80264026>] generic_file_aio_write+0x66/0xd0
> > [<ffffffff802c9506>] ext3_file_write+0x26/0xc0
> > [<ffffffff80264250>] ? generic_file_aio_read+0x0/0x670
> > [<ffffffff802c94e0>] ? ext3_file_write+0x0/0xc0
> > [<ffffffff8028921b>] do_sync_readv_writev+0xeb/0x130
> > [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> > [<ffffffff802449c0>] ? autoremove_wake_function+0x0/0x40
> > [<ffffffff80289055>] ? rw_copy_check_uvector+0x95/0x130
> > [<ffffffff80289953>] do_readv_writev+0xc3/0x120
> > [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> > [<ffffffff802527b5>] ? trace_hardirqs_on_caller+0xd5/0x160
> > [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> > [<ffffffff802899e9>] vfs_writev+0x39/0x60
> > [<ffffffff80289d60>] sys_writev+0x50/0x90
> > [<ffffffff8020b65b>] system_call_fastpath+0x16/0x1b
> >
> > 0xffffffff8026190e is in iov_iter_advance (mm/filemap.c:1882).
> > 1877
> > 1878 /*
> > 1879 * The !iov->iov_len check ensures we skip over unlikely
> > 1880 * zero-length segments (without overruning the iovec).
> > 1881 */
> > 1882 ===> while (bytes || unlikely(!iov->iov_len && i->count)) {
>
> And yes, that oopsing op would be the one that loads 'iov->iov_len'.
>
> So it very much looks like iov_iter_advance() advances past the end of the
> iov array. We've had issues like that before. And I bet it's due to a
> combination of Nick's commit f7009264c519603b8ec67c881bd368a56703cfc9
> ("iov_iter_advance() fix") and 124d3b7041f9a0ca7c43a6293e1cae4576c32fd5
> ("fix writev regression: pan hanging unkillable and un-straceable").
>
> It's simply _not_ acceptable to look at iov->iov_len when 'bytes' has gone
> down to zero, because there may be no 'iov' left!
>
> Nick?
>
> That said, I do think that we have another issue with iovec's - I think we
> should strive to always pass in the number of iovec's when we pass a
> pointer to an iovec, in addition to the bytes. The sad part is that
> 'iov_iter_advance' actually -has- the count, but it's the byte count
> remaining, not the iovec's remaining.
>
> In this particular case, the trivial fix _may_ be to just change the order
> of testing iov->iov_len && i->count, but I really think we should also
> count actual iov entries and pass them around (and keep them updated).
>
> So does this (hacky, ugly) patch fix it for you?
You forgot "untested". And, yes, it helps.
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1879,7 +1879,7 @@ void iov_iter_advance(struct iov_iter *i, size_t bytes)
> * The !iov->iov_len check ensures we skip over unlikely
> * zero-length segments (without overruning the iovec).
> */
> - while (bytes || unlikely(!iov->iov_len && i->count)) {
> + while (bytes || unlikely(i->count && !iov->iov_len)) {
> int copy;
>
> copy = min(bytes, iov->iov_len - base);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90
2008-07-30 21:37 ` Alexey Dobriyan
@ 2008-07-30 21:44 ` Linus Torvalds
2008-07-30 22:02 ` Linus Torvalds
0 siblings, 1 reply; 7+ messages in thread
From: Linus Torvalds @ 2008-07-30 21:44 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: akpm, Nick Piggin, Linux Kernel Mailing List
On Thu, 31 Jul 2008, Alexey Dobriyan wrote:
> >
> > So does this (hacky, ugly) patch fix it for you?
>
> You forgot "untested".
I'm trying to teach people to just take my patches for granted as being
untested. I think there is more shock value when I actually claim one of
my patches is actually _tested_, so I guess I'll just emphasize that part
in case it ever happens (and ok, it did happen for the cpu_mask thing).
> And, yes, it helps.
Ok, I'll commit it as obvious, ugly as it may be. And hope that Nick or
some other god of iov_iter will think about trying to make these things
more reliable and easier to use.
Linus
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90
2008-07-30 21:44 ` Linus Torvalds
@ 2008-07-30 22:02 ` Linus Torvalds
2008-07-30 22:32 ` Alexey Dobriyan
0 siblings, 1 reply; 7+ messages in thread
From: Linus Torvalds @ 2008-07-30 22:02 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: akpm, Nick Piggin, Linux Kernel Mailing List, stable
On Wed, 30 Jul 2008, Linus Torvalds wrote:
>
> Ok, I'll commit it as obvious, ugly as it may be. And hope that Nick or
> some other god of iov_iter will think about trying to make these things
> more reliable and easier to use.
Oh, and I think it's stable material. Unless I misread something, this
area hasn't changed since 2.6.25. But somebody should double-check me,
that just sounds wrong.
stable added to cc, with patch appended.
Linus
---
commit 94ad374a0751f40d25e22e036c37f7263569d24c
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed Jul 30 14:45:12 2008 -0700
Fix off-by-one error in iov_iter_advance()
The iov_iter_advance() function would look at the iov->iov_len entry
even though it might have iterated over the whole array, and iov was
pointing past the end. This would cause DEBUG_PAGEALLOC to trigger a
kernel page fault if the allocation was at the end of a page, and the
next page was unallocated.
The quick fix is to just change the order of the tests: check that there
is any iovec data left before we check the iov entry itself.
Thanks to Alexey Dobriyan for finding this case, and testing the fix.
Reported-and-tested-by: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: <stable@kernel.org> [2.6.25.x, 2.6.26.x]
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
mm/filemap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index 42bbc69..d97d1ad 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1879,7 +1879,7 @@ void iov_iter_advance(struct iov_iter *i, size_t bytes)
* The !iov->iov_len check ensures we skip over unlikely
* zero-length segments (without overruning the iovec).
*/
- while (bytes || unlikely(!iov->iov_len && i->count)) {
+ while (bytes || unlikely(i->count && !iov->iov_len)) {
int copy;
copy = min(bytes, iov->iov_len - base);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90
2008-07-30 22:02 ` Linus Torvalds
@ 2008-07-30 22:32 ` Alexey Dobriyan
0 siblings, 0 replies; 7+ messages in thread
From: Alexey Dobriyan @ 2008-07-30 22:32 UTC (permalink / raw)
To: Linus Torvalds; +Cc: akpm, Nick Piggin, Linux Kernel Mailing List, stable
On Wed, Jul 30, 2008 at 03:02:32PM -0700, Linus Torvalds wrote:
> On Wed, 30 Jul 2008, Linus Torvalds wrote:
> >
> > Ok, I'll commit it as obvious, ugly as it may be. And hope that Nick or
> > some other god of iov_iter will think about trying to make these things
> > more reliable and easier to use.
>
> Oh, and I think it's stable material. Unless I misread something, this
> area hasn't changed since 2.6.25. But somebody should double-check me,
> that just sounds wrong.
2.6.26 fails the same way and patch also seems to help.
> commit 94ad374a0751f40d25e22e036c37f7263569d24c
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed Jul 30 14:45:12 2008 -0700
>
> Fix off-by-one error in iov_iter_advance()
>
> The iov_iter_advance() function would look at the iov->iov_len entry
> even though it might have iterated over the whole array, and iov was
> pointing past the end. This would cause DEBUG_PAGEALLOC to trigger a
> kernel page fault if the allocation was at the end of a page, and the
> next page was unallocated.
>
> The quick fix is to just change the order of the tests: check that there
> is any iovec data left before we check the iov entry itself.
>
> Thanks to Alexey Dobriyan for finding this case, and testing the fix.
>
> Reported-and-tested-by: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: <stable@kernel.org> [2.6.25.x, 2.6.26.x]
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> mm/filemap.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 42bbc69..d97d1ad 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1879,7 +1879,7 @@ void iov_iter_advance(struct iov_iter *i, size_t bytes)
> * The !iov->iov_len check ensures we skip over unlikely
> * zero-length segments (without overruning the iovec).
> */
> - while (bytes || unlikely(!iov->iov_len && i->count)) {
> + while (bytes || unlikely(i->count && !iov->iov_len)) {
> int copy;
>
> copy = min(bytes, iov->iov_len - base);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90
2008-07-30 20:09 ` Linus Torvalds
2008-07-30 21:37 ` Alexey Dobriyan
@ 2008-07-31 2:29 ` Nick Piggin
1 sibling, 0 replies; 7+ messages in thread
From: Nick Piggin @ 2008-07-31 2:29 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexey Dobriyan, akpm, Nick Piggin, Linux Kernel Mailing List
On Thursday 31 July 2008 06:09, Linus Torvalds wrote:
> On Wed, 30 Jul 2008, Alexey Dobriyan wrote:
> > Steps to reproduce:
> >
> > # while true; do ./ftest03; done
> >
> > ftest03 from LTP 20080603
>
> Hmm. The oops disassembles to
>
> -12: 4c 8b 07 mov (%rdi),%r8
> -9: 48 8b 4f 10 mov 0x10(%rdi),%rcx
> -5: 48 85 f6 test %rsi,%rsi
> -2: 75 17 jne 0x42
> 0: 49 83 78 08 00 cmpq $0x0,0x8(%r8) <---
> 5: 75 07 jne 0xe
> 7: 48 83 7f 18 00 cmpq $0x0,0x18(%rdi)
> c: 75 09 jne 0x17
>
> So it looks like we just overflowed %r8 to a new page and you presumably
> have DEBUG_PAGEALLOC on.
>
> (And yes, I see in the oops that you do)
>
> > RIP [<ffffffff8026190e>] iov_iter_advance+0x2e/0x90
> > Call Trace:
> > [<ffffffff80263452>] generic_file_buffered_write+0x1e2/0x710
> > [<ffffffff8040cfd0>] ? _spin_unlock+0x30/0x60
> > [<ffffffff80263e0f>] __generic_file_aio_write_nolock+0x29f/0x450
> > [<ffffffff80264026>] generic_file_aio_write+0x66/0xd0
> > [<ffffffff802c9506>] ext3_file_write+0x26/0xc0
> > [<ffffffff80264250>] ? generic_file_aio_read+0x0/0x670
> > [<ffffffff802c94e0>] ? ext3_file_write+0x0/0xc0
> > [<ffffffff8028921b>] do_sync_readv_writev+0xeb/0x130
> > [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> > [<ffffffff802449c0>] ? autoremove_wake_function+0x0/0x40
> > [<ffffffff80289055>] ? rw_copy_check_uvector+0x95/0x130
> > [<ffffffff80289953>] do_readv_writev+0xc3/0x120
> > [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> > [<ffffffff802527b5>] ? trace_hardirqs_on_caller+0xd5/0x160
> > [<ffffffff8025284d>] ? trace_hardirqs_on+0xd/0x10
> > [<ffffffff802899e9>] vfs_writev+0x39/0x60
> > [<ffffffff80289d60>] sys_writev+0x50/0x90
> > [<ffffffff8020b65b>] system_call_fastpath+0x16/0x1b
> >
> > 0xffffffff8026190e is in iov_iter_advance (mm/filemap.c:1882).
> > 1877
> > 1878 /*
> > 1879 * The !iov->iov_len check ensures we skip over
> > unlikely 1880 * zero-length segments (without
> > overruning the iovec). 1881 */
> > 1882 ===> while (bytes || unlikely(!iov->iov_len &&
> > i->count)) {
>
> And yes, that oopsing op would be the one that loads 'iov->iov_len'.
>
> So it very much looks like iov_iter_advance() advances past the end of the
> iov array. We've had issues like that before. And I bet it's due to a
> combination of Nick's commit f7009264c519603b8ec67c881bd368a56703cfc9
> ("iov_iter_advance() fix") and 124d3b7041f9a0ca7c43a6293e1cae4576c32fd5
> ("fix writev regression: pan hanging unkillable and un-straceable").
>
> It's simply _not_ acceptable to look at iov->iov_len when 'bytes' has gone
> down to zero, because there may be no 'iov' left!
>
> Nick?
Thanks Linus, patch looks exactly right.
> That said, I do think that we have another issue with iovec's - I think we
> should strive to always pass in the number of iovec's when we pass a
> pointer to an iovec, in addition to the bytes. The sad part is that
> 'iov_iter_advance' actually -has- the count, but it's the byte count
> remaining, not the iovec's remaining.
>
> In this particular case, the trivial fix _may_ be to just change the order
> of testing iov->iov_len && i->count, but I really think we should also
> count actual iov entries and pass them around (and keep them updated).
>
> So does this (hacky, ugly) patch fix it for you?
>
> Linus
>
> ---
> mm/filemap.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 42bbc69..d97d1ad 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1879,7 +1879,7 @@ void iov_iter_advance(struct iov_iter *i, size_t
> bytes) * The !iov->iov_len check ensures we skip over unlikely
> * zero-length segments (without overruning the iovec).
> */
> - while (bytes || unlikely(!iov->iov_len && i->count)) {
> + while (bytes || unlikely(i->count && !iov->iov_len)) {
> int copy;
>
> copy = min(bytes, iov->iov_len - base);
> --
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-07-31 2:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-07-30 19:54 2.6.27-rc1: IP: iov_iter_advance+0x2e/0x90 Alexey Dobriyan
2008-07-30 20:09 ` Linus Torvalds
2008-07-30 21:37 ` Alexey Dobriyan
2008-07-30 21:44 ` Linus Torvalds
2008-07-30 22:02 ` Linus Torvalds
2008-07-30 22:32 ` Alexey Dobriyan
2008-07-31 2:29 ` Nick Piggin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®