From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932537AbcI1TNJ (ORCPT ); Wed, 28 Sep 2016 15:13:09 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:53707 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752032AbcI1TM7 (ORCPT ); Wed, 28 Sep 2016 15:12:59 -0400 Date: Wed, 28 Sep 2016 15:10:27 -0400 (EDT) From: Thomas Gleixner To: Arvind Yadav cc: baohua@kernel.org, daniel.lezcano@linaro.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4] clocksource/timer-imx-gpt: Prevent resource leake in error path. In-Reply-To: <1475084605-8433-1-git-send-email-arvind.yadav.cs@gmail.com> Message-ID: References: <1475084605-8433-1-git-send-email-arvind.yadav.cs@gmail.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 28 Sep 2016, Arvind Yadav wrote: > -Free previously allocated memory. > -Unmap I/O memory from kernel address space. Ok. So we have proper looking patch with a proper looking changelog now. Though > @@ -507,11 +511,17 @@ static int __init mxc_timer_init_dt(struct device_node *np, enum imx_gpt_type t > > ret = _mxc_timer_init(imxtm); > if (ret) > - return ret; > + goto error_iounmap; Have you actually thought about the implications of this? Assume the last function call() which gets invoked via _mxc_timer_init() fails and then figure out what the iounmap() and the kfree() are going to cause. Hint: You just turned a resource leak into a fatal crash. Just slapping unmap/kfree blindly at everything which can return a failure is not a good idea if you do not check what the functions actually do and what the error pathes there are. While I agree that the error handling in this and in other drivers is lousy, fixing it just mechanicaly and thereby introducing harder to debug wreckage is certainly not the right approach. Thanks, tglx