[PATCH v3 2/4] soc: amlogic: meson-gx-socinfo-sm: Add Amlogic secure-monitor SoC Information driver
Krzysztof Kozlowski
krzysztof.kozlowski at linaro.org
Thu Mar 14 06:31:29 PDT 2024
On 14/03/2024 13:22, Viacheslav wrote:
>> +
>>> + soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
>>> + GFP_KERNEL);
>>> + if (!soc_dev_attr)
>>> + return -ENOMEM;
>>> +
>>> + soc_dev_attr->serial_number = socinfo_get_chipid(&pdev->dev, fw, &socinfo);
>>> +
>>> + soc_dev_attr->family = "Amlogic Meson";
>>> + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
>>> + socinfo.v1.major_id,
>>> + socinfo.v1.chip_rev,
>>> + socinfo.v1.pack_id,
>>> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
>>> + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
>>> + socinfo_v1_to_soc_id(socinfo),
>>> + socinfo_v1_to_package_id(socinfo));
>>> +
>>> + soc_dev = soc_device_register(soc_dev_attr);
>>> +
>>> +
>>> + if (IS_ERR(soc_dev)) {
>>> + kfree(soc_dev_attr->revision);
>>> + kfree_const(soc_dev_attr->soc_id);
>>> + kfree(soc_dev_attr);
>>
>> That's a double free. This was not tested.
>
>
> Please, describe the problem.
Test your code. What's the point of arguing over it if regular test
would show this?
> I don't quite understand what the issue is:
>
> - kfree() releases memory allocated with kmalloc()
So point me where is kmalloc(). I don't see. I see only devm.
> - kasprintf() allocates memory using kmalloc_track_caller()
>
> Technically, I see no difficulty in freeing the newly allocated memory.
> In case of memory allocation issues in kasprintf, we would just get
> NULL, which kfree should also handle properly. Considering that we don't
> need soc_dev_attr anymore, we don't need to worry about the contents of
> .revision and .soc_id.
Please pay attention that my comment is under specific line. We do not
discuss unrelated code.
>
> I see that kfree_const has crept in by accident, which is essentially
> needed here only if we replace kasprintf with kasprintf_const for
> .soc_id, but it does not introduce any erroneous behavior.
>
>>
>>> + return PTR_ERR(soc_dev);
>>> + }
>>> +
>>> + dev = soc_device_to_device(soc_dev);
>>> + platform_set_drvdata(pdev, soc_dev);
>>> +
>>> + dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected (SM)\n",
>>> + soc_dev_attr->soc_id,
>>> + socinfo.v1.major_id,
>>> + socinfo.v1.chip_rev,
>>> + socinfo.v1.pack_id,
>>> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
>>> +
>>> + return PTR_ERR_OR_ZERO(dev);
>>> +}
>>> +
>>> +
>>> +static int meson_gx_socinfo_sm_remove(struct platform_device *pdev)
>>> +{
>>> + struct soc_device *soc_dev = platform_get_drvdata(pdev);
>>> +
>>> + soc_device_unregister(soc_dev);
>>
>> If you free the memory in probe() error path, why you did not decide to
>> free it here as well? It is symmetrical, so this should make you wonder
>> - error path is wrong.
>
> This is something I can easily explain:
>
> In the case where we have successfully registered with
> soc_device_register, we clean up everything that was manually allocated
> and not used.
> In the case of unloading the driver, the cleanup should be handled by
> the soc_device_unregister command.
>
> Technically, it's not possible to insert memory release because until
> the command is called, the driver is active, and afterwards, there's no
> guarantee of the pointer's validity.
Then you do not understand lifecycle of device. There is release here
via devm. Exactly at after my comment, when } finishes.
> Perhaps it would have been better if soc_device_register copied the
> entire soc_device_attribute structure and took care of memory allocation
> and release itself, then we could comfortably free any excess memory
> back in _probe.
>
> Are you currently recommending not to release memory within the if
> (IS_ERR(soc_dev)) section?
You have double free which will be pointed out by testing. Yes, of
course I recommend not to have double free, so not to release memory
which is being released.
Best regards,
Krzysztof
More information about the linux-amlogic
mailing list