lkml.org 
[lkml]   [2013]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] HID: multitouch: prevent memleak with the allocated name
From
On Sat, Jun 1, 2013 at 1:33 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> mt_free_input_name() was never called during .remove(): hid_hw_stop()
>>> removes the hid_input items in hdev->inputs, and so the list is
>>> therefore empty after the call. In the end, we never free the special
>>> names that has been allocated during .probe().
>>>
>>> We switch to devm_kzalloc that manages resources when driver is removed.
>
> Thanks for review. See my answers below.
>
>>> --- a/drivers/hid/hid-multitouch.c
>>> +++ b/drivers/hid/hid-multitouch.c
>
>>> @@ -412,10 +404,12 @@ static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
>>> static void mt_pen_input_configured(struct hid_device *hdev,
>>> struct hid_input *hi)
>>> {
>>> - char *name = kzalloc(strlen(hi->input->name) + 5, GFP_KERNEL);
>>> - if (name) {
>>> - sprintf(name, "%s Pen", hi->input->name);
>>> - mt_free_input_name(hi);
>>> + char *name;
>>> +
>>> + if (hdev->name) {
>>
>> hdev->name is always not null, so no need to check this (hint: it
>> contains hid->name when allocated).
>
> Okay, I'll fix it.

thanks. And sorry, I thought the test was against hi->input->name,
thus my "hint" which was exactly the same as what you tested.
Anyway, hdev->name is still never null, so the remark still applies.
>
>>> + name = devm_kzalloc(&hdev->dev, strlen(hdev->name) + 5,
>>> + GFP_KERNEL);
>>
>> Does devm_kzalloc always return a valid pointer? If not, you should
>> just use devm_kzalloc instead of kzalloc and keep the old ordering of
>> allocation, test, and snprintf.
>
> Good point, will fix.
>
>>> @@ -925,16 +919,18 @@ static void mt_post_parse(struct mt_device *td)
>>> static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>> {
>>> struct mt_device *td = hid_get_drvdata(hdev);
>>> - char *name = kstrdup(hdev->name, GFP_KERNEL);
>>> -
>>> - if (name)
>>> - hi->input->name = name;
>>>
>>> if (hi->report->id == td->mt_report_id)
>>> mt_touch_input_configured(hdev, hi);
>>>
>>> if (hi->report->id == td->pen_report_id)
>>> mt_pen_input_configured(hdev, hi);
>>> +
>>> + if (!hi->input->name) {
>>
>> will never happen, so can be dropped.
>
> Why not? As far as I understood this logic the input->name is assigned
> accordingly to what device is configured. Like input->name =
> hdev->name, except for pen it equals to hdev->name + " Pen". The last
> one is assigned in the mt_pen_input_configure. Otherwise input->name
> is NULL. Am I correct?

No. When the hid_input struct is allocated in hid-input.c, the field
hi->input->name is set to hdev->name (my previous "hint" should be
here).

The whole dirty part of the patch when I included the "Pen" in the
name was because of that: hi->input->name is never null, and it was
difficult to change this in hid-core.c without any side effects in the
other hid drivers.

Cheers,
Benjamin


\
 
 \ /
  Last update: 2013-06-01 16:21    [W:0.050 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site