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

From: Chris Diamand <chris.diamand@foss.arm.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: "Neil Armstrong" <neil.armstrong@linaro.org>,
	"Nicolas Boichat" <drinkcat@chromium.org>,
	"Daniel Stone" <daniels@collabora.com>,
	"Liviu Dudau" <Liviu.Dudau@arm.com>,
	dri-devel@lists.freedesktop.org,
	"Steven Price" <steven.price@arm.com>,
	"Clément Péron" <peron.clem@gmail.com>,
	kernel@collabora.com, "Robin Murphy" <robin.murphy@arm.com>,
	"Faith Ekstrand" <faith.ekstrand@collabora.com>,
	"Marty E . Plummer" <hanetzer@startmail.com>
Subject: Re: [PATCH v3 08/14] drm/panthor: Add the FW logical block
Date: Mon, 22 Jan 2024 21:14:05 +0000	[thread overview]
Message-ID: <101719ac-079a-42e8-9997-1d44fb27e80f@foss.arm.com> (raw)
In-Reply-To: <20240122173419.79abd4e3@collabora.com>

On 22/01/2024 16:34, Boris Brezillon wrote:
> 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().

Sounds good to me - thanks!