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

[Nathan Chancellor <natechancellor@gmail.com>]

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