Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Wed, 18 Oct 2023 16:30:06 -0700
From:   "Darrick J. Wong" <djwong@kernel.org>
To:     Jan Stancek <jstancek@redhat.com>
Cc:     willy@infradead.org, hch@lst.de, linux-xfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        viro@zeniv.linux.org.uk
Subject: Re: [PATCH v2] iomap: fix short copy in iomap_write_iter()
Message-ID: <20231018233006.GK3195650@frogsfrogsfrogs>
References: <8762e91a210f4cc5713fce05fe5906c18513bd0a.1697617238.git.jstancek@redhat.com>
 <e1cb4f8981f8c6e7e0384e95faf1911d9937e979.1697647960.git.jstancek@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e1cb4f8981f8c6e7e0384e95faf1911d9937e979.1697647960.git.jstancek@redhat.com>
Precedence: bulk

On Wed, Oct 18, 2023 at 08:32:32PM +0200, Jan Stancek wrote:
> Starting with commit 5d8edfb900d5 ("iomap: Copy larger chunks from
> userspace"), iomap_write_iter() can get into endless loop. This can
> be reproduced with LTP writev07 which uses partially valid iovecs:
>         struct iovec wr_iovec[] = {
>                 { buffer, 64 },
>                 { bad_addr, 64 },
>                 { buffer + 64, 64 },
>                 { buffer + 64 * 2, 64 },
>         };
> 
> commit bc1bb416bbb9 ("generic_perform_write()/iomap_write_actor():
> saner logics for short copy") previously introduced the logic, which
> made short copy retry in next iteration with amount of "bytes" it
> managed to copy:
> 
>                 if (unlikely(status == 0)) {
>                         /*
>                          * A short copy made iomap_write_end() reject the
>                          * thing entirely.  Might be memory poisoning
>                          * halfway through, might be a race with munmap,
>                          * might be severe memory pressure.
>                          */
>                         if (copied)
>                                 bytes = copied;
> 
> However, since 5d8edfb900d5 "bytes" is no longer carried into next
> iteration, because it is now always initialized at the beginning of
> the loop. And for iov_iter_count < PAGE_SIZE, "bytes" ends up with
> same value as previous iteration, making the loop retry same copy
> over and over, which leads to writev07 testcase hanging.
> 
> Make next iteration retry with amount of bytes we managed to copy.
> 
> Fixes: 5d8edfb900d5 ("iomap: Copy larger chunks from userspace")
> Signed-off-by: Jan Stancek <jstancek@redhat.com>

Looks fine to me, will send it out for testing...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> Changes in v2:
> - use goto instead of new variable (suggested by Christoph Hellwig)
> 
>  fs/iomap/buffered-io.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 5db54ca29a35..2bc0aa23fde3 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -881,8 +881,10 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  		size_t bytes;		/* Bytes to write to folio */
>  		size_t copied;		/* Bytes copied from user */
>  
> +		bytes = iov_iter_count(i);
> +retry:
>  		offset = pos & (chunk - 1);
> -		bytes = min(chunk - offset, iov_iter_count(i));
> +		bytes = min(chunk - offset, bytes);
>  		status = balance_dirty_pages_ratelimited_flags(mapping,
>  							       bdp_flags);
>  		if (unlikely(status))
> @@ -933,10 +935,12 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  			 * halfway through, might be a race with munmap,
>  			 * might be severe memory pressure.
>  			 */
> -			if (copied)
> -				bytes = copied;
>  			if (chunk > PAGE_SIZE)
>  				chunk /= 2;
> +			if (copied) {
> +				bytes = copied;
> +				goto retry;
> +			}
>  		} else {
>  			pos += status;
>  			written += status;
> -- 
> 2.31.1
>