mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* 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®