linux-kernel - Re: [PATCH] drm/mediatek: Set sensible cursor width/height values to fix crash

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 for Android: free password hash cracker in your pocket

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

Message-ID: <e74e25b01f85fb33d95e073f18a8049288b5c70d.camel@mediatek.com>
Date: Fri, 19 Jul 2024 08:42:22 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "daniel@...ishbar.org"
	<daniel@...ishbar.org>
CC: "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"wenst@...omium.org" <wenst@...omium.org>, "chunkuang.hu@...nel.org"
	<chunkuang.hu@...nel.org>, "kernel@...labora.com" <kernel@...labora.com>,
	Shawn Sung (宋孝謙) <Shawn.Sung@...iatek.com>,
	"daniel@...ll.ch" <daniel@...ll.ch>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "airlied@...il.com" <airlied@...il.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "fshao@...omium.org" <fshao@...omium.org>
Subject: Re: [PATCH] drm/mediatek: Set sensible cursor width/height values to
 fix crash

Hi, Angelo:

On Thu, 2024-07-18 at 13:23 +0200, AngeloGioacchino Del Regno wrote:
> Il 18/07/24 13:10, Daniel Stone ha scritto:
> > Hi all,
> > 
> > On Thu, 18 Jul 2024 at 11:24, AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@...labora.com> wrote:
> > > Il 18/07/24 11:27, Fei Shao ha scritto:
> > > > This matches my preference in [1], so of course I'd like to see it
> > > > merged... if maintainers are okay with it.
> > > > Given I've tested the exact same change before:
> > > > Reviewed-by: Fei Shao <fshao@...omium.org>
> > > > Tested-by: Fei Shao <fshao@...omium.org>
> > > 
> > > Thanks!
> > 
> > And:
> > Reviewed-by: Daniel Stone <daniels@...labora.com>
> > 
> > > > > OOTH, Intel recently added a feature for enumerating "suggested"
> > > > > cursor sizes. See https://urldefense.com/v3/__https://patchwork.freedesktop.org/patch/583299/__;!!CTRNKA9wMg0ARbw!nRf6mf-9tnE7vLYracLE6Xq_oblRvtENffF73fRzgz_E3zPc3yxeQPE5yPw95sj-ZeoiYJCQSIPWFZ0C3HCXpBkHikWK$ 
> > > > > 
> > > > > Not sure if other compositors will end up using it or not.
> > > 
> > > Yeah, that's good, and we might do that as well in MediaTek DRM... in a slightly
> > > different way, as it looks like they are simply hinting the same values as the
> > > mode_config is declaring... while we'd be adding a hint with a sensible size that
> > > is less than the maximum supported one from the overlay.
> > > 
> > > In reality, here, the issue is that the most popular compositors do not support
> > > overlay planes (as in, they don't use them at all)... my first idea was to remove
> > > the CURSOR plane entirely and declare it as per what it is for real (an OVERLAY),
> > > but that would only give a performance penalty as that'd become yet another unused
> > > plane and nothing else.
> > > 
> > > If at least the most popular compositors did support overlay planes, I'd have done
> > > that instead... but oh, well!
> > > 
> > > And anyway I hope that the maintainers are okay with this because, well, otherwise
> > > MediaTek SoCs won't be usable with any popular WM.
> > 
> > Every compositor is going to use it, yeah. But until it does, people
> > are just going to use cursor_width and cursor_size. A lot of older
> > desktop hardware supports only a single fixed dimension for the cursor
> > plane (hence the single values), so rather than guess if it needs to
> > be 32x32 or 64x64 or whatever, people just allocate to the size. Not
> > to mention that the old pre-atomic cursor ioctls actually require that
> > you allocate for cursor_width x cursor_height.
> > 
> > So yeah, this is the right fix - though you could even be more
> > aggressive and reduce it to 256x256 - and supporting the CURSOR_SIZE
> > property would be even more useful again.
> > 
> 
> I thought about being more aggressive, but then I saw that IGT tests for up to 512
> and that MSM also declares the same, so, making IGT happy because we can indeed
> support that much (since we can support even more, but doesn't make sense) :-)
> 
> Regarding CURSOR_SIZE ... right, I can take a look at that a bit later, most
> probably not for this merge window, though.

This patch looks acceptable but it could be better.
It's urgent to fix the crash, if better solution does not come out soon,
I would apply this patch first.

Reviewed-by: CK Hu <ck.hu@...iatek.com>

I will remove the Fixes tag because Shawn's patch has no logical problem but the system resource is not enough.

It's a dilemma that small size has no resource problem but application is limited
and large size has resource problem but support more application.

Regards,
CK

> 
> Cheers!
> 
> > Cheers,
> > Daniel
> 
>