Re: [PATCH 1/6] seq_file: add seq_read_iter

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Nathan Chancellor]

Hi Al,

On Wed, Nov 11, 2020 at 10:21:16PM +0000, Al Viro wrote:
> On Wed, Nov 11, 2020 at 09:52:20PM +0000, Al Viro wrote:
> 
> > That can be done, but I would rather go with
> > 		n = copy_to_iter(m->buf + m->from, m->count, iter);
> > 		m->count -= n;
> > 		m->from += n;
> >                 copied += n;
> >                 if (!size)
> >                         goto Done;
> > 		if (m->count)
> > 			goto Efault;
> > if we do it that way.  Let me see if I can cook something
> > reasonable along those lines...
> 
> Something like below (build-tested only):
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3b20e21604e7..07b33c1f34a9 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
>  ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  {
>  	struct seq_file *m = iocb->ki_filp->private_data;
> -	size_t size = iov_iter_count(iter);
>  	size_t copied = 0;
>  	size_t n;
>  	void *p;
> @@ -208,14 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  	}
>  	/* if not empty - flush it first */
>  	if (m->count) {
> -		n = min(m->count, size);
> -		if (copy_to_iter(m->buf + m->from, n, iter) != n)
> -			goto Efault;
> +		n = copy_to_iter(m->buf + m->from, m->count, iter);
>  		m->count -= n;
>  		m->from += n;
> -		size -= n;
>  		copied += n;
> -		if (!size)
> +		if (!iov_iter_count(iter) || m->count)
>  			goto Done;
>  	}
>  	/* we need at least one record in buffer */
> @@ -249,6 +245,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  	goto Done;
>  Fill:
>  	/* they want more? let's try to get some more */
> +	/* m->count is positive and there's space left in iter */
>  	while (1) {
>  		size_t offs = m->count;
>  		loff_t pos = m->index;
> @@ -263,7 +260,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  			err = PTR_ERR(p);
>  			break;
>  		}
> -		if (m->count >= size)
> +		if (m->count >= iov_iter_count(iter))
>  			break;
>  		err = m->op->show(m, p);
>  		if (seq_has_overflowed(m) || err) {
> @@ -273,16 +270,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		}
>  	}
>  	m->op->stop(m, p);
> -	n = min(m->count, size);
> -	if (copy_to_iter(m->buf, n, iter) != n)
> -		goto Efault;
> +	n = copy_to_iter(m->buf, m->count, iter);
>  	copied += n;
>  	m->count -= n;
>  	m->from = n;
>  Done:
> -	if (!copied)
> -		copied = err;
> -	else {
> +	if (unlikely(!copied)) {
> +		copied = m->count ? -EFAULT : err;
> +	} else {
>  		iocb->ki_pos += copied;
>  		m->read_pos += copied;
>  	}
> @@ -291,9 +286,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  Enomem:
>  	err = -ENOMEM;
>  	goto Done;
> -Efault:
> -	err = -EFAULT;
> -	goto Done;
>  }
>  EXPORT_SYMBOL(seq_read_iter);
>  

This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks
WSL2's interoperability feature, where Windows paths automatically get
added to PATH on start up so that Windows binaries can be accessed from
within Linux (such as clip.exe to pipe output to the clipboard). Before,
I would see a bunch of Linux + Windows folders in $PATH but after, I
only see the Linux folders (I can give you the actual PATH value if you
care but it is really long).

