Re: [PATCH v2 0/2] scsi: ufs: fix an RPMB teardown cycle and a shift UB

[Bart Van Assche <bvanassche@acm.org>]

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

On 11/3/26 10:59 AM, shao.mingyin@zte.com.cn wrote:
> Two independent fixes in the UFS stack, found while reading the probe and
> remove paths and the task-management hooks.

Please take a look at the attached review report from an LLM.

Thanks,

Bart.

[-- Attachment #2: review.md --]
[-- Type: text/markdown, Size: 9948 bytes --]

### Overview

Series: [[PATCH v2 0/2] scsi: ufs: fix an RPMB teardown cycle and a shift UB](https://lore.kernel.org/linux-scsi/202611040259247915t_cYYK0k5pPbEqfdTAQu@zte.com.cn/) (rebased onto `v7.4-rc1` / `mkp-scsi/for-next`, applies cleanly).

* **Patch 1/2 (`drivers/ufs/core/ufs-rpmb.c`):** **Needs changes / likely superseded.** It partially addresses the parent–child refcount cycle in [`ufs_rpmb_remove()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L260-L277), but leaves dead code in [`ufs_rpmb_device_release()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L152-L157), leaves [`struct ufs_rpmb_dev`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L35-L41) (which embeds `struct device`) allocated via `devm_kzalloc()`—exposing a use-after-free on unbind—and conflicts directly with Stanley Jhu's reviewed v5 series already on the list.
* **Patch 2/2 (`drivers/ufs/host/ufs-exynos.c`):** **Code change is fine as a `for-next` cleanup, but commit message and stable/Fixes tags need adjustment.**

---

### Series-Wide Issues

1. **Missing author name in `From:` header (`checkpatch.pl` warning):**
   ZTE's mailer sends `From: <shao.mingyin@zte.com.cn>` with no display name and no in-body `From:` line. Applying the mbox with `git am` / `b4 am` sets the commit author to `shao.mingyin@zte.com.cn <shao.mingyin@zte.com.cn>` and triggers [checkpatch.pl](file:///usr/local/google/home/bvanassche/software/linux-kernel/scripts/checkpatch.pl):
   ```text
   WARNING: From:/Signed-off-by: email name mismatch: 'From: shao.mingyin@zte.com.cn' != 'Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>'
   ```
   Generating the patches with `git format-patch --from="Shao Mingyin <shao.mingyin@zte.com.cn>"` adds an in-body `From:` header that survives the corporate MTA.
2. **Unrelated patches bundled together:**
   As the cover letter notes, Patch 1 (UFS core OP-TEE RPMB glue) and Patch 2 (Exynos host driver) share no code, context, or reviewers. Sending them separately avoids tying the simple Exynos cleanup to the active UFS RPMB rework.

---

### Patch 1/2: `scsi: ufs: core: unregister RPMB devices before their parent`

1. **Overlaps with Stanley Jhu's active v5 series (and cites outdated related work):**
   * The cover letter cites Stanley Jhu's v3 series and Dmitry Baryshkov's September 20 patch ([`[PATCH] scsi: ufs: rpmb: register and unregister the ufs_rpmb bus`](https://lore.kernel.org/linux-scsi/20260920-fix-ufs-rpmb-v1-1-560b842c589d@oss.qualcomm.com/)).
   * Dmitry's patch fails to link when `CONFIG_SCSI_UFSHCD=m` (`multiple definition of 'init_module'` / `'cleanup_module'`, reported by LKP), whereas Stanley Jhu posted a **v5** series on September 14 ([`[PATCH v5 0/3] rpmb: Fix request serialisation and teardown races`](https://lore.kernel.org/all/20260914144910.931518-1-stanleyjhu@google.com/), all three patches `Reviewed-by: Bean Huo`) that drops the unused `ufs_rpmb_bus_type`, breaks this same refcount cycle, decouples `ufs_rpmb` from devres, pins the RPMB WLUN `scsi_device`, and guards `rpmb_route_frames()` against teardown.

2. **`if (ufs_rpmb->rdev)` in `ufs_rpmb_device_release()` is dead code:**
   ```c
   static void ufs_rpmb_device_release(struct device *dev)
   {
   	struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);

   	if (ufs_rpmb->rdev)
   		rpmb_dev_unregister(ufs_rpmb->rdev);
   }
   ```
   `ufs_rpmb->rdev` can **never** be non-`NULL` when [`ufs_rpmb_device_release()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L152-L157) runs:
   * On the probe error paths before `ufs_rpmb->rdev = rdev` ([ufs-rpmb.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L207-L235)), `ufs_rpmb->rdev` is still `NULL`.
   * Once [`rpmb_dev_register()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/misc/rpmb-core.c#L154-L199) succeeds, `rdev->dev.parent` points to `&ufs_rpmb->dev` ([rpmb-core.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/misc/rpmb-core.c#L182)) and holds a parent reference until `device_del(&rdev->dev)` inside [`rpmb_dev_unregister()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/misc/rpmb-core.c#L131-L141). The only caller of `rpmb_dev_unregister()` is the new `ufs_rpmb_unregister_device()`, which clears `ufs_rpmb->rdev = NULL` before calling `device_unregister(&ufs_rpmb->dev)`.

3. **Broken `struct device` lifetime (`devm_kzalloc` + Use-After-Free):**
   * [`ufs_rpmb_probe()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L192) allocates [`struct ufs_rpmb_dev`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L35-L41)—which embeds `struct device dev`—using `devm_kzalloc(hba->dev, ...)`.
   * Once Patch 1 breaks the parent–child refcount cycle, `ufs_rpmb_device_release()` becomes an effective no-op that still dereferences `ufs_rpmb->rdev` instead of freeing `ufs_rpmb`. If anything holds a reference to `&ufs_rpmb->dev` across HBA driver unbind (e.g., open sysfs attributes, `CONFIG_DEBUG_KOBJECT_RELEASE=y` delayed release, or an OP-TEE client holding `rdev` via `rpmb_dev_get()`), `devres_release_all(hba->dev)` frees `ufs_rpmb` while `&ufs_rpmb->dev` is still live, causing a slab-use-after-free in `kobject_cleanup()` / `ufs_rpmb_device_release()` (and in [`ufs_rpmb_route_frames()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c#L76-L150) if a request is in flight).
   * Compare with [`mmc_blk_alloc_rpmb_part()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/mmc/core/block.c#L2870-L2929) and [`mmc_blk_rpmb_device_release()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/mmc/core/block.c#L2714-L2721) (which the commit message cites as its model): MMC allocates `struct mmc_rpmb_data` with `kzalloc_obj()` and frees it with `kfree()` inside the `.release` callback, and never calls `rpmb_dev_unregister()` from `.release`.
   * (Also, because [`ufshcd_add_lus()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L9358) ignores the return value of `ufs_rpmb_probe()`, `devm_kzalloc()` leaks memory on `hba->dev` until HBA removal whenever `ufs_rpmb_probe()` fails.)

4. **Minor naming / description nits (if kept):**
   * Recent commits to [ufs-rpmb.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufs-rpmb.c) use the subject prefix `scsi: ufs: rpmb: <Capitalized verb>...`.
   * The commit message ambiguously refers to both `&hba->ufs_rpmb_wlun->sdev_gendev` ("the UFS RPMB device") and `&ufs_rpmb->dev` / `&rdev->dev` ("child"/"parent") without distinguishing the three `struct device` layers (`sdev_gendev` -> `ufs_rpmb->dev` -> `rdev->dev`).

---

### Patch 2/2: `scsi: ufs: exynos: use BIT() for the nexus type tag mask`

The code change in [`exynos_ufs_specify_nexus_t_xfer_req()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-exynos.c#L991-L1003) and [`exynos_ufs_specify_nexus_t_tm_req()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-exynos.c#L1005-L1025) is clean, type-safe on both 32-bit and 64-bit builds (`type` is `u32`, so `type & ~BIT(tag)` zero-extends the upper 32 bits before masking), and matches [`exynos_ufs_post_link()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-exynos.c#L1192-L1193).

However, the commit metadata and description have three issues:

1. **Drop `Cc: stable@vger.kernel.org` (and likely `Fixes:`):**
   * The kernel builds with `-fno-strict-overflow` ([Makefile](file:///usr/local/google/home/bvanassche/software/linux-kernel/Makefile#L1175)), under which both GCC and Clang define two's-complement signed left-shift into the sign bit (`1 << 31` == `0x80000000`, and neither compiler emits a `-fsanitize=shift` check for `tag == 31`).
   * Unlike commit `01aad16c2257` ("scsi: ufs: exynos: Fix programming of HCI_UTRL_NEXUS_TYPE"), which shifted by `hba->nutrs == 32` (shift exponent `>= 32`, genuinely undefined even with `-fno-strict-overflow` and producing `0` on hardware), `tag` here is at most `31` and has no runtime bug, as the commit message itself states. Per [stable-kernel-rules.rst](file:///usr/local/google/home/bvanassche/software/linux-kernel/Documentation/process/stable-kernel-rules.rst#L15-L31), pure cleanups with no user-visible bug must not carry `Cc: stable@vger.kernel.org`.

2. **Inaccurate `tag` range and UFS terminology in the commit message:**
   * `nutrs` is the number of **UTP transfer request** slots, not "unified task tags".
   * Only [`exynos_ufs_specify_nexus_t_xfer_req()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-exynos.c#L991-L1003) receives `tag` in `0..nutrs - 1` (`0..31`, [`MASK_TRANSFER_REQUESTS_SLOTS_SDB`](file:///usr/local/google/home/bvanassche/software/linux-kernel/include/ufs/ufshci.h#L73)). [`exynos_ufs_specify_nexus_t_tm_req()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/host/ufs-exynos.c#L1005-L1025) receives a task management tag in `0..nutmrs - 1`, which per [`ufshcd_hba_capabilities()`](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/ufs/core/ufshcd.c#L2533-L2543) and [`MASK_TASK_MANAGEMENT_REQUEST_SLOTS`](file:///usr/local/google/home/bvanassche/software/linux-kernel/include/ufs/ufshci.h#L76) (`0x00070000`) is `0..7` and never reaches bit 31 (converting it to `BIT(tag)` is purely for consistency).

3. **Subject capitalization nit:**
   `scsi: ufs: exynos: Use BIT() for the nexus type tag mask` (capital `Use` to match SCSI/UFS commit conventions).