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

On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires
[off-list ref] wrote:

On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko
[off-list ref] wrote:

quoted

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.

quoted

--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

quoted

@@ -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.

quoted

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

quoted

@@ -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