I am not at all familiar with the semantics of this patch or how
Microsoft would be using it to inject folders into PATH (they have some
documentation on it here:
https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure
how to go about figuring that out to see why this patch breaks something
(unless you have an idea). I have added the Hyper-V maintainers and list
to CC in case they know someone who could help.

Cheers,
Nathan

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Fri, Nov 13, 2020 at 04:54:53PM -0700, Nathan Chancellor wrote:

> This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks
> WSL2's interoperability feature, where Windows paths automatically get
> added to PATH on start up so that Windows binaries can be accessed from
> within Linux (such as clip.exe to pipe output to the clipboard). Before,
> I would see a bunch of Linux + Windows folders in $PATH but after, I
> only see the Linux folders (I can give you the actual PATH value if you
> care but it is really long).
> 
> I am not at all familiar with the semantics of this patch or how
> Microsoft would be using it to inject folders into PATH (they have some
> documentation on it here:
> https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure
> how to go about figuring that out to see why this patch breaks something
> (unless you have an idea). I have added the Hyper-V maintainers and list
> to CC in case they know someone who could help.

Out of curiosity: could you slap WARN_ON(!iov_iter_count(iter)); right in
the beginning of seq_read_iter() and see if that triggers?

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Nathan Chancellor]

On Sat, Nov 14, 2020 at 01:17:54AM +0000, Al Viro wrote:
> On Fri, Nov 13, 2020 at 04:54:53PM -0700, Nathan Chancellor wrote:
> 
> > This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks
> > WSL2's interoperability feature, where Windows paths automatically get
> > added to PATH on start up so that Windows binaries can be accessed from
> > within Linux (such as clip.exe to pipe output to the clipboard). Before,
> > I would see a bunch of Linux + Windows folders in $PATH but after, I
> > only see the Linux folders (I can give you the actual PATH value if you
> > care but it is really long).
> > 
> > I am not at all familiar with the semantics of this patch or how
> > Microsoft would be using it to inject folders into PATH (they have some
> > documentation on it here:
> > https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure
> > how to go about figuring that out to see why this patch breaks something
> > (unless you have an idea). I have added the Hyper-V maintainers and list
> > to CC in case they know someone who could help.
> 
> Out of curiosity: could you slap WARN_ON(!iov_iter_count(iter)); right in
> the beginning of seq_read_iter() and see if that triggers?

Sure thing, it does trigger.

[    0.235058] ------------[ cut here ]------------
[    0.235062] WARNING: CPU: 15 PID: 237 at fs/seq_file.c:176 seq_read_iter+0x3b3/0x3f0
[    0.235064] CPU: 15 PID: 237 Comm: localhost Not tainted 5.10.0-rc2-microsoft-cbl-00002-g6a9f696d1627-dirty #15
[    0.235065] RIP: 0010:seq_read_iter+0x3b3/0x3f0
[    0.235066] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b 54 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff <0f> 0b e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00
[    0.235067] RSP: 0018:ffff9c774063bd08 EFLAGS: 00010246
[    0.235068] RAX: ffff91a77ac01f00 RBX: ffff91a50133c348 RCX: 0000000000000001
[    0.235069] RDX: ffff9c774063bdb8 RSI: ffff9c774063bd60 RDI: ffff9c774063bd88
[    0.235069] RBP: 0000000000000000 R08: 0000000000000000 R09: ffff91a50058b768
[    0.235070] R10: ffff91a7f79f0000 R11: ffffffffbc2c2030 R12: ffff9c774063bd88
[    0.235070] R13: ffff9c774063bd60 R14: ffff9c774063be48 R15: ffff91a77af58900
[    0.235072] FS:  000000000029c800(0000) GS:ffff91a7f7bc0000(0000) knlGS:0000000000000000
[    0.235073] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.235073] CR2: 00007ab6c1fabad0 CR3: 000000037a004000 CR4: 0000000000350ea0
[    0.235074] Call Trace:
[    0.235077]  seq_read+0x127/0x150
[    0.235078]  proc_reg_read+0x42/0xa0
[    0.235080]  do_iter_read+0x14c/0x1e0
[    0.235081]  do_readv+0x18d/0x240
[    0.235083]  do_syscall_64+0x33/0x70
[    0.235085]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    0.235086] RIP: 0033:0x22c483
[    0.235086] Code: 4e 66 48 0f 7e c8 48 83 f8 01 48 89 d0 48 83 d0 ff 48 89 46 08 66 0f 7f 46 10 48 63 7f 78 b8 13 00 00 00 ba 02 00 00 00 0f 05 <48> 89 c7 e8 15 bb ff ff 48 85 c0 7e 34 48 89 c1 48 2b 4c 24 08 76
[    0.235087] RSP: 002b:00007ffca2245ca0 EFLAGS: 00000257 ORIG_RAX: 0000000000000013
[    0.235088] RAX: ffffffffffffffda RBX: 0000000000a58120 RCX: 000000000022c483
[    0.235088] RDX: 0000000000000002 RSI: 00007ffca2245ca0 RDI: 0000000000000005
[    0.235089] RBP: 00000000ffffffff R08: fefefefefefefeff R09: 8080808080808080
[    0.235089] R10: 00007ab6c1fabb20 R11: 0000000000000257 R12: 0000000000a58120
[    0.235089] R13: 00007ffca2245d90 R14: 0000000000000001 R15: 00007ffca2245ce7
[    0.235091] CPU: 15 PID: 237 Comm: localhost Not tainted 5.10.0-rc2-microsoft-cbl-00002-g6a9f696d1627-dirty #15
[    0.235091] Call Trace:
[    0.235092]  dump_stack+0xa1/0xfb
[    0.235094]  __warn+0x7f/0x120
[    0.235095]  ? seq_read_iter+0x3b3/0x3f0
[    0.235096]  report_bug+0xb1/0x110
[    0.235097]  handle_bug+0x3d/0x70
[    0.235098]  exc_invalid_op+0x18/0xb0
[    0.235098]  asm_exc_invalid_op+0x12/0x20
[    0.235100] RIP: 0010:seq_read_iter+0x3b3/0x3f0
[    0.235100] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b 54 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff <0f> 0b e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00
[    0.235101] RSP: 0018:ffff9c774063bd08 EFLAGS: 00010246
[    0.235101] RAX: ffff91a77ac01f00 RBX: ffff91a50133c348 RCX: 0000000000000001
[    0.235102] RDX: ffff9c774063bdb8 RSI: ffff9c774063bd60 RDI: ffff9c774063bd88
[    0.235102] RBP: 0000000000000000 R08: 0000000000000000 R09: ffff91a50058b768
[    0.235103] R10: ffff91a7f79f0000 R11: ffffffffbc2c2030 R12: ffff9c774063bd88
[    0.235103] R13: ffff9c774063bd60 R14: ffff9c774063be48 R15: ffff91a77af58900
[    0.235104]  ? seq_open+0x70/0x70
[    0.235105]  ? path_openat+0xbc0/0xc40
[    0.235106]  seq_read+0x127/0x150
[    0.235107]  proc_reg_read+0x42/0xa0
[    0.235108]  do_iter_read+0x14c/0x1e0
[    0.235109]  do_readv+0x18d/0x240
[    0.235109]  do_syscall_64+0x33/0x70
[    0.235110]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    0.235111] RIP: 0033:0x22c483
[    0.235111] Code: 4e 66 48 0f 7e c8 48 83 f8 01 48 89 d0 48 83 d0 ff 48 89 46 08 66 0f 7f 46 10 48 63 7f 78 b8 13 00 00 00 ba 02 00 00 00 0f 05 <48> 89 c7 e8 15 bb ff ff 48 85 c0 7e 34 48 89 c1 48 2b 4c 24 08 76
[    0.235112] RSP: 002b:00007ffca2245ca0 EFLAGS: 00000257 ORIG_RAX: 0000000000000013
[    0.235113] RAX: ffffffffffffffda RBX: 0000000000a58120 RCX: 000000000022c483
[    0.235113] RDX: 0000000000000002 RSI: 00007ffca2245ca0 RDI: 0000000000000005
[    0.235113] RBP: 00000000ffffffff R08: fefefefefefefeff R09: 8080808080808080
[    0.235114] R10: 00007ab6c1fabb20 R11: 0000000000000257 R12: 0000000000a58120
[    0.235114] R13: 00007ffca2245d90 R14: 0000000000000001 R15: 00007ffca2245ce7
[    0.235115] ---[ end trace 92966dbcf1e9cae5 ]---

Cheers,
Nathan

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Fri, Nov 13, 2020 at 08:01:24PM -0700, Nathan Chancellor wrote:
> Sure thing, it does trigger.
> 
> [    0.235058] ------------[ cut here ]------------
> [    0.235062] WARNING: CPU: 15 PID: 237 at fs/seq_file.c:176 seq_read_iter+0x3b3/0x3f0
> [    0.235064] CPU: 15 PID: 237 Comm: localhost Not tainted 5.10.0-rc2-microsoft-cbl-00002-g6a9f696d1627-dirty #15
> [    0.235065] RIP: 0010:seq_read_iter+0x3b3/0x3f0
> [    0.235066] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b 54 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff <0f> 0b e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00
> [    0.235067] RSP: 0018:ffff9c774063bd08 EFLAGS: 00010246
> [    0.235068] RAX: ffff91a77ac01f00 RBX: ffff91a50133c348 RCX: 0000000000000001
> [    0.235069] RDX: ffff9c774063bdb8 RSI: ffff9c774063bd60 RDI: ffff9c774063bd88
> [    0.235069] RBP: 0000000000000000 R08: 0000000000000000 R09: ffff91a50058b768
> [    0.235070] R10: ffff91a7f79f0000 R11: ffffffffbc2c2030 R12: ffff9c774063bd88
> [    0.235070] R13: ffff9c774063bd60 R14: ffff9c774063be48 R15: ffff91a77af58900
> [    0.235072] FS:  000000000029c800(0000) GS:ffff91a7f7bc0000(0000) knlGS:0000000000000000
> [    0.235073] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.235073] CR2: 00007ab6c1fabad0 CR3: 000000037a004000 CR4: 0000000000350ea0
> [    0.235074] Call Trace:
> [    0.235077]  seq_read+0x127/0x150
> [    0.235078]  proc_reg_read+0x42/0xa0
> [    0.235080]  do_iter_read+0x14c/0x1e0
> [    0.235081]  do_readv+0x18d/0x240
> [    0.235083]  do_syscall_64+0x33/0x70
> [    0.235085]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

