Re: [PATCH v3] scsi: ufs: core: Fix UFS RPMB device teardown order

[Bean Huo]

On Wed, 2026-07-22 at 11:43 +0200, Bean Huo wrote:
> On Tue, 2026-07-21 at 08:41 +0000, Ao Sun wrote:
> > From: Ao Sun <ao.sun@xxxxxxxxxxxxx>
> > 
> > The child RPMB device holds a reference to its parent, so the parent's
> > release callback cannot be invoked if the child device is still registered.
> > Remove the rpmb_dev_unregister() from the parent release handler, and
> > unregister the child RPMB device ahead of the parent device in the remove
> > path.
> > 
> > Memory for struct ufs_rpmb_dev is allocated via kzalloc_obj(), and free it
> > from the device release callback, following the same pattern as MMC RPMB.
> > 
> > Initialize the hba->rpmbs list in ufshcd_alloc_host() to prevent NULL
> > pointer dereference in the device teardown path if ufs_rpmb_probe()
> > fails.
> > 
> > Reported-by: sashiko-bot@xxxxxxxxxx
> > Closes:
> > https://lore.kernel.org/all/20260714064356.CF7101F000E9@xxxxxxxxxxxxxxx/
> > Signed-off-by: Jiazi Li <jiazi.li@xxxxxxxxxxxxx>
> > Signed-off-by: Ao Sun <ao.sun@xxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> >   - switch devm_kzalloc() to kzalloc_obj()
> >   - init rpmbs list in ufshcd
> > ---
> > Changes in v2:
> >   - drop the release callback
> >   - init rpmbs list early
> > ---
> >  drivers/ufs/core/ufs-rpmb.c | 8 ++++----
> >  drivers/ufs/core/ufshcd.c   | 1 +
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> > index ffad049872b9..cfd1f074d004 100644
> > --- a/drivers/ufs/core/ufs-rpmb.c
> > +++ b/drivers/ufs/core/ufs-rpmb.c
> > @@ -128,7 +128,7 @@ static void ufs_rpmb_device_release(struct device *dev)
> >  {
> >         struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);
> >  
> > -       rpmb_dev_unregister(ufs_rpmb->rdev);
> > +       kfree(ufs_rpmb);
> >  }
> >  
> >  /* UFS RPMB device registration */
> > @@ -152,8 +152,6 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >                 return -EINVAL;
> >         }
> >  
> > -       INIT_LIST_HEAD(&hba->rpmbs);
> > -
> >         struct rpmb_descr descr = {
> >                 .type = RPMB_TYPE_UFS,
> >                 .route_frames = ufs_rpmb_route_frames,
> > @@ -165,7 +163,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >                 if (!cap)
> >                         continue;
> >  
> > -               ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb),
> > GFP_KERNEL);
> > +               ufs_rpmb = kzalloc_obj(*ufs_rpmb);
> >                 if (!ufs_rpmb) {
> >                         ret = -ENOMEM;
> >                         goto err_out;
> > @@ -224,6 +222,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
> >         kfree(cid);
> >         list_for_each_entry_safe(it, tmp, &hba->rpmbs, node) {
> >                 list_del(&it->node);
> > +               rpmb_dev_unregister(it->rdev);
> >                 device_unregister(&it->dev);
> >         }
> >  
> > @@ -244,6 +243,7 @@ void ufs_rpmb_remove(struct ufs_hba *hba)
> >                 /* Remove from list first */
> >                 list_del(&ufs_rpmb->node);
> >                 /* Unregister device */
> > +               rpmb_dev_unregister(ufs_rpmb->rdev);
> >                 device_unregister(&ufs_rpmb->dev);
> >         }
> >  
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index d3044a3089b5..60227069db06 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -10988,6 +10988,7 @@ int ufshcd_alloc_host(struct device *dev, struct
> > ufs_hba **hba_handle)
> >         hba->nop_out_timeout = NOP_OUT_TIMEOUT;
> >         ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
> >         INIT_LIST_HEAD(&hba->clk_list_head);
> > +       INIT_LIST_HEAD(&hba->rpmbs);
> >         spin_lock_init(&hba->outstanding_lock);
> >  
> >         *hba_handle = hba;
> 
> 
> Hi Ao,
> 
> Thanks for the patch. I reviewed the whole RPMB core and OPTEE call sequencey,
> the issue is real, 
> 
> The issue is the lifetime of ufs_rpmb / ufs_rpmb->dev versus the rpmb device
> rdev when a consumer holds a reference of rdev.
> 
> 
> but I don't think we can take this patch. moving rpmb_dev_unregister() into
> the
> remove path and freeing ufs_rpmb eagerly, then make use-after-free.
> 
> 
> OP-TEE caches the rpmb device: on the RPMB probe path: optee->rpmb_dev =
> rpmb_dev_find_device(), whch taking its own reference, and reuses that handle
> for every subsequent frame in handle_rpc_func_rpmb_frames(). It only drops the
> reference in secure-world rescan or at optee teardown. And rpmb_class_intf has
> only add_dev, but no remove_dev, so nothing tells OP-TEE core when we
> unregister
> the device.
> 
> 
> I think the correct fix belongs in rpmb-core: a revocation barrier so that
> once
> rpmb_dev_unregister() returns, route_frames() is guaranteed never to run
> again, for example:
> 
>       route_frames()
>         {
>           down_read(&rdev->route_lock);  //block 
>           if (rdev->no_route)
>               ret = -ENODEV;
>           else
>               ret = rdev->descr.route_frames(rdev->dev.parent, ...);
>           up_read(&rdev->route_lock);
>           return ret;
>       }
>       
>       int rpmb_dev_unregister(struct rpmb_dev *rdev)
>       {
>           down_write(&rdev->route_lock);  //drain in-flight, block future 
>           rdev->no_route = true;
>           up_write(&rdev->route_lock);
>           device_del(&rdev->dev);
>           rpmb_dev_put(rdev);
>           return 0;
>       }   
>       
> @jens, how do you think? I'm happy to help with the rpmb-core piece if that's
> useful.
> 

Hi Jens, 

noticed your email chnaged, congrats on the new company! When you get a chance,
would you mind taking a look at the proposal above? Your suggestion on the rpmb-
core side would be much appreciated.


Kind regards,
Bean

> 
> the INIT_LIST_HEAD() move is a separate patch (NULL deref on the early-return
> probe paths, since hba->rpmbs is left zeroed). Please split it into new patch
> with a Fixes tag.
> 
> 
> Kind regards, 
> Bean
> 
>