Re: [PATCH] HID: multitouch: prevent memleak with the allocated name

[Andy Shevchenko]

On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxx> wrote:
> On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> 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.

>> +               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?

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/