*blink*

	Lovely...  For one thing, it did *not* go through
proc_reg_read_iter().  For another, it has hit proc_reg_read() with
zero length, which must've been an iovec with zero ->iov_len in
readv(2) arguments.  I wonder if we should use that kind of
pathology (readv() with zero-length segment in the middle of
iovec array) for regression tests...

	OK...  First of all, since that kind of crap can happen,
let's do this (incremental to be folded); then (and that's
a separate patch) we ought to switch the proc_ops with ->proc_read
equal to seq_read to ->proc_read_iter = seq_read_iter, so that
those guys would not mess with seq_read() wrapper at all.

	Finally, is there any point having do_loop_readv_writev()
call any methods for zero-length segments?

	In any case, the following should be folded into
"fix return values of seq_read_iter()"; could you check if that
fixes the problem you are seeing?

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 07b33c1f34a9..e66d6b8bae23 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -211,9 +211,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		m->count -= n;
 		m->from += n;
 		copied += n;
-		if (!iov_iter_count(iter) || m->count)
-			goto Done;
 	}
+	if (m->count || !iov_iter_count(iter))
+		goto Done;
 	/* we need at least one record in buffer */
 	m->from = 0;
 	p = m->op->start(m, &m->index);

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Nathan Chancellor]

On Sat, Nov 14, 2020 at 03:54:53AM +0000, Al Viro wrote:
> On Fri, Nov 13, 2020 at 08:01:24PM -0700, Nathan Chancellor wrote:
> > Sure thing, it does trigger.
> > 
> > [    0.235058] ------------[ cut here ]------------
> > [    0.235062] WARNING: CPU: 15 PID: 237 at fs/seq_file.c:176 seq_read_iter+0x3b3/0x3f0
> > [    0.235064] CPU: 15 PID: 237 Comm: localhost Not tainted 5.10.0-rc2-microsoft-cbl-00002-g6a9f696d1627-dirty #15
> > [    0.235065] RIP: 0010:seq_read_iter+0x3b3/0x3f0
> > [    0.235066] Code: ba 01 00 00 00 e8 6d d2 fc ff 4c 89 e7 48 89 ee 48 8b 54 24 10 e8 ad 8b 45 00 49 01 c5 48 29 43 18 48 89 43 10 e9 61 fe ff ff <0f> 0b e9 6f fc ff ff 0f 0b 45 31 ed e9 0d fd ff ff 48 c7 43 18 00
> > [    0.235067] RSP: 0018:ffff9c774063bd08 EFLAGS: 00010246
> > [    0.235068] RAX: ffff91a77ac01f00 RBX: ffff91a50133c348 RCX: 0000000000000001
> > [    0.235069] RDX: ffff9c774063bdb8 RSI: ffff9c774063bd60 RDI: ffff9c774063bd88
> > [    0.235069] RBP: 0000000000000000 R08: 0000000000000000 R09: ffff91a50058b768
> > [    0.235070] R10: ffff91a7f79f0000 R11: ffffffffbc2c2030 R12: ffff9c774063bd88
> > [    0.235070] R13: ffff9c774063bd60 R14: ffff9c774063be48 R15: ffff91a77af58900
> > [    0.235072] FS:  000000000029c800(0000) GS:ffff91a7f7bc0000(0000) knlGS:0000000000000000
> > [    0.235073] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    0.235073] CR2: 00007ab6c1fabad0 CR3: 000000037a004000 CR4: 0000000000350ea0
> > [    0.235074] Call Trace:
> > [    0.235077]  seq_read+0x127/0x150
> > [    0.235078]  proc_reg_read+0x42/0xa0
> > [    0.235080]  do_iter_read+0x14c/0x1e0
> > [    0.235081]  do_readv+0x18d/0x240
> > [    0.235083]  do_syscall_64+0x33/0x70
> > [    0.235085]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> *blink*
> 
> 	Lovely...  For one thing, it did *not* go through
> proc_reg_read_iter().  For another, it has hit proc_reg_read() with
> zero length, which must've been an iovec with zero ->iov_len in
> readv(2) arguments.  I wonder if we should use that kind of
> pathology (readv() with zero-length segment in the middle of
> iovec array) for regression tests...
> 
> 	OK...  First of all, since that kind of crap can happen,
> let's do this (incremental to be folded); then (and that's
> a separate patch) we ought to switch the proc_ops with ->proc_read
> equal to seq_read to ->proc_read_iter = seq_read_iter, so that
> those guys would not mess with seq_read() wrapper at all.
> 
> 	Finally, is there any point having do_loop_readv_writev()
> call any methods for zero-length segments?
> 
> 	In any case, the following should be folded into
> "fix return values of seq_read_iter()"; could you check if that
> fixes the problem you are seeing?
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 07b33c1f34a9..e66d6b8bae23 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -211,9 +211,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		m->count -= n;
>  		m->from += n;
>  		copied += n;
> -		if (!iov_iter_count(iter) || m->count)
> -			goto Done;
>  	}
> +	if (m->count || !iov_iter_count(iter))
> +		goto Done;
>  	/* we need at least one record in buffer */
>  	m->from = 0;
>  	p = m->op->start(m, &m->index);

Unfortunately that patch does not solve my issue. Is there any other
debugging I should add?

Cheers,
Nathan

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Fri, Nov 13, 2020 at 09:14:20PM -0700, Nathan Chancellor wrote:

> Unfortunately that patch does not solve my issue. Is there any other
> debugging I should add?

Hmm...  I wonder which file it is; how about
		if (WARN_ON(!iovec.iov_len))
			printk(KERN_ERR "odd readv on %pd4\n", file);
in the loop in fs/read_write.c:do_loop_readv_writev()?

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Fri, Nov 13, 2020 at 11:19:34PM -0700, Nathan Chancellor wrote:

