Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Jun 2023 22:58:39 +0200
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Andy Shevchenko <andriy.shevchenko@intel.com>
Cc:     Michal Wilczynski <michal.wilczynski@intel.com>,
        linux-acpi@vger.kernel.org, rafael@kernel.org,
        ilpo.jarvinen@linux.intel.com, hdegoede@redhat.com,
        markgross@kernel.org, fengguang.wu@intel.com,
        dvhart@linux.intel.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on
 error path
Message-ID: <20230612205839.wxo2h2ahqcdo73rc@pali>
References: <20230612090250.1417940-1-michal.wilczynski@intel.com>
 <20230612175205.eom2guabgfmnzrce@pali>
 <ZIeFjkAAiS+TMa8g@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZIeFjkAAiS+TMa8g@smile.fi.intel.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Monday 12 June 2023 23:52:30 Andy Shevchenko wrote:
> On Mon, Jun 12, 2023 at 07:52:05PM +0200, Pali Rohár wrote:
> > On Monday 12 June 2023 12:02:50 Michal Wilczynski wrote:
> > > Currently rbtn_add() in case of failure is leaking resources. Fix this
> > > by adding a proper rollback. While at it, remove unnecessary assignment
> > > of NULL to device->driver_data and unnecessary whitespace, plus add a
> > > break for the default case in a switch.
> 
> ...
> 
> > Hello! I'm looking at rbtn_add() function and there is also code:
> > 
> > 	rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL);
> > 	if (!rbtn_data)
> > 		return -ENOMEM;
> > 
> > which is called after rbtn_acquire(). So it looks like when kzalloc
> > fails then there is another leak...
> 
> Side note: In that case we would need a devm wrapper on acquire call.

Does it makes sense to invest time and more resources for these fixes?
Driver is not used on new Dell machines, so I would not expect new
users (instead less users, if they start upgrading HW to new Dell
machines).

Simple fix for this issue: Just move devm_kzalloc() call before
rbtn_acquire(true). And call cleanup rbtn_acquire(false) exactly like
Michal did in this patch.

I think that this should be enough, should cover all failure paths and
does not require to introduce new code or new design, which should be
properly tested for no regression.

What do you think?