Re: [PATCH v2] phy: rcar-gen3-usb2: cancel OTG work on unbind to avoid use-after-free

[khj]

 Hi Biju,

 Thanks for the review :)

 devm_add_action_or_reset() runs the action once immediately if it
fails to register it, and then returns the error. So on failure
rcar_gen3_phy_usb2_cancel_work() is called right away, i.e.
cancel_work_sync(&channel->work).

 That is safe here: channel->work has already been INIT_WORK()'d on
the line above, and the IRQ that schedules it (rcar_gen3_phy_usb2_irq)
is only requested after this call succeeds, so at that point the work
has never been queued. cancel_work_sync() on an
initialised-but-never-queued work is a no-op -- it just returns false.
(Unlike a timer, a work_struct has no uninitialised-object check on
the cancel path once INIT_WORK() has run.)

 So both paths are fine: either devres registers the cancel and runs
it on unbind, or registration fails, the cancel runs once immediately
(a harmless no-op), and probe returns the error.

 Thanks,
 Hyunjung

2026년 11월 9일 (월) 오후 8:45, Biju Das <biju.das.jz@xxxxxxxxxxxxxx>님이 작성:
>
> Hi hyunjungg,
>
> Thanks for the patch.
>
> > -----Original Message-----
> > From: hyunjungg <hj351016@xxxxxxxxx>
> > Sent: 09 November 2026 11:31
> > Subject: [PATCH v2] phy: rcar-gen3-usb2: cancel OTG work on unbind to avoid use-after-free
> >
> > From: Hyunjung Ko <hj351016@xxxxxxxxx>
> >
> > The OTG role/VBUS interrupt (rcar_gen3_phy_usb2_irq) and the role sysfs attribute schedule channel-
> > >work (rcar_gen3_phy_usb2_work), whose handler dereferences the channel (ch->extcon). The channel is
> > devm-allocated, so it is freed at unbind, but nothing cancels channel->work:
> > rcar_gen3_phy_usb2_remove() only drops the role sysfs file. A work scheduled just before unbind can
> > run after the channel has been freed, a use-after-free.
> >
> > Cancel the work via devm, registered before the interrupt is requested so that on unbind devm frees
> > the interrupt first (no new work can be
> > scheduled) and then runs the cancel action before the channel is freed.
> >
> > Fixes: c14f8a4032ef ("phy: rcar-gen3-usb2: fix mutex_lock calling in interrupt")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Hyunjung Ko <hj351016@xxxxxxxxx>
> > ---
> > Changes in v2 (thanks to the Sashiko AI review):
> >  - correct the Fixes: tag to c14f8a4032ef, which introduced channel->work
> >    and rcar_gen3_phy_usb2_work(), rather than 2b38543c8db1 which only added
> >    synchronous extcon handling
> >  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > index b5ba751..35e519a 100644
> > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > @@ -1171,6 +1171,13 @@ static int rcar_gen3_phy_usb2_vbus_regulator_register(struct rcar_gen3_chan
> > *cha
> >       return rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(channel, enable);  }
> >
> > +static void rcar_gen3_phy_usb2_cancel_work(void *data) {
> > +     struct rcar_gen3_chan *channel = data;
> > +
> > +     cancel_work_sync(&channel->work);
> > +}
> > +
> >  static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)  {
> >       struct device *dev = &pdev->dev;
> > @@ -1277,6 +1284,17 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> >               return irq;
> >       } else if (irq > 0) {
> >               INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> > +             /*
> > +              * Registered before the IRQ so that, on unbind, devm frees the
> > +              * IRQ first (no more work can be scheduled) and then cancels any
> > +              * work still pending before the channel is freed.
> > +              */
> > +             ret = devm_add_action_or_reset(dev,
> > +                                            rcar_gen3_phy_usb2_cancel_work,
> > +                                            channel);
> > +             if (ret < 0)
> > +                     return ret;
>
> I have a question,
>
> What happens if this return error? Is it ok to call
> cancel_work_sync() before scheduling it?
>
> Cheers,
> Biju
>
> > +
> >               ret = devm_request_irq(dev, irq, rcar_gen3_phy_usb2_irq,
> >                                      IRQF_SHARED, dev_name(dev), channel);
> >               if (ret < 0)
> > --
> > 2.43.0
> >
>