> Assuming so, I have attached the output both with and without the
> WARN_ON. Looks like mountinfo is what is causing the error?

Cute...  FWIW, on #origin + that commit with fix folded in I don't
see anything unusual in reads from mountinfo ;-/  OTOH, they'd
obviously been... creative with readv(2) arguments, so it would
be very interesting to see what it is they are passing to it.

I'm half-asleep right now; will try to cook something to gather
that information tomorrow morning.  'Later...

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Sat, Nov 14, 2020 at 07:00:25AM +0000, Al Viro wrote:
> On Fri, Nov 13, 2020 at 11:19:34PM -0700, Nathan Chancellor wrote:
> 
> > Assuming so, I have attached the output both with and without the
> > WARN_ON. Looks like mountinfo is what is causing the error?
> 
> Cute...  FWIW, on #origin + that commit with fix folded in I don't
> see anything unusual in reads from mountinfo ;-/  OTOH, they'd
> obviously been... creative with readv(2) arguments, so it would
> be very interesting to see what it is they are passing to it.
> 
> I'm half-asleep right now; will try to cook something to gather
> that information tomorrow morning.  'Later...

OK, so let's do this: fix in seq_read_iter() + in do_loop_readv_writev()
(on entry) the following (racy as hell, but will do for debugging):

	bool weird = false;

	if (unlikely(memcmp(file->f_path.dentry->d_name.name, "mountinfo", 10))) {
		int i;

		for (i = 0; i < iter->nr_segs; i++)
			if (!iter->iov[i].iov_len)
				weird = true;
		if (weird) {
			printk(KERN_ERR "[%s]: weird readv on %p4D (%ld) ",
				current->comm, filp, (long)filp->f_pos);
			for (i = 0; i < iter->nr_segs; i++)
				printk(KERN_CONT "%c%zd", i ? ':' : '<',
					iter->iov[i].iov_len);
			printk(KERN_CONT "> ");
		}
	}
and in the end (just before return)
	if (weird)
		printk(KERN_CONT "-> %zd\n", ret);

Preferably along with the results of cat /proc/<whatever it is>/mountinfo both
on that and on the working kernel...

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Sat, Nov 14, 2020 at 08:50:00PM +0000, Al Viro wrote:

OK, I think I understand what's going on.  Could you check if
reverting the variant in -next and applying the following instead
fixes what you are seeing?

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..35667112bbd1 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
 ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct seq_file *m = iocb->ki_filp->private_data;
-	size_t size = iov_iter_count(iter);
 	size_t copied = 0;
 	size_t n;
 	void *p;
@@ -208,16 +207,15 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	}
 	/* if not empty - flush it first */
 	if (m->count) {
-		n = min(m->count, size);
-		if (copy_to_iter(m->buf + m->from, n, iter) != n)
-			goto Efault;
+		n = copy_to_iter(m->buf + m->from, m->count, iter);
 		m->count -= n;
 		m->from += n;
-		size -= n;
 		copied += n;
-		if (!size)
-			goto Done;
+		if (m->count)
+			goto Efault;
 	}
+	if (!iov_iter_count(iter))
+		goto Done;
 	/* we need at least one record in buffer */
 	m->from = 0;
 	p = m->op->start(m, &m->index);
@@ -249,6 +247,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	goto Done;
 Fill:
 	/* they want more? let's try to get some more */
+	/* m->count is positive and there's space left in iter */
 	while (1) {
 		size_t offs = m->count;
 		loff_t pos = m->index;
@@ -263,7 +262,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 			err = PTR_ERR(p);
 			break;
 		}
-		if (m->count >= size)
+		if (m->count >= iov_iter_count(iter))
 			break;
 		err = m->op->show(m, p);
 		if (seq_has_overflowed(m) || err) {
@@ -273,12 +272,12 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		}
 	}
 	m->op->stop(m, p);
-	n = min(m->count, size);
-	if (copy_to_iter(m->buf, n, iter) != n)
-		goto Efault;
+	n = copy_to_iter(m->buf, m->count, iter);
 	copied += n;
-	m->count -= n;
 	m->from = n;
+	m->count -= n;
+	if (m->count)
+		goto Efault;
 Done:
 	if (!copied)
 		copied = err;

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Linus Torvalds]

On Sun, Nov 15, 2020 at 7:54 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> OK, I think I understand what's going on.  Could you check if
> reverting the variant in -next and applying the following instead
> fixes what you are seeing?

Side note: if this ends up working, can you add a lot of comments
about this thing (both in the code and the commit message)? It
confused both Christoph and me, and clearly you were stumped too.
That's not a great sign.

                  Linus

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Sun, Nov 15, 2020 at 02:41:25PM -0700, Nathan Chancellor wrote:
> Hi Al,
> 
> Apologies for the delay.
> 
> On Sun, Nov 15, 2020 at 03:53:55PM +0000, Al Viro wrote:
> > On Sat, Nov 14, 2020 at 08:50:00PM +0000, Al Viro wrote:
> > 
> > OK, I think I understand what's going on.  Could you check if
> > reverting the variant in -next and applying the following instead
> > fixes what you are seeing?
> 
> The below diff on top of d4d50710a8b46082224376ef119a4dbb75b25c56 does
> not fix my issue unfortunately.

OK...  Now that I have a reproducer[1], I think I've sorted it out.
And yes, it had been too subtle for its own good ;-/

[1] I still wonder what the hell in the userland has come up with the
idea of reading through a file with readv(), each time with 2-element
iovec array, the first element covering 0 bytes and the second one - 1024.
AFAICS, nothing is systemd git appears to be _that_ weird...  Makes for
a useful testcase, though...

Anyway, could you test this replacement?

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..c0dfe2861b35 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,12 +168,14 @@ EXPORT_SYMBOL(seq_read);
 ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct seq_file *m = iocb->ki_filp->private_data;
-	size_t size = iov_iter_count(iter);
 	size_t copied = 0;
 	size_t n;
 	void *p;
 	int err = 0;
 
+	if (!iov_iter_count(iter))
+		return 0;
+
 	mutex_lock(&m->lock);
 
 	/*
@@ -208,34 +210,32 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	}
 	/* if not empty - flush it first */
 	if (m->count) {
-		n = min(m->count, size);
-		if (copy_to_iter(m->buf + m->from, n, iter) != n)
-			goto Efault;
+		n = copy_to_iter(m->buf + m->from, m->count, iter);
 		m->count -= n;
 		m->from += n;
-		size -= n;
 		copied += n;
-		if (!size)
+		if (m->count)	// hadn't managed to copy everything
 			goto Done;
 	}
