[Intel-gfx] [PATCH 1/2] drm/core: Preserve the framebuffer after removing it.

[Tvrtko Ursulin]

On 09/10/2015 10:56 AM, Daniel Vetter wrote:
> On Thu, Sep 10, 2015 at 10:07:41AM +0100, Tvrtko Ursulin wrote:
>>
>> On 09/09/2015 08:06 PM, Daniel Vetter wrote:
>>> On Wed, Sep 9, 2015 at 6:36 PM, Tvrtko Ursulin
>>> <tvrtko.ursulin at linux.intel.com> wrote:
>>>> I am not even going that far, just talking about last frame stuck on screen.
>>>> For me making that easier is a regression.
>>>
>>> So let's look at various systems:
>>> - super-modern fbdev less system: logind keeps a dup of every
>>> master-capabel drm fd. Compositor crashing won't ever result in
>>> close() getting called since logind still has its copy. Cleanup needs
>>> to be done manually anyway with the system compositor.
>>> - Current systems: Compositor restarts and cleans up the mess we left behind.
>>
>> What if the compositor doesn't restart? Or logind crashes in the former
>> case?
>>
>> Maybe I don't understand something, but I don't see how it is not quite bad
>> to expect userspace to clean up the kernel structures after the previous
>> userspace client.
>
> That's not different from the compositor just freezing instead of
> crashing: Screen contents stays on and nothing happens. Imo this really is
> all just broken userspace, and the kernel can't make sure userspace
> doesn't randomly fall over.
>
> What we need to make sure is that assuming things work ok-ish there's no
> observed regression. And I still think that's the case here.

I would disagree on the no regressions when things work okay-ish 
principle, there should be no regressions in the pessimistic scenario 
when security is concerned.

If we can agree the stuck frame on screen is not desirable from the 
security point of view, then this change does enlarge the attack surface.

Because, apart from freezing the compositor, it now also works to crash 
it and prevent restart. Maybe it is far fetched, but as I said, 
attackers have much better imagination with these things.

So for me changes like this one shouldn't be pushed in easily.

>> What happens if something keeps crashing leaving framebuffers around?
>
> Only the active ones would be kept around, we still clean up everything
> else. So the leak is very limited from a memory pov.
>
>> If the only reason is to avoid modeset, why SETPLANE with NULL fb to disable
>> planes associated with a framebuffers to be released wouldn't work?
>
> Because in general drivers don't support that - primary plane helpers
> cant' do that and for many drivers that's the only thing we have.

Could that be extended so that primary plane helpers would try to 
disable planes for which framebuffers need to be removed?

Then drivers who can't disable planes keep doing a modeset and the ones 
that can just disable planes and correctly clean up framebuffers?

Regards,

Tvrtko