linux-kernel - Re: [PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due to error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20130131174540.GL2042@gmail.com>
Date:	Thu, 31 Jan 2013 17:45:40 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Linus Walleij <linus.walleij@...ricsson.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Stephen Warren <swarren@...dia.com>,
	Anmar Oueja <anmar.oueja@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due
 to error

On Thu, 31 Jan 2013, Stephen Warren wrote:

> On 01/31/2013 01:01 AM, Lee Jones wrote:
> > On Wed, 30 Jan 2013, Stephen Warren wrote:
> > 
> >> On 01/30/2013 12:40 PM, Linus Walleij wrote:
> >>> From: Lee Jones <lee.jones@...aro.org>
> >>>
> >>> Current failure path neglects to mutex_destroy() before returning
> >>> an error due to an invalid parameter or an error received from
> >>> gpiochip_add(). This patch aims to remedy that behaviour.
> >>
> >>> diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
> >>
> >>> @@ -1155,11 +1155,13 @@ static int abx500_gpio_probe(struct platform_device *pdev)
> >>>  	default:
> >>>  		dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
> >>>  				(int) platid->driver_data);
> >>> +		mutex_destroy(&pct->lock);
> >>>  		return -EINVAL;
> >>
> >> Especially given there's already a label out_free which performs this
> >> mutex_destroy(), those last two lines would be better as:
> >>
> >> ret = -EINVAL;
> >> goto out_free;
> > 
> > Yes, that's one way of doing it. I figured it was 6 of one and half a
> > dozen of the other to be honest.
> > 
> > Either I:
> > 
> >  +           mutex_destroy(&pct->lock);
> > 
> > Or:
> > 
> >  +           ret = -EINVAL; 
> >  +           goto out_free;
> >  -           return -EINVAL;
> > 
> > I figured the smallest diff would be best. To be honest, I'm not
> > bothered either way. If it offends you, I can do it the other way, no
> > problem. Just let me know quick, so I can get the fixed up patch to
> > Linus.
> > 
> > NB: There is no 'out_free:' at this point, it has already been
> > removed.
> 
> Where has it been removed? Both the latest linux-next and LinusW's
> pinctrl tree on git.kernel.org still contain it...

It's an ordering thing. I submitted a 13 patch patch-set to Linus
which completely reworks the driver. One of the first things I did was
to remove all IRQ handling from the driver and pass responsibility over
to the AB8500 core driver. However, there were some issues with some
of the patches, so Linus decided it would be better to get the simple
stuff out of the way whilst I fixup the more complex IRQ stuff. This
was one of the simple patches which was located at the 'end' of the
patch-set. I don't think you've seen the other stuff yet.

> The style in that code is clearly "goto foo" for error-handling, and
> makes for smaller simpler code, so I don't see why the label would be
> removed.
> 
> Still, this review is just a suggestion; this driver isn't anything I
> have any ownership of, so I guess feel free to go either way.

I too am easy and would be happy to go either way. The implementation
now only has one goto for post gpiochip_add() stuff. As I say, the
others have now been removed as they pertained to IRQ stuff.

Sorry for any confusion.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/