-	/* we need at least one record in buffer */
+	/* we need at least one non-empty record in the buffer */
 	m->from = 0;
 	p = m->op->start(m, &m->index);
 	while (1) {
 		err = PTR_ERR(p);
-		if (!p || IS_ERR(p))
+		if (!p || IS_ERR(p))	// EOF or an error
 			break;
 		err = m->op->show(m, p);
-		if (err < 0)
+		if (err < 0)		// hard error
 			break;
-		if (unlikely(err))
+		if (unlikely(err))	// ->show() says "skip it"
 			m->count = 0;
-		if (unlikely(!m->count)) {
+		if (unlikely(!m->count)) { // empty record
 			p = m->op->next(m, p, &m->index);
 			continue;
 		}
-		if (m->count < m->size)
+		if (!seq_has_overflowed(m)) // got it
 			goto Fill;
+		// need a bigger buffer
 		m->op->stop(m, p);
 		kvfree(m->buf);
 		m->count = 0;
@@ -244,11 +244,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 			goto Enomem;
 		p = m->op->start(m, &m->index);
 	}
+	// EOF or an error
 	m->op->stop(m, p);
 	m->count = 0;
 	goto Done;
 Fill:
-	/* they want more? let's try to get some more */
+	// one non-empty record is in the buffer; if they want more,
+	// try to fit more in, but in any case we need to advance
+	// the iterator at least once.
 	while (1) {
 		size_t offs = m->count;
 		loff_t pos = m->index;
@@ -259,11 +262,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 					    m->op->next);
 			m->index++;
 		}
-		if (!p || IS_ERR(p)) {
-			err = PTR_ERR(p);
+		if (!p || IS_ERR(p))	// no next record for us
 			break;
-		}
-		if (m->count >= size)
+		if (m->count >= iov_iter_count(iter))
 			break;
 		err = m->op->show(m, p);
 		if (seq_has_overflowed(m) || err) {
@@ -273,16 +274,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		}
 	}
 	m->op->stop(m, p);
-	n = min(m->count, size);
-	if (copy_to_iter(m->buf, n, iter) != n)
-		goto Efault;
+	n = copy_to_iter(m->buf, m->count, iter);
 	copied += n;
 	m->count -= n;
 	m->from = n;
 Done:
-	if (!copied)
-		copied = err;
-	else {
+	if (unlikely(!copied)) {
+		copied = m->count ? -EFAULT : err;
+	} else {
 		iocb->ki_pos += copied;
 		m->read_pos += copied;
 	}
@@ -291,9 +290,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 Enomem:
 	err = -ENOMEM;
 	goto Done;
-Efault:
-	err = -EFAULT;
-	goto Done;
 }
 EXPORT_SYMBOL(seq_read_iter);
 

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Nathan Chancellor]

On Sun, Nov 15, 2020 at 11:38:14PM +0000, Al Viro wrote:
> On Sun, Nov 15, 2020 at 02:41:25PM -0700, Nathan Chancellor wrote:
> > Hi Al,
> > 
> > Apologies for the delay.
> > 
> > On Sun, Nov 15, 2020 at 03:53:55PM +0000, Al Viro wrote:
> > > On Sat, Nov 14, 2020 at 08:50:00PM +0000, Al Viro wrote:
> > > 
> > > OK, I think I understand what's going on.  Could you check if
> > > reverting the variant in -next and applying the following instead
> > > fixes what you are seeing?
> > 
> > The below diff on top of d4d50710a8b46082224376ef119a4dbb75b25c56 does
> > not fix my issue unfortunately.
> 
> OK...  Now that I have a reproducer[1], I think I've sorted it out.
> And yes, it had been too subtle for its own good ;-/
> 
> [1] I still wonder what the hell in the userland has come up with the
> idea of reading through a file with readv(), each time with 2-element
> iovec array, the first element covering 0 bytes and the second one - 1024.
> AFAICS, nothing is systemd git appears to be _that_ weird...  Makes for
> a useful testcase, though...
> 
> Anyway, could you test this replacement?

Looks good to me on top of d4d50710a8b46082224376ef119a4dbb75b25c56,
thanks for quickly looking into this!

Tested-by: Nathan Chancellor <natechancellor@gmail.com>

> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3b20e21604e7..c0dfe2861b35 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -168,12 +168,14 @@ EXPORT_SYMBOL(seq_read);
>  ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  {
>  	struct seq_file *m = iocb->ki_filp->private_data;
> -	size_t size = iov_iter_count(iter);
>  	size_t copied = 0;
>  	size_t n;
>  	void *p;
>  	int err = 0;
>  
> +	if (!iov_iter_count(iter))
> +		return 0;
> +
>  	mutex_lock(&m->lock);
>  
>  	/*
> @@ -208,34 +210,32 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  	}
>  	/* if not empty - flush it first */
>  	if (m->count) {
> -		n = min(m->count, size);
> -		if (copy_to_iter(m->buf + m->from, n, iter) != n)
> -			goto Efault;
> +		n = copy_to_iter(m->buf + m->from, m->count, iter);
>  		m->count -= n;
>  		m->from += n;
> -		size -= n;
>  		copied += n;
> -		if (!size)
> +		if (m->count)	// hadn't managed to copy everything
>  			goto Done;
>  	}
> -	/* we need at least one record in buffer */
> +	/* we need at least one non-empty record in the buffer */
>  	m->from = 0;
>  	p = m->op->start(m, &m->index);
>  	while (1) {
>  		err = PTR_ERR(p);
> -		if (!p || IS_ERR(p))
> +		if (!p || IS_ERR(p))	// EOF or an error
>  			break;
>  		err = m->op->show(m, p);
> -		if (err < 0)
> +		if (err < 0)		// hard error
>  			break;
> -		if (unlikely(err))
> +		if (unlikely(err))	// ->show() says "skip it"
>  			m->count = 0;
> -		if (unlikely(!m->count)) {
> +		if (unlikely(!m->count)) { // empty record
>  			p = m->op->next(m, p, &m->index);
>  			continue;
>  		}
> -		if (m->count < m->size)
> +		if (!seq_has_overflowed(m)) // got it
>  			goto Fill;
> +		// need a bigger buffer
>  		m->op->stop(m, p);
>  		kvfree(m->buf);
>  		m->count = 0;
> @@ -244,11 +244,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  			goto Enomem;
>  		p = m->op->start(m, &m->index);
>  	}
> +	// EOF or an error
>  	m->op->stop(m, p);
>  	m->count = 0;
>  	goto Done;
>  Fill:
> -	/* they want more? let's try to get some more */
> +	// one non-empty record is in the buffer; if they want more,
> +	// try to fit more in, but in any case we need to advance
> +	// the iterator at least once.
>  	while (1) {
>  		size_t offs = m->count;
>  		loff_t pos = m->index;
> @@ -259,11 +262,9 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  					    m->op->next);
>  			m->index++;
>  		}
> -		if (!p || IS_ERR(p)) {
> -			err = PTR_ERR(p);
> +		if (!p || IS_ERR(p))	// no next record for us
>  			break;
> -		}
> -		if (m->count >= size)
> +		if (m->count >= iov_iter_count(iter))
>  			break;
>  		err = m->op->show(m, p);
>  		if (seq_has_overflowed(m) || err) {
> @@ -273,16 +274,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		}
>  	}
>  	m->op->stop(m, p);
> -	n = min(m->count, size);
> -	if (copy_to_iter(m->buf, n, iter) != n)
> -		goto Efault;
> +	n = copy_to_iter(m->buf, m->count, iter);
>  	copied += n;
>  	m->count -= n;
>  	m->from = n;
>  Done:
> -	if (!copied)
> -		copied = err;
> -	else {
> +	if (unlikely(!copied)) {
> +		copied = m->count ? -EFAULT : err;
> +	} else {
>  		iocb->ki_pos += copied;
>  		m->read_pos += copied;
>  	}
> @@ -291,9 +290,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  Enomem:
>  	err = -ENOMEM;
>  	goto Done;
> -Efault:
> -	err = -EFAULT;
> -	goto Done;
>  }
>  EXPORT_SYMBOL(seq_read_iter);
>  

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Sun, Nov 15, 2020 at 04:51:49PM -0700, Nathan Chancellor wrote:
> Looks good to me on top of d4d50710a8b46082224376ef119a4dbb75b25c56,
> thanks for quickly looking into this!
> 
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>

