ceph: fix num_ops OBOE when crypto allocation fails

move_dirty_folio_in_page_array() may fail if the file is encrypted, the dirty folio is not the first in the batch, and it fails to allocate a bounce buffer to hold the ciphertext. When that happens, ceph_process_folio_batch() simply redirties the folio and flushes the current batch -- it can retry that folio in a future batch.

However, if this failed folio is not contiguous with the last folio that did make it into the batch, then ceph_process_folio_batch() has already incremented ceph_wbc->num_ops; because it doesn't follow through and add the discontiguous folio to the array, ceph_submit_write() -- which expects that ceph_wbc->num_ops accurately reflects the number of contiguous ranges (and therefore the required number of "write extent" ops) in the writeback -- will panic the kernel:

    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);

This issue can be reproduced on affected kernels by writing to fscrypt-enabled CephFS file(s) with a 4KiB-written/4KiB-skipped/repeat pattern (total filesize should not matter) and gradually increasing the system's memory pressure until a bounce buffer allocation fails.

Fix this crash by decrementing ceph_wbc->num_ops back to the correct value when move_dirty_folio_in_page_array() fails, but the folio already started counting a new (i.e. still-empty) extent.

The defect corrected by this patch has existed since 2022 (see first Fixes:), but another bug blocked multi-folio encrypted writeback until recently (see second Fixes:). The second commit made it into 6.18.16, 6.19.6, and 7.0-rc1, unmasking the panic in those versions. This patch therefore fixes a regression (panic) introduced by cac190c7674f.

Cc [email protected] # v6.18+

Fixes d55207717ded ("ceph: add encryption support to writepage and writepages")

Fixes cac190c7674f ("ceph: fix write storm on fscrypted files")

Signed-off-by Sam Edwards <[email protected]>

---

Changes v1->v2:
- Added a paragraph to the commit log briefly explaining the I/O pattern to reproduce the issue (thanks Slava)

- Additionally Cc'd [email protected] as required when handling regressions

Feedback not addressed:
- "Commit message should link to the mentioned BUG_ON line in a source listing" (link would not really help anyone, and the line is a moving target anyway)

- "Commit message should indicate that ceph_wbc->num_ops is passed to ceph_osdc_new_request() to explain why ceph_wbc->num_ops == req->r_num_ops"

    (ceph_wbc->num_ops is easy enough to search; and the cause->effect of the
     BUG_ON() is secondary to the central point that ceph_process_folio_batch()
     is responsible for ensuring ceph_wbc->num_ops is correct before returning)

- "An issue should be filed in the Ceph Redmine, linked via Closes:" (thanks Ilya for clarifying this is unnecessary)

---
 fs/ceph/addr.c | 4 ++++
 1 file changed, 4 insertions(+)

fs/ceph/addr.c +0 -0

index e87b3bb94ee8..f366e159ffa6 100644

Quoted 63 lines · Sam Edwards

move_dirty_folio_in_page_array() may fail if the file is encrypted, the
dirty folio is not the first in the batch, and it fails to allocate a
bounce buffer to hold the ciphertext. When that happens, ceph_process_folio_batch() simply redirties the folio and flushes the
current batch -- it can retry that folio in a future batch.

However, if this failed folio is not contiguous with the last folio that
did make it into the batch, then ceph_process_folio_batch() has already
incremented ceph_wbc->num_ops; because it doesn't follow through and
add the discontiguous folio to the array, ceph_submit_write() -- which
expects that ceph_wbc->num_ops accurately reflects the number of contiguous ranges (and therefore the required number of "write extent"
ops) in the writeback -- will panic the kernel:

    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);

This issue can be reproduced on affected kernels by writing to fscrypt-enabled CephFS file(s) with a 4KiB-written/4KiB- skipped/repeat
pattern (total filesize should not matter) and gradually increasing the
system's memory pressure until a bounce buffer allocation fails.

Fix this crash by decrementing ceph_wbc->num_ops back to the correct
value when move_dirty_folio_in_page_array() fails, but the folio already
started counting a new (i.e. still-empty) extent.

The defect corrected by this patch has existed since 2022 (see first
Fixes:), but another bug blocked multi-folio encrypted writeback until
recently (see second Fixes:). The second commit made it into 6.18.16,
6.19.6, and 7.0-rc1, unmasking the panic in those versions. This patch
therefore fixes a regression (panic) introduced by cac190c7674f.

Cc [email protected] # v6.18+

Fixes d55207717ded ("ceph: add encryption support to writepage and

writepages")

Fixes cac190c7674f ("ceph: fix write storm on fscrypted files")

Signed-off-by Sam Edwards <[email protected]>

---

Changes v1->v2:
- Added a paragraph to the commit log briefly explaining the I/O pattern to
  reproduce the issue (thanks Slava)

- Additionally Cc'd [email protected] as required when handling
  regressions

Quoted 5 lines

Feedback not addressed:
- "Commit message should link to the mentioned BUG_ON line in a source listing"
    (link would not really help anyone, and the line is a moving target anyway)

My request was to identify the location of:

BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);

Because, it's completely not clear from the commit message the location of this
code pattern.

There are two possible ways:
(1) Link
https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/ceph/addr.c#L1555
.
I hope you can see that it includes kernel version. So, if the line will change
with time, then this link always will identify the position of this code pattern
in v7.0-rc4, for example.

(2) You can show the function that contains this code pattern:

static
int ceph_submit_write(struct address_space *mapping,
struct writeback_control *wbc, struct ceph_writeback_ctl *ceph_wbc)
{
<skipped>

    BUG_ON(ceph_wbc->op_idx + 1 != req->r_num_ops);

<skipped>
}

Quoted 35 lines

- "Commit message should indicate that ceph_wbc->num_ops is passed to
   ceph_osdc_new_request() to explain why ceph_wbc->num_ops == req-

Quoted 1 line

r_num_ops"

    (ceph_wbc->num_ops is easy enough to search; and the cause-

Quoted 1 line

effect of the

     BUG_ON() is secondary to the central point that ceph_process_folio_batch()
     is responsible for ensuring ceph_wbc->num_ops is correct before returning)

- "An issue should be filed in the Ceph Redmine, linked via Closes:"
    (thanks Ilya for clarifying this is unnecessary)

---
 fs/ceph/addr.c | 4 ++++

 1 file changed, 4 insertions(+)

fs/ceph/addr.c +1 -0

index e87b3bb94ee8..f366e159ffa6 100644
@@ -1366,6 +1366,10 @@ void ceph_process_folio_batch(struct
 address_space *mapping,
  		rc = move_dirty_folio_in_page_array(mapping, wbc,
 ceph_wbc,
  				folio);
  		if (rc) {
+			/* Did we just begin a new contiguous op?

Nevermind! */

fs/ceph/addr.c +3 -0

+			if (ceph_wbc->len == 0)
+				ceph_wbc->num_ops--;
+

Quoted 3 lines

  folio_redirty_for_writepage(wbc, folio); folio_unlock(folio);
  break;

Let me run the xfstests for the patch. I'll be back with the result ASAP.

Reviewed-by Viacheslav Dubeyko <[email protected]>

...

Reviewed-by Viacheslav Dubeyko <[email protected]>

Quoted 1 line · Viacheslav Dubeyko

...

Hi Slava,

Quoted 1 line

Reviewed-by Viacheslav Dubeyko <[email protected]>

This looks like you gave "for future reference" feedback and provided a R-b tag for the current version of the patch; is that it? Or is this a tag to roll forward to a v3 with your feedback applied?