From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932098AbaENL0f (ORCPT ); Wed, 14 May 2014 07:26:35 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:62042 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754020AbaENL0d (ORCPT ); Wed, 14 May 2014 07:26:33 -0400 X-AuditID: cbfec7f4-b7fb36d000006ff7-8d-537352e62ec2 Message-id: <537352E5.20901@samsung.com> Date: Wed, 14 May 2014 13:26:29 +0200 From: Sylwester Nawrocki User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-version: 1.0 To: Stephen Boyd Cc: Mike Turquette , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Jiada Wang , Kyungmin Park Subject: Re: [PATCH 1/2] clk: Fix double free due to devm_clk_register() References: <1397863783-10728-1-git-send-email-sboyd@codeaurora.org> <1397863783-10728-2-git-send-email-sboyd@codeaurora.org> In-reply-to: <1397863783-10728-2-git-send-email-sboyd@codeaurora.org> Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrFLMWRmVeSWpSXmKPExsVy+t/xK7rPgoqDDeY+FLbo+VNpcbbpDbvF psfXWC0u75rDZvF0wkU2ix9nulkc2Dwu9/Uyedy5tofNY/OSeo/dX5sYPfq2rGL0+LxJLoAt issmJTUnsyy1SN8ugSvj6dfvjAXXDSq+ff/B3MB4TL2LkZNDQsBE4tahXWwQtpjEhXvrwWwh gaWMEhseWHcxcgHZnxgl2r79YQdJ8ApoSLRdWwdWxCKgKrGi+ScLiM0mYCjRe7SPsYuRg0NU IELi8QUhiHJBiR+T74GViAioS3zfcZINZCazwEFGiXt/IeYIC3hInLh8iBVicb1Ew9Q+NpA5 nAKuEutnSYCEmQV0JPa3TmODsOUlNq95yzyBUWAWkhWzkJTNQlK2gJF5FaNoamlyQXFSeq6h XnFibnFpXrpecn7uJkZIeH/Zwbj4mNUhRgEORiUe3h+rioKFWBPLiitzDzFKcDArifAyWhQH C/GmJFZWpRblxxeV5qQWH2Jk4uCUamDkviH53c8/z0zMdU//3Ya3a18deXl69X+H1KXMBXM3 TGI9sv9mK5/jmZ9Oj9LuMjA/z4i+zx237F2dtOou1qCdxyI0jq1clv3shIfYzkzeygwD9rmT o5bPtqr5tjZc9ZinVJlUfnb39VLm21xbi6SS/t5dLLpN77GjTuesht05SveUWv8HWExXYinO SDTUYi4qTgQAEBV5jU0CAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19/04/14 01:29, Stephen Boyd wrote: > Now that clk_unregister() frees the struct clk we're > unregistering we'll free memory twice: first we'll call kfree() > in __clk_release() with an address kmalloc doesn't know about and > second we'll call kfree() in the devres layer. Remove the > allocation of struct clk in devm_clk_register() and let > clk_release() handle it. This fixes slab errors like: > > ============================================================================= > BUG kmalloc-128 (Not tainted): Invalid object pointer 0xed08e8d0 > ----------------------------------------------------------------------------- > > Disabling lock debugging due to kernel taint > INFO: Slab 0xeec503f8 objects=25 used=15 fp=0xed08ea00 flags=0x4081 > CPU: 2 PID: 73 Comm: rmmod Tainted: G B 3.14.0-11032-g526e9c764381 #34 > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0x70/0xbc) > [] (dump_stack) from [] (slab_err+0x74/0x84) > [] (slab_err) from [] (free_debug_processing+0x2cc/0x31c) > [] (free_debug_processing) from [] (__slab_free+0x38/0x41c) > [] (__slab_free) from [] (clk_unregister+0xd4/0x140) > [] (clk_unregister) from [] (release_nodes+0x164/0x1d8) > [] (release_nodes) from [] (__device_release_driver+0x60/0xb0) > [] (__device_release_driver) from [] (driver_detach+0xb4/0xb8) > [] (driver_detach) from [] (bus_remove_driver+0x5c/0xc4) > [] (bus_remove_driver) from [] (SyS_delete_module+0x148/0x1d8) > [] (SyS_delete_module) from [] (ret_fast_syscall+0x0/0x48) > FIX kmalloc-128: Object at 0xed08e8d0 not freed > > Fixes: fcb0ee6a3d33 (clk: Implement clk_unregister) > Cc: Jiada Wang > Cc: Sylwester Nawrocki > Cc: Kyungmin Park > Signed-off-by: Stephen Boyd Thank you for correcting this, and my apologies for introducing this bug. Acked-by: Sylwester Nawrocki > --- > drivers/clk/clk.c | 71 +++++++++++++++++++++++-------------------------------- > 1 file changed, 30 insertions(+), 41 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index dff0373f53c1..f71093bf83ab 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1984,9 +1984,28 @@ struct clk *__clk_register(struct device *dev, struct clk_hw *hw) > } > EXPORT_SYMBOL_GPL(__clk_register); > > -static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) > +/** > + * clk_register - allocate a new clock, register it and return an opaque cookie > + * @dev: device that is registering this clock > + * @hw: link to hardware-specific clock data > + * > + * clk_register is the primary interface for populating the clock tree with new > + * clock nodes. It returns a pointer to the newly allocated struct clk which > + * cannot be dereferenced by driver code but may be used in conjuction with the > + * rest of the clock API. In the event of an error clk_register will return an > + * error code; drivers must test for an error code after calling clk_register. > + */ > +struct clk *clk_register(struct device *dev, struct clk_hw *hw) > { > int i, ret; > + struct clk *clk; > + > + clk = kzalloc(sizeof(*clk), GFP_KERNEL); > + if (!clk) { > + pr_err("%s: could not allocate clk\n", __func__); > + ret = -ENOMEM; > + goto fail_out; > + } > > clk->name = kstrdup(hw->init->name, GFP_KERNEL); > if (!clk->name) { > @@ -2026,7 +2045,7 @@ static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) > > ret = __clk_init(dev, clk); > if (!ret) > - return 0; > + return clk; > > fail_parent_names_copy: > while (--i >= 0) > @@ -2035,36 +2054,6 @@ fail_parent_names_copy: > fail_parent_names: > kfree(clk->name); > fail_name: > - return ret; > -} > - > -/** > - * clk_register - allocate a new clock, register it and return an opaque cookie > - * @dev: device that is registering this clock > - * @hw: link to hardware-specific clock data > - * > - * clk_register is the primary interface for populating the clock tree with new > - * clock nodes. It returns a pointer to the newly allocated struct clk which > - * cannot be dereferenced by driver code but may be used in conjuction with the > - * rest of the clock API. In the event of an error clk_register will return an > - * error code; drivers must test for an error code after calling clk_register. > - */ > -struct clk *clk_register(struct device *dev, struct clk_hw *hw) > -{ > - int ret; > - struct clk *clk; > - > - clk = kzalloc(sizeof(*clk), GFP_KERNEL); > - if (!clk) { > - pr_err("%s: could not allocate clk\n", __func__); > - ret = -ENOMEM; > - goto fail_out; > - } > - > - ret = _clk_register(dev, hw, clk); > - if (!ret) > - return clk; > - > kfree(clk); > fail_out: > return ERR_PTR(ret); > @@ -2173,7 +2162,7 @@ EXPORT_SYMBOL_GPL(clk_unregister); > > static void devm_clk_release(struct device *dev, void *res) > { > - clk_unregister(res); > + clk_unregister(*(struct clk **)res); > } > > /** > @@ -2188,18 +2177,18 @@ static void devm_clk_release(struct device *dev, void *res) > struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw) > { > struct clk *clk; > - int ret; > + struct clk **clkp; > > - clk = devres_alloc(devm_clk_release, sizeof(*clk), GFP_KERNEL); > - if (!clk) > + clkp = devres_alloc(devm_clk_release, sizeof(*clkp), GFP_KERNEL); > + if (!clkp) > return ERR_PTR(-ENOMEM); > > - ret = _clk_register(dev, hw, clk); > - if (!ret) { > - devres_add(dev, clk); > + clk = clk_register(dev, hw); > + if (!IS_ERR(clk)) { > + *clkp = clk; > + devres_add(dev, clkp); > } else { > - devres_free(clk); > - clk = ERR_PTR(ret); > + devres_free(clkp); > } > > return clk; > -- Sylwester Nawrocki Samsung R&D Institute Poland