Corrected edge case in uncompressed ARC->L2ARC handling #13375

rincebrain · 2022-04-24T21:05:20Z

Motivation and Context

Came up during #13244 after the recent rebase of 100 or so versions - I have no idea what's different now, since this codepath seems relatively unchanged for some time, but here we are.

I found the early abort code was panicking now where it hadn't before the rebase, and ASAN helpfully told me that it was reading off the edge of an allocated-512b buffer. Further debugging showed it had told zio_compress_data that the source (and since we calculate the dest size as a function of that, the dest) were 128k while doing so. Oops.

Description

Make this codepath always use the value for allocating that it then tells the compressor is contained within, then check against the actual payload size we think is the part we care about in there later.

(I am far from certain this is the intended outcome, since this inconsistency seems to predate even this function's creation, as it seems like a straightforward translation of an even earlier function before native encryption went in, but all the other permutations I thought might be arguably correct either caused panics, wild memory IO, or checksum/decryption errors (I think? We deleted the only way of counting those after the fact a little while ago, so I'm just speculating that it was encryption errors from it not showing up as rd/wr/cksum but still reporting the pool suspended...))

How Has This Been Tested?

The rebased #13244 not including this PR: causes all sorts of crashing when it runs the "turn off compressed ARC while using L2ARC" test cases.
The rebased #13244 with this PR: no crashing or errors reported.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

behlendorf · 2022-04-25T19:18:57Z

@gamanakis would you mind taking a look at this.

gamanakis · 2022-04-25T19:28:38Z

I found the early abort code was panicking now where it hadn't before the rebase, and ASAN helpfully told me that it was reading off the edge of an allocated-512b buffer.

Is this perhaps because compression is enabled by default now?

rincebrain · 2022-04-25T20:00:24Z

I found the early abort code was panicking now where it hadn't before the rebase, and ASAN helpfully told me that it was reading off the edge of an allocated-512b buffer.

Is this perhaps because compression is enabled by default now?

Maybe, I should experiment with that, but that's LZ4, and AFAICT the test always passes without the early abort PR, so I don't understand why that should break matters, particularly like this?

gamanakis

@rincebrain thank you for narrowing this down. I made some comments, I still need to take another look though.

gamanakis · 2022-04-28T11:52:42Z

module/zfs/arc.c

-		cabd = abd_alloc_for_io(asize, ismd);
-		tmp = abd_borrow_buf(cabd, asize);
+		cabd = abd_alloc_for_io(size, ismd);
+		tmp = abd_borrow_buf(cabd, size);

 		psize = zio_compress_data(compress, to_write, tmp, size,
 		    hdr->b_complevel);


This seems to be the zio_compress_data() that panic'd while trying to read off the edge of an allocated 512b buffer, right?
For that panic to occur we should have size > asize. Since we reached this point, then the buffer hasn't been compressed previously. So the only reason size > asize I can think of, is that the buffer is possibly encrypted.

If we allocate cabd, tmp with size, we should probably make sure that size is aligned to the L2ARC vdev, something like vdev_psize_to_asize(dev->l2ad_vdev, size). However we don't have the vdev information available here. Let me think about it.

That would be correct.

In particular, what I observed was it reporting size of 128k and asize of 512b.

Would you mind adding a comment saying that there are cases of size > asize (e.g encryption) and we have to allocate tmp and cabd with size instead of asize.

I have no problem with adding the comment that it can happen, but I'd want to have much more detailed examples if I'm going to include anything beyond "it can happen", I think - the last thing I'd like to do is suggest someone look in a certain part of the code for the cause and be incorrect.

What I see possibly happening is the following:

there is a compressed/encrypted buffer on-disk

arc compression is disabled, so when arc_hdr_decrypt() is called it allocates buffers based on the lsize (==size) of the buffer.

arc_hdr_decrypt() goes on decrypting and decompressing the data.

that buffer still has psize <= asize <= size

l2arc_write_buffers() comes across that buffer in ARC and goes on calling l2arc_apply_transforms() on it, which then causes your panic.

However, I am fine with not providing detailed examples of why (asize < size).

There we are. How's that?

module/zfs/arc.c

gamanakis · 2022-04-28T12:39:14Z

module/zfs/arc.c

 			goto encrypt;
 		}
 		ASSERT3U(psize, <=, HDR_GET_PSIZE(hdr));
 		if (psize < asize)
 			memset((char *)tmp + psize, 0, asize - psize);
 		psize = HDR_GET_PSIZE(hdr);
-		abd_return_buf_copy(cabd, tmp, asize);
+		abd_return_buf_copy(cabd, tmp, size);


Since we also allocated cabd with size in this PR, it is not going to be aligned to the vdev for writing (see vdev_psize_to_asize()).

After taking a second look, I don't think calling vdev_psize_to_asize() matters here, because l2arc_write_buffers() is going to write the buffer with a size of asize to L2ARC.

gamanakis

This looks good to me. I think it may be useful to add a comment explaining why we allocate cabd and tmp with size instead of asize (see above).

