Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4010f21b-4478-d860-c5ba-d6680d35993b@redhat.com>
Date:   Fri, 14 Oct 2022 02:07:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH drm-misc-next v3 4/7] drm/arm/hdlcd: use drm_dev_unplug()
Content-Language: en-US
To:     Liviu Dudau <liviu.dudau@arm.com>
Cc:     daniel@ffwll.ch, airlied@linux.ie, tzimmermann@suse.de,
        mripard@kernel.org, brian.starkey@arm.com,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20221001011905.433408-1-dakr@redhat.com>
 <20221001011905.433408-5-dakr@redhat.com>
 <Y0bYGjtAHbjeGJHF@e110455-lin.cambridge.arm.com>
From:   Danilo Krummrich <dakr@redhat.com>
Organization: RedHat
In-Reply-To: <Y0bYGjtAHbjeGJHF@e110455-lin.cambridge.arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Liviu,

On 10/12/22 17:07, Liviu Dudau wrote:
> Hi Danilo,
> 
> Appologies again for the delay in reviewing this as I was at XDC last week.

No worries, thanks for following up.

> Looking at the documentation for drm_dev_unplug, you can get a hint about what is going on:
> 
>   /*
>   * [....] There is one
>   * shortcoming however, drm_dev_unplug() marks the drm_device as unplugged before
>   * drm_atomic_helper_shutdown() is called. This means that if the disable code
>   * paths are protected, they will not run on regular driver module unload,
>   * possibly leaving the hardware enabled.
>   */
> 

Yes, that's the issue we have and pretty unfortunate. What we'd want for 
platform device drivers is to still be able to enter the sections locked 
with drm_dev_{enter,exit} on driver unbind, which we can't for at the 
moment.

I discussed this with Daniel Vetter on #dri-devel and for now he 
suggests to just not lock access to platform device bound resources and 
respin the patchset removing those parts.

Besides that I'll also work on a solution for drm_dev_unplug() / 
drm_dev_{enter,exit} to overcome this issue in the future.

> I'm finally getting to a conclusion: I'm still not sure what problem you were trying
> to solve when you have started this series and if you have found a scenario where
> you've got use after free then I would like to be able to reproduce it on my setup.
> Otherwise, I think this whole series introduces a regression on the behaviour of the
> driver and I would be inclined to reject it.

The problem is that DRM modeset objects should never be allocated with 
devm_*, since this can result in use-after free issues on driver unload. 
They should be freed when the last reference to the object is dropped, 
which DRM managed APIs take care of. As a consequence, DRM managed 
objects can potentially out-live platform device bound resources, which 
then should be protected from access. The first at least we can and 
should do.

It's not an issue that's unique to hdlcd, it's just a known issue to be 
addressed by all drivers. There's still the shortcoming concerning 
protecting platform bound resources as discussed above, but "the drmm 
parts should be a good idea no matter what" to cite Daniel.

I'll send a v4 without the drm_dev_{enter,exit} parts removed if that's 
fine for you.

- Danilo

> 
> Best regards,
> Liviu