Re: [PATCH] media: bttv: fix use after free error due to btv->timeout timer

[Zheng Hacker]

Hillf Danton <hdanton@xxxxxxxx> 于2023年2月21日周二 19:15写道：
>
> Feel free to add info like how this fix was tested.

Hi Hillf,

Sorry for my unclear description. I don't have the device to test. I
learn it from someone else's experience.

> >
> > Fix it by adding del_timer_sync invoking to the remove function.
> >
> > cpu0                cpu1
> >                   bttv_probe
> >                     ->timer_setup
> >                       ->bttv_set_dma
> >                         ->mod_timer;
> > bttv_remove
> >   ->kfree(btv);
> >                   ->bttv_irq_timeout
> >                     ->USE btv
> >
> > Signed-off-by: Zheng Wang <zyytlz.wz@xxxxxxx>
> > ---
> >  drivers/media/pci/bt8xx/bttv-driver.c | 1 +
> >  drivers/media/pci/bt8xx/bttv-risc.c   | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
> > index d40b537f4e98..24ba5729969d 100644
> > --- a/drivers/media/pci/bt8xx/bttv-driver.c
> > +++ b/drivers/media/pci/bt8xx/bttv-driver.c
> > @@ -4248,6 +4248,7 @@ static void bttv_remove(struct pci_dev *pci_dev)
> >
> >       /* free resources */
> >       free_irq(btv->c.pci->irq,btv);
> > +     del_timer_sync(&btv->timeout);
> >       iounmap(btv->bt848_mmio);
> >       release_mem_region(pci_resource_start(btv->c.pci,0),
> >                          pci_resource_len(btv->c.pci,0));
> > diff --git a/drivers/media/pci/bt8xx/bttv-risc.c b/drivers/media/pci/bt8xx/bttv-risc.c
> > index 32fa4a7fe76f..ada469198645 100644
> > --- a/drivers/media/pci/bt8xx/bttv-risc.c
> > +++ b/drivers/media/pci/bt8xx/bttv-risc.c
> > @@ -481,7 +481,7 @@ bttv_set_dma(struct bttv *btv, int override)
> >       if (btv->curr.frame_irq || btv->loop_irq || btv->cvbi) {
> >               mod_timer(&btv->timeout, jiffies+BTTV_TIMEOUT);
> >       } else {
> > -             del_timer(&btv->timeout);
> > +             del_timer_sync(&btv->timeout);
> >       }
> >       btv->main.cpu[RISC_SLOT_LOOP] = cpu_to_le32(cmd);
> >
> > --
> > 2.25.1
>
> This patch adds chance for deadlock by replacing del_timer() with del_timer_sync(), right?
>
>         cpu 0                           cpu 2
>         ---                             ---
>         bttv_read()                     bttv_irq_timeout()
>         bttv_reinit_bt848()
>         spin_lock_irqsave(&btv->s_lock,flags);
>
>                                         spin_lock_irqsave(&btv->s_lock,flags);
>                                         deadlock;
>         btv->errors=0;
>         bttv_set_dma(btv,0);
>         del_timer_sync(&btv->timeout);

Yes, it is. Thanks for pointing that out. Could you please give some
advice about the fix?

Best regards,
Zheng