linux-kernel - Re: [PATCH v4 03/14] drm/fb-helper: separate the fb_setcmap helper into atomic and legacy paths

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <45e64660-b903-6f2b-42f9-55a42e478810@axentia.se>
Date:   Wed, 12 Jul 2017 09:47:27 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     linux-kernel@...r.kernel.org,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        dri-devel@...ts.freedesktop.org,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [PATCH v4 03/14] drm/fb-helper: separate the fb_setcmap helper
 into atomic and legacy paths

On 2017-07-12 09:03, Daniel Vetter wrote:
> On Tue, Jul 11, 2017 at 02:12:26PM +0200, Peter Rosin wrote:
>> On 2017-07-11 10:10, Daniel Vetter wrote:
>>> Tiny nit you might want to improve (since you need to respin for my naming
>>> bikeshed of the property_replace_blob function anyway): Properties are
>>> refcounting and invariant, which means you can just create the property
>>> once, and then use it for all the CRTC. Slightly cleaner code.
>>
>> Yes, I thought about that, but ended up not. The reason is that as far
>> as I could tell, all involved crtc need not have the same original
>> gamma_lut. Sure, if all crtc have the same history, that should be the
>> case, but isn't it possible to tie things together one way first and
>> set some clut, then "rewire" things so that the crtc no longer have the
>> same history?
>>
>> But if you in the light of that still think it's wise to set the same
>> clut for all crtc I will factor that part out of the loop.
> 
> Blob properties are invariant, if you want to change a lut you _have_ to
> create a new blob property. They're also reference-counted, which means
> users of a blob property can come&go as they wish, it will only get freed
> when the last one is released.
> 
> So even when you change the lut of 1 CRTC the other CRTCs will be able to
> keep using the existing lut blob property unchanged. That's the beauty of
> having refcounted objects with invariant data over their lifetime, makes a
> lot of things a lot simpler. drm_framebuffer work the same (only their
> metadata is invariant, the data of the actual backing storage can change
> ofc, but not where that backing storage is). Allows you to do simple
> pointer comparison of objects to check whether their equal or something
> has changed.
> 
> tldr; sharing blobs is perfectly safe and how this is designed to work.

Yes, I get that, but that wasn't my problem. At all.

Say that you have a driver with two crtc, A and B. Then this happens:

1. A gets a clut with, say, only various red colors.
2. B gets a different clut with various green colors.
3. Someone ties things up so that one fbdev is used on both A and B.
   I don't know if this is possible, but if it is, the two crtc now
   have different cluts.
4. Via fbdev, only part of the clut is updated for this A/B combo.

If A and B starts sharing clut in 4, the part that is not updated is
clobbered for either crtc A or B.
  
(updating only part of the clut is only possible with fbdev, AFAICT)

Yes, it's a fringe thing to cater to...

Cheers,
Peter