I genuinely don't know why this didn't come up before, but adding the LZ4 early abort pointed out this flaw, in which we're allocating a buffer of one size, and then telling the compressor that we're handing it buffers of a different size, which may be Very Different - say, allocating 512b and then telling it the inputs are 128k. Signed-off-by: Rich Ercolani <rincebrain@gmail.com>

I genuinely don't know why this didn't come up before, but adding the LZ4 early abort pointed out this flaw, in which we're allocating a buffer of one size, and then telling the compressor that we're handing it buffers of a different size, which may be Very Different - say, allocating 512b and then telling it the inputs are 128k. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: George Amanakis <gamanakis@gmail.com> Signed-off-by: Rich Ercolani <rincebrain@gmail.com> Closes openzfs#13375

In #13375 we modified the allocation size of the buffer that we use to apply l2arc transforms to be the size of the arc hdr we're using, rather than the allocation size that will be in place on the disk, because sometimes the hdr size is larger. Unfortunately, sometimes the allocation size is larger, which means that we overflow the buffer in that case. This change modifies the allocation to be the max of the two values Reviewed-by: Mark Maybee <mark.maybee@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Closes #15177 Closes #15248

In openzfs#13375 we modified the allocation size of the buffer that we use to apply l2arc transforms to be the size of the arc hdr we're using, rather than the allocation size that will be in place on the disk, because sometimes the hdr size is larger. Unfortunately, sometimes the allocation size is larger, which means that we overflow the buffer in that case. This change modifies the allocation to be the max of the two values Reviewed-by: Mark Maybee <mark.maybee@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Closes openzfs#15177 Closes openzfs#15248

In #13375 we modified the allocation size of the buffer that we use to apply l2arc transforms to be the size of the arc hdr we're using, rather than the allocation size that will be in place on the disk, because sometimes the hdr size is larger. Unfortunately, sometimes the allocation size is larger, which means that we overflow the buffer in that case. This change modifies the allocation to be the max of the two values Reviewed-by: Mark Maybee <mark.maybee@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Closes #15177 Closes #15248

In openzfs#13375 we modified the allocation size of the buffer that we use to apply l2arc transforms to be the size of the arc hdr we're using, rather than the allocation size that will be in place on the disk, because sometimes the hdr size is larger. Unfortunately, sometimes the allocation size is larger, which means that we overflow the buffer in that case. This change modifies the allocation to be the max of the two values Reviewed-by: Mark Maybee <mark.maybee@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Closes openzfs#15177 Closes openzfs#15248

rincebrain mentioned this pull request Apr 24, 2022

zstd early abort #13244

Merged

13 tasks

behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 25, 2022

gamanakis reviewed Apr 28, 2022

View reviewed changes

gamanakis approved these changes Apr 29, 2022

View reviewed changes

rincebrain force-pushed the uncomp_arc_is_untested branch from 56d9337 to 9cc52f5 Compare May 1, 2022 22:20

behlendorf approved these changes May 2, 2022

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 4, 2022

behlendorf merged commit 7bf06f7 into openzfs:master May 4, 2022

pcd1193182 mentioned this pull request Sep 7, 2023

Fix l2arc_apply_transforms ztest crash #15248

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Corrected edge case in uncompressed ARC->L2ARC handling #13375

Corrected edge case in uncompressed ARC->L2ARC handling #13375

rincebrain commented Apr 24, 2022

behlendorf commented Apr 25, 2022

gamanakis commented Apr 25, 2022

rincebrain commented Apr 25, 2022

gamanakis left a comment

gamanakis Apr 28, 2022

rincebrain Apr 28, 2022

gamanakis Apr 29, 2022

rincebrain Apr 29, 2022

gamanakis Apr 29, 2022 •

edited

Loading

gamanakis Apr 29, 2022

rincebrain May 1, 2022

gamanakis Apr 28, 2022

gamanakis Apr 28, 2022 •

edited

Loading

gamanakis left a comment

Corrected edge case in uncompressed ARC->L2ARC handling #13375

Corrected edge case in uncompressed ARC->L2ARC handling #13375

Conversation

rincebrain commented Apr 24, 2022

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

behlendorf commented Apr 25, 2022

gamanakis commented Apr 25, 2022

rincebrain commented Apr 25, 2022

gamanakis left a comment

Choose a reason for hiding this comment

gamanakis Apr 28, 2022

Choose a reason for hiding this comment

rincebrain Apr 28, 2022

Choose a reason for hiding this comment

gamanakis Apr 29, 2022

Choose a reason for hiding this comment

rincebrain Apr 29, 2022

Choose a reason for hiding this comment

gamanakis Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

gamanakis Apr 29, 2022

Choose a reason for hiding this comment

rincebrain May 1, 2022

Choose a reason for hiding this comment

gamanakis Apr 28, 2022

Choose a reason for hiding this comment

gamanakis Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

gamanakis left a comment

Choose a reason for hiding this comment

gamanakis Apr 29, 2022 •

edited

Loading

gamanakis Apr 28, 2022 •

edited

Loading