Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755518Ab3FALeI (ORCPT ); Sat, 1 Jun 2013 07:34:08 -0400 Received: from mail-ob0-f175.google.com ([209.85.214.175]:36985 "EHLO mail-ob0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752001Ab3FALeA (ORCPT ); Sat, 1 Jun 2013 07:34:00 -0400 MIME-Version: 1.0 In-Reply-To: References: <1369817109-4277-1-git-send-email-benjamin.tissoires@redhat.com> <1369858343-681-1-git-send-email-andy.shevchenko@gmail.com> Date: Sat, 1 Jun 2013 14:33:59 +0300 Message-ID: Subject: Re: [PATCH] HID: multitouch: prevent memleak with the allocated name From: Andy Shevchenko To: Benjamin Tissoires Cc: linux-input , Jiri Kosina , Henrik Rydberg , Stephane Chatty , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2840 Lines: 75 On Thu, May 30, 2013 at 4:28 PM, Benjamin Tissoires wrote: > On Wed, May 29, 2013 at 10:12 PM, Andy Shevchenko > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/