[PATCH v2] xfs: fix unaligned access to rmapbt high keys in scrub

[PATCH v2] xfs: fix unaligned access to rmapbt high keys in scrub

[Stian Halseth]

An rmapbt node stores each key as a low/high pair of packed 20-byte
struct xfs_rmap_key, so every high key is only 4-byte aligned.
xchk_rmapbt_check_unwritten_in_keyflags() reads rm_offset through
union xfs_btree_key, which is 8-byte aligned, so the compiler may use
an 8-byte load. On sparc64 every high key it checks then takes an
unaligned access trap:

  Kernel unaligned access at TPC[105a54d8] xchk_rmapbt_check_unwritten_in_keyflags+0xb8/0x160 [xfs]

Read both keys through struct xfs_rmap_key and convert rm_offset with
be64_to_cpu(), like the other rmapbt key accessors, instead of
comparing against a byte-swapped flag.

Fixes: 38384569a2a8 ("xfs: detect unwritten bit set in rmapbt node block keys")
Reported-by: Jan Engelhardt <jengelh@inai.de>
Closes: https://github.com/sparclinux/issues/issues/115
Cc: stable@vger.kernel.org # v6.4+
Signed-off-by: Stian Halseth <stian@itx.no>
---
Tested on sparc64 (T4, 7.3.3) by scrubbing the rmapbt of a loop-mounted
image: alignment faults per scrub went from about 4000 to 0.

v2: read both keys through struct xfs_rmap_key with be64_to_cpu() and drop
the byte-swapped badflag (Dave). be64_to_cpu() on the union alone still
traps, as it is a no-op on big-endian.
v1: https://lore.kernel.org/all/20261109134853.1659991-1-stian@itx.no/

 fs/xfs/scrub/rmap.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/scrub/rmap.c b/fs/xfs/scrub/rmap.c
index 68e2847c962bb..29609c5967fe3 100644
--- a/fs/xfs/scrub/rmap.c
+++ b/fs/xfs/scrub/rmap.c
@@ -149,8 +149,7 @@ xchk_rmapbt_check_unwritten_in_keyflags(
 	struct xfs_scrub	*sc = bs->sc;
 	struct xfs_btree_cur	*cur = bs->cur;
 	struct xfs_btree_block	*keyblock;
-	union xfs_btree_key	*lkey, *hkey;
-	__be64			badflag = cpu_to_be64(XFS_RMAP_OFF_UNWRITTEN);
+	const struct xfs_rmap_key *lkey, *hkey;
 	unsigned int		level;
 
 	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_PREEN)