OK... a variant with (hopefully) better comments and cleaned up
logics in the second loop (
                if (seq_has_overflowed(m) || err) {
                        m->count = offs;
                        if (likely(err <= 0))
                                break;
                }
replaced with
                if (err > 0) {          // ->show() says "skip it"
                        m->count = offs;
                } else if (err || seq_has_overflowed(m)) {
                        m->count = offs;
                        break;
                }
) follows.  I'm quite certain that it is an equivalent transformation
(seq_has_overflowed() has no side effects) and IMO it's slightly
more readable that way.  Survives local beating; could you check if
it's still OK with your testcase?  Equivalent transformation or not,
I'd rather not slap anyone's Tested-by: on a modified variant of
patch...

BTW, is that call of readv() really coming from init?  And if it
is, what version of init are you using?

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3b20e21604e7..03a369ccd28c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -168,12 +168,14 @@ EXPORT_SYMBOL(seq_read);
 ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct seq_file *m = iocb->ki_filp->private_data;
-	size_t size = iov_iter_count(iter);
 	size_t copied = 0;
 	size_t n;
 	void *p;
 	int err = 0;
 
+	if (!iov_iter_count(iter))
+		return 0;
+
 	mutex_lock(&m->lock);
 
 	/*
@@ -206,36 +208,34 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		if (!m->buf)
 			goto Enomem;
 	}
-	/* if not empty - flush it first */
+	// something left in the buffer - copy it out first
 	if (m->count) {
-		n = min(m->count, size);
-		if (copy_to_iter(m->buf + m->from, n, iter) != n)
-			goto Efault;
+		n = copy_to_iter(m->buf + m->from, m->count, iter);
 		m->count -= n;
 		m->from += n;
-		size -= n;
 		copied += n;
-		if (!size)
+		if (m->count)	// hadn't managed to copy everything
 			goto Done;
 	}
-	/* we need at least one record in buffer */
+	// get a non-empty record in the buffer
 	m->from = 0;
 	p = m->op->start(m, &m->index);
 	while (1) {
 		err = PTR_ERR(p);
-		if (!p || IS_ERR(p))
+		if (!p || IS_ERR(p))	// EOF or an error
 			break;
 		err = m->op->show(m, p);
-		if (err < 0)
+		if (err < 0)		// hard error
 			break;
-		if (unlikely(err))
+		if (unlikely(err))	// ->show() says "skip it"
 			m->count = 0;
-		if (unlikely(!m->count)) {
+		if (unlikely(!m->count)) { // empty record
 			p = m->op->next(m, p, &m->index);
 			continue;
 		}
-		if (m->count < m->size)
+		if (!seq_has_overflowed(m)) // got it
 			goto Fill;
+		// need a bigger buffer
 		m->op->stop(m, p);
 		kvfree(m->buf);
 		m->count = 0;
@@ -244,11 +244,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 			goto Enomem;
 		p = m->op->start(m, &m->index);
 	}
+	// EOF or an error
 	m->op->stop(m, p);
 	m->count = 0;
 	goto Done;
 Fill:
-	/* they want more? let's try to get some more */
+	// one non-empty record is in the buffer; if they want more,
+	// try to fit more in, but in any case we need to advance
+	// the iterator once for every record shown.
 	while (1) {
 		size_t offs = m->count;
 		loff_t pos = m->index;
@@ -259,30 +262,27 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 					    m->op->next);
 			m->index++;
 		}
-		if (!p || IS_ERR(p)) {
-			err = PTR_ERR(p);
+		if (!p || IS_ERR(p))	// no next record for us
 			break;
-		}
-		if (m->count >= size)
+		if (m->count >= iov_iter_count(iter))
 			break;
 		err = m->op->show(m, p);
-		if (seq_has_overflowed(m) || err) {
+		if (err > 0) {		// ->show() says "skip it"
 			m->count = offs;
-			if (likely(err <= 0))
-				break;
+		} else if (err || seq_has_overflowed(m)) {
+			m->count = offs;
+			break;
 		}
 	}
 	m->op->stop(m, p);
-	n = min(m->count, size);
-	if (copy_to_iter(m->buf, n, iter) != n)
-		goto Efault;
+	n = copy_to_iter(m->buf, m->count, iter);
 	copied += n;
 	m->count -= n;
 	m->from = n;
 Done:
-	if (!copied)
-		copied = err;
-	else {
+	if (unlikely(!copied)) {
+		copied = m->count ? -EFAULT : err;
+	} else {
 		iocb->ki_pos += copied;
 		m->read_pos += copied;
 	}
@@ -291,9 +291,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 Enomem:
 	err = -ENOMEM;
 	goto Done;
-Efault:
-	err = -EFAULT;
-	goto Done;
 }
 EXPORT_SYMBOL(seq_read_iter);
 

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Nathan Chancellor]

