| 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
| ||
|
Message-ID: <6673435f-250a-4fb7-9843-20f050e85c7c@wanadoo.fr>
Date: Fri, 4 Oct 2024 19:54:37 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Jani Nikula <jani.nikula@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Tvrtko Ursulin <tursulin@...ulin.net>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>
Cc: linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/i915/display: Remove kstrdup_const() and
kfree_const() usage
Le 04/10/2024 à 11:35, Jani Nikula a écrit :
> On Thu, 03 Oct 2024, Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
>> kstrdup_const() and kfree_const() can be confusing in code built as a
>> module. In such a case, it does not do what one could expect from the name
>> of the functions.
>>
>> The code is not wrong by itself, but in such a case, it is equivalent to
>> kstrdup() and kfree().
>>
>> So, keep thinks simple and straightforward.
>>
>> This reverts commit 379b63e7e682 ("drm/i915/display: Save a few bytes of
>> memory in intel_backlight_device_register()")
>
> Sorry, I guess I'm confused here. Or I just didn't read the commit
> message to [1] properly. Or both.
>
> So the whole point of [1] was that the _const versions can be confusing
> if i915 is builtin? But not wrong?
I'll try to explain the whole story and (try to) be clearer.
[2] the intent of this initial patch was a micro-optimization which was
expected to save a few bytes of memory. The naming of the function
looked promising. However kstrdup_const() only saves the allocation
within the rodata section of the kernel [5,6]. The mechanism does not
work for code built as module.
This patch *is not* broken by itself, it is just pointless most of the
time. So keeping it as-is is just fine, from my point of view.
If built as a module, kstrdup_const() is just a plain kstrdup() and
kfree_const() is just kfree().
[3] was a variation that tried to avoid the allocation in all cases,
should it be built as a module or not.
Being a micro-optimization of a slow path, your argument of keeping
things simple is just fine for me.
[4] just revert [2].
[2] was not broken, so [4] does not fix anything. It just makes things
simpler and as before.
So the whole point of [1,3] was that the _const versions can be
confusing if i915 is *NOT* builtin.
But it *is* not wrong, just likely useless in such a case.
So, from my point of view, keeping [2] as is, or applying [3] or [4] on
top of it does not change things much, and each solution is correct.
The idea behind removing some usage of _const() function in modules is
related to the patch proposal [7] and more precisely the response of
Christoph Hellwig [8]. The patch [7] will not be applied because it
breaks things.
So, should this API be removed one day, or at least removed for modules,
the more preparation work is already done (up to now: 4,9,10] the better
it is.
CJ
[2]: 379b63e7e682 ("drm/i915/display: Save a few bytes of memory in
intel_backlight_device_register()")
[3]:
https://lore.kernel.org/all/3b3d3af8739e3016f3f80df0aa85b3c06230a385.1727533674.git.christophe.jaillet@wanadoo.fr/
[4]:
https://lore.kernel.org/all/f82be2ee3ac7d18dd9982b5368a88a5bf2aeb777.1727977199.git.christophe.jaillet@wanadoo.fr/
[5]: https://elixir.bootlin.com/linux/v6.12-rc1/source/mm/util.c#L84
[6]:
https://elixir.bootlin.com/linux/v6.12-rc1/source/include/asm-generic/sections.h#L177
[7]:
https://lore.kernel.org/all/20240924050937.697118-1-senozhatsky@chromium.org/
[8]: https://lore.kernel.org/all/ZvJfhDrv-eArtU8Y@infradead.org/
[9]:
https://lore.kernel.org/all/63ac20f64234b7c9ea87a7fa9baf41e8255852f7.1727374631.git.christophe.jaillet@wanadoo.fr/
[10]:
https://lore.kernel.org/all/06630f9ec3e153d0e7773b8d97a17e7c53e0d606.1727375615.git.christophe.jaillet@wanadoo.fr/
>
> BR,
> Jani.
>
>
> [1] https://lore.kernel.org/r/3b3d3af8739e3016f3f80df0aa85b3c06230a385.1727533674.git.christophe.jaillet@wanadoo.fr
>
>
>
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>> drivers/gpu/drm/i915/display/intel_backlight.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
>> index 9e05745d797d..3f81a726cc7d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
>> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
>> @@ -949,7 +949,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>> else
>> props.power = BACKLIGHT_POWER_OFF;
>>
>> - name = kstrdup_const("intel_backlight", GFP_KERNEL);
>> + name = kstrdup("intel_backlight", GFP_KERNEL);
>> if (!name)
>> return -ENOMEM;
>>
>> @@ -963,7 +963,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>> * compatibility. Use unique names for subsequent backlight devices as a
>> * fallback when the default name already exists.
>> */
>> - kfree_const(name);
>> + kfree(name);
>> name = kasprintf(GFP_KERNEL, "card%d-%s-backlight",
>> i915->drm.primary->index, connector->base.name);
>> if (!name)
>> @@ -987,7 +987,7 @@ int intel_backlight_device_register(struct intel_connector *connector)
>> connector->base.base.id, connector->base.name, name);
>>
>> out:
>> - kfree_const(name);
>> + kfree(name);
>>
>> return ret;
>> }
>
Powered by blists - more mailing lists