@@ -166,15 +165,15 @@ xchk_rmapbt_check_unwritten_in_keyflags(
 
 		keyblock = xfs_btree_get_block(cur, level, &bp);
 		for (ptr = 1; ptr <= be16_to_cpu(keyblock->bb_numrecs); ptr++) {
-			lkey = xfs_btree_key_addr(cur, ptr, keyblock);
+			lkey = &xfs_btree_key_addr(cur, ptr, keyblock)->rmap;
 
-			if (lkey->rmap.rm_offset & badflag) {
+			if (be64_to_cpu(lkey->rm_offset) & XFS_RMAP_OFF_UNWRITTEN) {
 				xchk_btree_set_preen(sc, cur, level);
 				break;
 			}
 
-			hkey = xfs_btree_high_key_addr(cur, ptr, keyblock);
-			if (hkey->rmap.rm_offset & badflag) {
+			hkey = &xfs_btree_high_key_addr(cur, ptr, keyblock)->rmap;
+			if (be64_to_cpu(hkey->rm_offset) & XFS_RMAP_OFF_UNWRITTEN) {
 				xchk_btree_set_preen(sc, cur, level);
 				break;
 			}
-- 
2.43.0

Re: [PATCH v2] xfs: fix unaligned access to rmapbt high keys in scrub

[Dave Chinner]

On Tue, Nov 10, 2026 at 11:39:27PM +0100, Stian Halseth wrote:
> An rmapbt node stores each key as a low/high pair of packed 20-byte
> struct xfs_rmap_key, so every high key is only 4-byte aligned.
> xchk_rmapbt_check_unwritten_in_keyflags() reads rm_offset through
> union xfs_btree_key, which is 8-byte aligned, so the compiler may use
> an 8-byte load.

Ok, this trips my 'written by an LLM' filter. i.e. long
run-on sentences, strung together with commas and "so ...". I
mention this because....

> diff --git a/fs/xfs/scrub/rmap.c b/fs/xfs/scrub/rmap.c
> index 68e2847c962bb..29609c5967fe3 100644
> --- a/fs/xfs/scrub/rmap.c
> +++ b/fs/xfs/scrub/rmap.c
> @@ -149,8 +149,7 @@ xchk_rmapbt_check_unwritten_in_keyflags(
>  	struct xfs_scrub	*sc = bs->sc;
>  	struct xfs_btree_cur	*cur = bs->cur;
>  	struct xfs_btree_block	*keyblock;
> -	union xfs_btree_key	*lkey, *hkey;
> -	__be64			badflag = cpu_to_be64(XFS_RMAP_OFF_UNWRITTEN);
> +	const struct xfs_rmap_key *lkey, *hkey;
>  	unsigned int		level;
>  
>  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_PREEN)
> @@ -166,15 +165,15 @@ xchk_rmapbt_check_unwritten_in_keyflags(
>  
>  		keyblock = xfs_btree_get_block(cur, level, &bp);
>  		for (ptr = 1; ptr <= be16_to_cpu(keyblock->bb_numrecs); ptr++) {
> -			lkey = xfs_btree_key_addr(cur, ptr, keyblock);
> +			lkey = &xfs_btree_key_addr(cur, ptr, keyblock)->rmap;

The code fix smells of LLM, too: technically correct, but unreadable
and unmaintainable.

Regardless of where it came from, it's objectively -poor code-
because:

- it uses a pattern that is not found anywhere else in the XFS code.
- it costs nothing to use two variables here which would make the
  code much more readable for a maintainer.
- no comment to explain the issue the code has to avoid.

At minimum, a comment explaining why the code needs to be structured
this way is needed. Humans like me are going to forget why the code
needed to be structured this way in short order. Hence a comment is
needed to remind me why the code was written this way next time I
look at it.

IOWs, the code should look something like this:

			/*
			 * The on-disk high key in the rmap block may not be naturally
			 * aligned, so we have to tell the compiler that we are
			 * decoding rmap keys so it treats them correctly as an
			 * unaligned packed structure.
			 */
			union xfs_btree_key *key;
			const struct xfs_rmap_key *rkey;

			key = xfs_btree_key_addr(cur, ptr, keyblock);
			rkey = &key->rmap;
			if (be64_to_cpu(rkey->rm_offset) & XFS_RMAP_OFF_UNWRITTEN) {
				xchk_btree_set_preen(sc, cur, level);
				break;
			}

			key = xfs_btree_high_key_addr(cur, ptr, keyblock);
			rkey = &key->rmap;
			if (be64_to_cpu(rkey->rm_offset) & XFS_RMAP_OFF_UNWRITTEN) {
				xchk_btree_set_preen(sc, cur, level);
				break;
			}


-Dave.


-- 
Dave Chinner
dgc@kernel.org

Re: [PATCH v2] xfs: fix unaligned access to rmapbt high keys in scrub

[Stian Halseth]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

Hi Dave,

On Wed, 2026-11-11 at 16:45 +1100, Dave Chinner wrote:
> On Tue, Nov 10, 2026 at 11:39:27PM +0100, Stian Halseth wrote:
> > An rmapbt node stores each key as a low/high pair of packed 20-byte
> > struct xfs_rmap_key, so every high key is only 4-byte aligned.
> > xchk_rmapbt_check_unwritten_in_keyflags() reads rm_offset through
> > union xfs_btree_key, which is 8-byte aligned, so the compiler may use
> > an 8-byte load.
> 
> Ok, this trips my 'written by an LLM' filter. i.e. long
> run-on sentences, strung together with commas and "so ...". I
> mention this because....

The changelog was drafted with LLM help, and I'll rewrite it for v3.

> 
> > diff --git a/fs/xfs/scrub/rmap.c b/fs/xfs/scrub/rmap.c
> > index 68e2847c962bb..29609c5967fe3 100644
> > --- a/fs/xfs/scrub/rmap.c
> > +++ b/fs/xfs/scrub/rmap.c
> > @@ -149,8 +149,7 @@ xchk_rmapbt_check_unwritten_in_keyflags(
> >  	struct xfs_scrub	*sc = bs->sc;
> >  	struct xfs_btree_cur	*cur = bs->cur;
> >  	struct xfs_btree_block	*keyblock;
> > -	union xfs_btree_key	*lkey, *hkey;
> > -	__be64			badflag =
> > cpu_to_be64(XFS_RMAP_OFF_UNWRITTEN);
> > +	const struct xfs_rmap_key *lkey, *hkey;
> >  	unsigned int		level;
> >  
> >  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_PREEN)
> > @@ -166,15 +165,15 @@ xchk_rmapbt_check_unwritten_in_keyflags(
> >  
> >  		keyblock = xfs_btree_get_block(cur, level, &bp);
> >  		for (ptr = 1; ptr <= be16_to_cpu(keyblock->bb_numrecs);
> > ptr++) {
> > -			lkey = xfs_btree_key_addr(cur, ptr, keyblock);
> > +			lkey = &xfs_btree_key_addr(cur, ptr, keyblock)-
> > >rmap;
> 
> The code fix smells of LLM, too: technically correct, but unreadable
> and unmaintainable.
> 
> Regardless of where it came from, it's objectively -poor code-
> because:
> 
> - it uses a pattern that is not found anywhere else in the XFS code.
> - it costs nothing to use two variables here which would make the
>   code much more readable for a maintainer.
> - no comment to explain the issue the code has to avoid.
> 
> At minimum, a comment explaining why the code needs to be structured
> this way is needed. Humans like me are going to forget why the code
> needed to be structured this way in short order. Hence a comment is
> needed to remind me why the code was written this way next time I
> look at it.

Thank you for the feedback, and no argument from me on those three points.

You're correct in your assumption that LLM assistance was used for the final
code. Earlier iterations were more readable, and I should have kept them.
I try to keep my patches small, and this time it came at the cost of
readability. I stand by the correctness and testing, but will send a v3
following your suggestion, which reads much better.

I take this feedback to heart, and it's truly useful for my personal
development. My goal is to learn and improve. I will pay closer attention
to the existing code patterns, and try to adhere more closely to them.
I'm very sceptical of the output I get from the LLM models, so I test
everything to the best of my abilities before posting.


/Stian

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]