On Mon, Nov 16, 2020 at 12:25:13AM +0000, Al Viro wrote:
> On Sun, Nov 15, 2020 at 04:51:49PM -0700, Nathan Chancellor wrote:
> > Looks good to me on top of d4d50710a8b46082224376ef119a4dbb75b25c56,
> > thanks for quickly looking into this!
> > 
> > Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> 
> OK... a variant with (hopefully) better comments and cleaned up
> logics in the second loop (
>                 if (seq_has_overflowed(m) || err) {
>                         m->count = offs;
>                         if (likely(err <= 0))
>                                 break;
>                 }
> replaced with
>                 if (err > 0) {          // ->show() says "skip it"
>                         m->count = offs;
>                 } else if (err || seq_has_overflowed(m)) {
>                         m->count = offs;
>                         break;
>                 }
> ) follows.  I'm quite certain that it is an equivalent transformation
> (seq_has_overflowed() has no side effects) and IMO it's slightly
> more readable that way.  Survives local beating; could you check if
> it's still OK with your testcase?  Equivalent transformation or not,
> I'd rather not slap anyone's Tested-by: on a modified variant of
> patch...

Still good.

Tested-by: Nathan Chancellor <natechancellor@gmail.com>

> BTW, is that call of readv() really coming from init?  And if it
> is, what version of init are you using?

I believe that it is but since this is WSL2, I believe that /init is a
proprietary Microsoft implementation, rather than systemd or another
init system:

https://wiki.ubuntu.com/WSL#Keeping_Ubuntu_Updated_in_WSL

So I am not sure how possible it is to see exactly what is going on or
getting it improved.

Cheers,
Nathan

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Sun, Nov 15, 2020 at 05:34:16PM -0700, Nathan Chancellor wrote:
 
> Still good.
> 
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>

Pushed into #fixes

> > BTW, is that call of readv() really coming from init?  And if it
> > is, what version of init are you using?
> 
> I believe that it is but since this is WSL2, I believe that /init is a
> proprietary Microsoft implementation, rather than systemd or another
> init system:
> 
> https://wiki.ubuntu.com/WSL#Keeping_Ubuntu_Updated_in_WSL
> 
> So I am not sure how possible it is to see exactly what is going on or
> getting it improved.

Oh, well...  Anyway, as a regression test it's interesting:

#include <sys/uio.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>
main()
{
	static char s[1024];
	static struct iovec v[2] = {{NULL, 0}, {s, 1024}};

	for(;;) {
		ssize_t n = readv(0, v, 2), m, w;

		if (n < 0) {
			perror("readv");
			return -1;
		}
		if (!n)
			return 0;
		for (m = 0; m < n; m += w) {
			w = write(1, s + m, n - m);
			if (w < 0)
				perror("write");
		}
	}
}

which ought to copy stdin to stdout; with this bug it would (on sufficiently
large seq_file-based files) fail with "readv: Bad address" (-EFAULT, that is).

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Christoph Hellwig]

On Mon, Nov 16, 2020 at 03:29:42AM +0000, Al Viro wrote:
> > Still good.
> > 
> > Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> 
> Pushed into #fixes

Shouldn't this go to Linus before v5.10 is released?

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Christoph Hellwig]

On Fri, Nov 27, 2020 at 05:29:02PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 16, 2020 at 03:29:42AM +0000, Al Viro wrote:
> > > Still good.
> > > 
> > > Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> > 
> > Pushed into #fixes
> 
> Shouldn't this go to Linus before v5.10 is released?

ping?

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Linus Torvalds]

On Tue, Dec 8, 2020 at 8:35 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > Shouldn't this go to Linus before v5.10 is released?
>
> ping?

So by now I'm a bit worried about this series, because the early fixes
caused more problems than the current state.

So considering the timing and Al having been spotty, I think this is
post-5.10 and marked for stable.

Al, I'm willing to be convinced otherwise, but you need to respond..

              Linus

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Tue, Dec 08, 2020 at 10:34:45AM -0800, Linus Torvalds wrote:
> On Tue, Dec 8, 2020 at 8:35 AM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > Shouldn't this go to Linus before v5.10 is released?
> >
> > ping?
> 
> So by now I'm a bit worried about this series, because the early fixes
> caused more problems than the current state.

*nod*

Said that, it does appear to survive all beating, and it does fix
a regression introduced in this cycle, so, provided that amount of
comments in there is OK with you...

The following changes since commit d4d50710a8b46082224376ef119a4dbb75b25c56:

  seq_file: add seq_read_iter (2020-11-06 10:05:18 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git fixes

for you to fetch changes up to 4bbf439b09c5ac3f8b3e9584fe080375d8d0ad2d:

  fix return values of seq_read_iter() (2020-11-15 22:12:53 -0500)

----------------------------------------------------------------
Al Viro (1):
      fix return values of seq_read_iter()

 fs/seq_file.c | 57 +++++++++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Linus Torvalds]

On Tue, Dec 8, 2020 at 11:49 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Said that, it does appear to survive all beating, and it does fix
> a regression introduced in this cycle, so, provided that amount of
> comments in there is OK with you...

Ok, considering Greg's note, I've pulled it. It's early in the last
week, if something comes up we can still fix it.

That said, considering that I think the only use-case was that odd
/proc splice use, and the really special WSL2 thing, and both of those
are verified, it does sound safe to pull.

Famous last words...

Al, since you're around, would you mind looking at the two
DCACHE_DONTCACHE patches too? Honestly, since they seem to be an issue
only for DAX, and only for DAX policy changes, I don't consider them
critical for 5.10, but they've been around for a while now.

         Linus

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Tue, Dec 08, 2020 at 12:25:55PM -0800, Linus Torvalds wrote:
> On Tue, Dec 8, 2020 at 11:49 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Said that, it does appear to survive all beating, and it does fix
> > a regression introduced in this cycle, so, provided that amount of
> > comments in there is OK with you...
> 
> Ok, considering Greg's note, I've pulled it. It's early in the last
> week, if something comes up we can still fix it.
> 
> That said, considering that I think the only use-case was that odd
> /proc splice use, and the really special WSL2 thing, and both of those
> are verified, it does sound safe to pull.
> 
> Famous last words...
> 
> Al, since you're around, would you mind looking at the two
> DCACHE_DONTCACHE patches too? Honestly, since they seem to be an issue
> only for DAX, and only for DAX policy changes, I don't consider them
> critical for 5.10, but they've been around for a while now.

