linux-kernel - Re: [PATCH 1/6] seq_file: add seq_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 15 Nov 2020 16:51:49 -0700
From:   Nathan Chancellor <natechancellor@...il.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Christoph Hellwig <hch@....de>,
        Greg KH <gregkh@...uxfoundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kys@...rosoft.com, haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        wei.liu@...nel.org, linux-hyperv@...r.kernel.org
Subject: Re: [PATCH 1/6] seq_file: add seq_read_iter

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@...il.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);
>