[PATCH] netfs: Fix missing xas_retry() calls in xarray iteration

[PATCH] netfs: Fix missing xas_retry() calls in xarray iteration

[Anthony Brennan]

commit 59d0d52c30d4991ac4b329f049cc37118e00f5b0 upstream

Stops kernel from crashing when encountering an XAS retry entry. Patch modified
from upstream to work with pages instead of folios, and omits fixes to "dodgy
maths" as unrelated to fixing the crash.

Signed-off-by: Anthony Brennan <a2brenna@hatguy.io>
---
 fs/netfs/read_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/netfs/read_helper.c b/fs/netfs/read_helper.c
index 242f8bcb34a4..4de15555bceb 100644
--- a/fs/netfs/read_helper.c
+++ b/fs/netfs/read_helper.c
@@ -248,6 +248,9 @@ static void netfs_rreq_unmark_after_write(struct netfs_read_request *rreq,
 		XA_STATE(xas, &rreq->mapping->i_pages, subreq->start / PAGE_SIZE);
 
 		xas_for_each(&xas, page, (subreq->start + subreq->len - 1) / PAGE_SIZE) {
+			if(xas_retry(&xas, page))
+				continue;
+
 			/* We might have multiple writes from the same huge
 			 * page, but we mustn't unlock a page more than once.
 			 */
@@ -403,6 +406,9 @@ static void netfs_rreq_unlock(struct netfs_read_request *rreq)
 		unsigned int pgend = pgpos + thp_size(page);
 		bool pg_failed = false;
 
+		if(xas_retry(&xas, page))
+			continue;
+
 		for (;;) {
 			if (!subreq) {
 				pg_failed = true;
-- 
2.30.2

RE: [PATCH] netfs: Fix missing xas_retry() calls in xarray iteration

[David Laight]

From: Anthony Brennan
> Sent: 22 December 2023 00:15
> 
> commit 59d0d52c30d4991ac4b329f049cc37118e00f5b0 upstream
> 
> Stops kernel from crashing when encountering an XAS retry entry. Patch modified
> from upstream to work with pages instead of folios, and omits fixes to "dodgy
> maths" as unrelated to fixing the crash.
> 
> Signed-off-by: Anthony Brennan <a2brenna@hatguy.io>
> ---
>  fs/netfs/read_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/netfs/read_helper.c b/fs/netfs/read_helper.c
> index 242f8bcb34a4..4de15555bceb 100644
> --- a/fs/netfs/read_helper.c
> +++ b/fs/netfs/read_helper.c
> @@ -248,6 +248,9 @@ static void netfs_rreq_unmark_after_write(struct netfs_read_request *rreq,
>  		XA_STATE(xas, &rreq->mapping->i_pages, subreq->start / PAGE_SIZE);
> 
>  		xas_for_each(&xas, page, (subreq->start + subreq->len - 1) / PAGE_SIZE) {
> +			if(xas_retry(&xas, page))

'if' is not a function.

> +				continue;
> +
>  			/* We might have multiple writes from the same huge
>  			 * page, but we mustn't unlock a page more than once.
>  			 */
> @@ -403,6 +406,9 @@ static void netfs_rreq_unlock(struct netfs_read_request *rreq)
>  		unsigned int pgend = pgpos + thp_size(page);
>  		bool pg_failed = false;
> 
> +		if(xas_retry(&xas, page))
> +			continue;
> +
>  		for (;;) {
>  			if (!subreq) {
>  				pg_failed = true;
> --
> 2.30.2

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH] netfs: Fix missing xas_retry() calls in xarray iteration

[David Howells]

netfslib has a number of places in which it performs iteration of an xarray
whilst being under the RCU read lock.  It *should* call xas_retry() as the
first thing inside of the loop and do "continue" if it returns true in case
the xarray walker passed out a special value indicating that the walk needs
to be redone from the root[*].

Fix this by adding the missing retry checks.

[*] I wonder if this should be done inside xas_find(), xas_next_node() and
    suchlike, but I'm told that's not an simple change to effect.

This can cause an oops like that below.  Note the faulting address - this
is an internal value (|0x2) returned from xarray.

BUG: kernel NULL pointer dereference, address: 0000000000000402
...
RIP: 0010:netfs_rreq_unlock+0xef/0x380 [netfs]
...
Call Trace:
 netfs_rreq_assess+0xa6/0x240 [netfs]
 netfs_readpage+0x173/0x3b0 [netfs]
 ? init_wait_var_entry+0x50/0x50
 filemap_read_page+0x33/0xf0
 filemap_get_pages+0x2f2/0x3f0
 filemap_read+0xaa/0x320
 ? do_filp_open+0xb2/0x150
 ? rmqueue+0x3be/0xe10
 ceph_read_iter+0x1fe/0x680 [ceph]
 ? new_sync_read+0x115/0x1a0
 new_sync_read+0x115/0x1a0
 vfs_read+0xf3/0x180
 ksys_read+0x5f/0xe0
 do_syscall_64+0x38/0x90
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Fixes: 3d3c95046742 ("netfs: Provide readahead and readpage netfs helpers")
Reported-by: George Law <glaw@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
cc: Matthew Wilcox <willy@infradead.org>
cc: linux-cachefs@redhat.com
cc: linux-fsdevel@vger.kernel.org
---

 fs/netfs/buffered_read.c |    9 +++++++--
 fs/netfs/io.c            |    3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index 0ce535852151..baf668fb4315 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -46,10 +46,15 @@ void netfs_rreq_unlock_folios(struct netfs_io_request *rreq)
 
 	rcu_read_lock();
 	xas_for_each(&xas, folio, last_page) {
-		unsigned int pgpos = (folio_index(folio) - start_page) * PAGE_SIZE;
-		unsigned int pgend = pgpos + folio_size(folio);
+		unsigned int pgpos, pgend;
 		bool pg_failed = false;
 
+		if (xas_retry(&xas, folio))
+			continue;
+
+		pgpos = (folio_index(folio) - start_page) * PAGE_SIZE;
+		pgend = pgpos + folio_size(folio);
+
 		for (;;) {
 			if (!subreq) {
 				pg_failed = true;
diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index 428925899282..e374767d1b68 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -121,6 +121,9 @@ static void netfs_rreq_unmark_after_write(struct netfs_io_request *rreq,
 		XA_STATE(xas, &rreq->mapping->i_pages, subreq->start / PAGE_SIZE);
 
 		xas_for_each(&xas, folio, (subreq->start + subreq->len - 1) / PAGE_SIZE) {
+			if (xas_retry(&xas, folio))
+				continue;
+
 			/* We might have multiple writes from the same huge
 			 * folio, but we mustn't unlock a folio more than once.
 			 */

Re: [PATCH] netfs: Fix missing xas_retry() calls in xarray iteration

[Matthew Wilcox]

On Thu, Nov 03, 2022 at 09:33:28PM +0000, David Howells wrote:
> +++ b/fs/netfs/buffered_read.c
> @@ -46,10 +46,15 @@ void netfs_rreq_unlock_folios(struct netfs_io_request *rreq)
>  
>  	rcu_read_lock();
>  	xas_for_each(&xas, folio, last_page) {
> -		unsigned int pgpos = (folio_index(folio) - start_page) * PAGE_SIZE;
> -		unsigned int pgend = pgpos + folio_size(folio);
> +		unsigned int pgpos, pgend;

"unsigned int" assumes that the number of bytes isn't going to exceed 32
bits.  I tend to err on the side of safety here and use size_t.

>  		bool pg_failed = false;
>  
> +		if (xas_retry(&xas, folio))
> +			continue;
> +
> +		pgpos = (folio_index(folio) - start_page) * PAGE_SIZE;
> +		pgend = pgpos + folio_size(folio);

What happens if start_page is somewhere inside folio?  Seems to me
that pgend ends up overhanging into the next folio?

Re: [PATCH] netfs: Fix missing xas_retry() calls in xarray iteration

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> "unsigned int" assumes that the number of bytes isn't going to exceed 32
> bits.  I tend to err on the side of safety here and use size_t.

Not unreasonable.

> > +		pgpos = (folio_index(folio) - start_page) * PAGE_SIZE;
> > +		pgend = pgpos + folio_size(folio);
> 
> What happens if start_page is somewhere inside folio?  Seems to me
> that pgend ends up overhanging into the next folio?

Yeah, I think my maths is dodgy.  I should probably use folio_pos() and/or
offset_in_folio().

David