Umm...  I've got
fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode()
and
fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set
in "to apply" pile; if that's what you are talking about, I don't
think they are anywhere critical enough for 5.10-final, but I might
be missing something...

Al, still buried under piles of email ;-/

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Linus Torvalds]

On Tue, Dec 8, 2020 at 12:53 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Umm...  I've got
> fs: Handle I_DONTCACHE in iput_final() instead of generic_drop_inode()
> and
> fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set
> in "to apply" pile; if that's what you are talking about,

Yup, those were the ones.

> I don't
> think they are anywhere critical enough for 5.10-final, but I might
> be missing something...

No, I agree, no hurry with them. I just wanted to make sure you're
aware of them.

                 Linus

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Greg KH]

On Tue, Dec 08, 2020 at 10:34:45AM -0800, Linus Torvalds wrote:
> On Tue, Dec 8, 2020 at 8:35 AM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > Shouldn't this go to Linus before v5.10 is released?
> >
> > ping?
> 
> So by now I'm a bit worried about this series, because the early fixes
> caused more problems than the current state.
> 
> So considering the timing and Al having been spotty, I think this is
> post-5.10 and marked for stable.

If you want some sort of "do these really work" validation, these have
been running for a while now in the android 5.10-rc kernels just fine,
as I cherry-picked the patches there to get past their testing issues.

But if you want to wait until after 5.10 is out, that's fine with me
too, it's up to Al.

thanks,

greg k-h

Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro]

On Fri, Nov 13, 2020 at 04:54:53PM -0700, Nathan Chancellor wrote:
> Hi Al,
> 
> On Wed, Nov 11, 2020 at 10:21:16PM +0000, Al Viro wrote:
> > On Wed, Nov 11, 2020 at 09:52:20PM +0000, Al Viro wrote:
> > 
> > > That can be done, but I would rather go with
> > > 		n = copy_to_iter(m->buf + m->from, m->count, iter);
> > > 		m->count -= n;
> > > 		m->from += n;
> > >                 copied += n;
> > >                 if (!size)
> > >                         goto Done;
> > > 		if (m->count)
> > > 			goto Efault;
> > > if we do it that way.  Let me see if I can cook something
> > > reasonable along those lines...
> > 
> > Something like below (build-tested only):
> > 
> > diff --git a/fs/seq_file.c b/fs/seq_file.c
> > index 3b20e21604e7..07b33c1f34a9 100644
> > --- a/fs/seq_file.c
> > +++ b/fs/seq_file.c
> > @@ -168,7 +168,6 @@ EXPORT_SYMBOL(seq_read);
> >  ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >  {
> >  	struct seq_file *m = iocb->ki_filp->private_data;
> > -	size_t size = iov_iter_count(iter);
> >  	size_t copied = 0;
> >  	size_t n;
> >  	void *p;
> > @@ -208,14 +207,11 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >  	}
> >  	/* if not empty - flush it first */
> >  	if (m->count) {
> > -		n = min(m->count, size);
> > -		if (copy_to_iter(m->buf + m->from, n, iter) != n)
> > -			goto Efault;
> > +		n = copy_to_iter(m->buf + m->from, m->count, iter);
> >  		m->count -= n;
> >  		m->from += n;
> > -		size -= n;
> >  		copied += n;
> > -		if (!size)
> > +		if (!iov_iter_count(iter) || m->count)
> >  			goto Done;
> >  	}
> >  	/* we need at least one record in buffer */
> > @@ -249,6 +245,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >  	goto Done;
> >  Fill:
> >  	/* they want more? let's try to get some more */
> > +	/* m->count is positive and there's space left in iter */
> >  	while (1) {
> >  		size_t offs = m->count;
> >  		loff_t pos = m->index;
> > @@ -263,7 +260,7 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >  			err = PTR_ERR(p);
> >  			break;
> >  		}
> > -		if (m->count >= size)
> > +		if (m->count >= iov_iter_count(iter))
> >  			break;
> >  		err = m->op->show(m, p);
> >  		if (seq_has_overflowed(m) || err) {
> > @@ -273,16 +270,14 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >  		}
> >  	}
> >  	m->op->stop(m, p);
> > -	n = min(m->count, size);
> > -	if (copy_to_iter(m->buf, n, iter) != n)
> > -		goto Efault;
> > +	n = copy_to_iter(m->buf, m->count, iter);
> >  	copied += n;
> >  	m->count -= n;
> >  	m->from = n;
> >  Done:
> > -	if (!copied)
> > -		copied = err;
> > -	else {
> > +	if (unlikely(!copied)) {
> > +		copied = m->count ? -EFAULT : err;
> > +	} else {
> >  		iocb->ki_pos += copied;
> >  		m->read_pos += copied;
> >  	}
> > @@ -291,9 +286,6 @@ ssize_t seq_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >  Enomem:
> >  	err = -ENOMEM;
> >  	goto Done;
> > -Efault:
> > -	err = -EFAULT;
> > -	goto Done;
> >  }
> >  EXPORT_SYMBOL(seq_read_iter);
> >  
> 
> This patch in -next (6a9f696d1627bacc91d1cebcfb177f474484e8ba) breaks
> WSL2's interoperability feature, where Windows paths automatically get
> added to PATH on start up so that Windows binaries can be accessed from
> within Linux (such as clip.exe to pipe output to the clipboard). Before,
> I would see a bunch of Linux + Windows folders in $PATH but after, I
> only see the Linux folders (I can give you the actual PATH value if you
> care but it is really long).
> 
> I am not at all familiar with the semantics of this patch or how
> Microsoft would be using it to inject folders into PATH (they have some
> documentation on it here:
> https://docs.microsoft.com/en-us/windows/wsl/interop) and I am not sure
> how to go about figuring that out to see why this patch breaks something
> (unless you have an idea). I have added the Hyper-V maintainers and list
> to CC in case they know someone who could help.

FWIW, just to make sure:
	1) does reverting just that commit recover the desired behaviour?
	2) could you verify that your latest tests had been done with
the incremental I'd posted (shifting the if (....) goto Done; out of the if
body)?
	3) does the build with that commit reverted produce any warnings
related to mountinfo?
	4) your posted log with WARN_ON unfortunately starts *after*
the mountinfo accesses; could you check which process had been doing those?
The Comm: ... part in there, that is.
	5) in the "I don't believe that could happen, but let's make sure"
department: turn the
        /* m->count is positive and there's space left in iter */
comment in seq_read_iter() into an outright
	BUG_ON(!m->count || !iov_iter_count(iter));