Re: Re: [PATCH 1/2] cxl/mbox: clamp the event record count to the received payload

[黄高彬]

> I sent a similar patch a part of a series while ago and received the general
> feedback that the driver does not need special handling for a buggy device:
> https://lore.kernel.org/linux-cxl/20260901002912.958-1-anisa.su@xxxxxxxxxxx/T/#m38ed8d6482902f857bceef314133a67937cc012f

Thank you for the pointer -- I had not found your series, and having read it I
am withdrawing our patch.  [PATCH v2 2/4] cxl/events: Validate the record count
reported by the device is the same bug in the same function, bounds the count
with the same expression, and carries the same Fixes: commit (6ebe28f9ec72).
It is also the better of the two, which I did not appreciate until I wrote both
loops out side by side:

  - yours fails the command, so the walk stops at the first sign of a bad count;
  - ours clamped nr_rec to what the payload holds and carried on, which leaves
    nr_rec non-zero and lets `} while (nr_rec);` keep issuing Get and Clear
    Event Records to a device that never actually clears them.  Against a device
    that keeps claiming records it does not deliver, that is a hang;
  - your series bounds the per-log loop as well (CXL_EVENT_LOG_MAX_PASSES),
    which ours had nothing for.

Anything that fixes the read but leaves a device able to spin the kernel is not
worth posting, so v2 of our series drops it.

> Report a bad record count as an error rather than a drained log, so a device
> repeating the bad count does not keep the event thread re-reading it.

This is the part I got wrong.  I had argued that clamping "terminates" because a
count of zero takes the existing `if (!nr_rec) break;`.  That only covers the
device that delivers nothing; a device that delivers one record and claims
65535 every time never reaches it.

> And could you CC me on v2? There are some minor conflicts with the DCD
> prep patch I'm working on, so I will need to rebase those.

You are Cc'd on the patch we kept:

  [PATCH v2] cxl/mbox: bound the Get Supported Logs entry count by the payload
  https://lore.kernel.org/all/20260917104603.2658529-1-huanggaobin23@xxxxxxxxxx/

It is not in your series: it is the Get Supported Logs entry count during
command enumeration, a different function and a different response, so it
should not conflict with your DCD preparatory work.

Two things that may be of use:

- we have an independent reproduction from a QEMU Type-3 device that lies about
  the record count per response, with the boundary measured: a claim of 16 stays
  inside the 2048 byte buffer and 17 is the first that does not, KASAN in
  cxl_clear_event_record.  Happy to send it as supporting evidence, or as a test
  case for your patch.

- one optional suggestion: cxl_clear_event_record() still re-reads
  get_pl->record_count to decide how many handles to send.  Passing the count
  you validated would remove the invariant the two functions have to agree
  about, though with a single static caller it is not load bearing today.  I
  mention it only because it is the one thing our version did differently that
  was not worse.

Gaobin