Re: [PATCH v3 08/14] drm/panthor: Add the FW logical block

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 18 Dec 2023 21:25:16 +0000
Chris Diamand <chris.diamand@foss.arm.com> wrote:

> > +void panthor_fw_unplug(struct panthor_device *ptdev)
> > +{
> > +	struct panthor_fw_section *section;
> > +
> > +	cancel_delayed_work_sync(&ptdev->fw->watchdog.ping_work);
> > +
> > +	/* Make sure the IRQ handler can be called after that
> > point. */
> > +	if (ptdev->fw->irq.irq)
> > +		panthor_job_irq_suspend(&ptdev->fw->irq);
> > +
> > +	panthor_fw_stop(ptdev);
> > +
> > +	if (ptdev->fw->vm)
> > +		panthor_vm_idle(ptdev->fw->vm);
> > +
> > +	list_for_each_entry(section, &ptdev->fw->sections, node)
> > +		panthor_kernel_bo_destroy(panthor_fw_vm(ptdev),
> > section->mem);  
> 
> Here's where we destroy the potentially invalid section->mem.
> 
> Note that the issues are twofold:
> Firstly, section->mem could be an error pointer as mentioned earlier.
> Secondly, panthor_kernel_bo_destroy() doesn't actually check for
> error values or even NULL.
> 
> It would probably be worth adding NULL checks to
> panthor_kernel_bo_destroy() and panthor_kernel_bo_vunmap() to protect
> against this. 

I ended up implementing your suggestion, because it simplifies all
call-sites in panthor_sched.c too. The new version defer the
IS_ERR_OR_NULL() check to panthor_kernel_bo_destroy().