linux-kernel - Re: [PATCH] pinctrl: renesas: rzg2l: Fix double unlock in rzg2l_dt_subnode_to

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <300d204b-8151-45f3-9977-7ceb3a5c5eb0@moroto.mountain>
Date: Fri, 12 Jan 2024 12:53:10 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: claudiu beznea <claudiu.beznea@...on.dev>
Cc: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-renesas-soc@...r.kernel.org, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] pinctrl: renesas: rzg2l: Fix double unlock in
 rzg2l_dt_subnode_to_map()

On Fri, Jan 12, 2024 at 10:55:40AM +0200, claudiu beznea wrote:
> Hi, Dan,
> 
> Thanks for your patch!
> 
> On 10.01.2024 20:41, Dan Carpenter wrote:
> > If rzg2l_map_add_config() fails then the error handling calls
> > mutex_unlock(&pctrl->mutex) but we're not holding that mutex.  Move
> > the unlocks to before the gotos to avoid this situation.
> > 
> > Fixes: d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration support for pinmux groups")
> > Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> > ---
> > (Not tested).
> 
> I've tested it on RZ/G3S SoC and all is good.
> 
> However, I think, to keep the locking scheme unchanged and simpler (FMPOV),
> commit d3aaa7203a17 ("pinctrl: renesas: rzg2l: Add pin configuration
> support for pinmux groups") should have been call rzg2l_map_add_config()
> just before the mutex is locked. That would be the following diff:
> 
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -447,6 +447,16 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
> *pctldev,
>  		name = np->name;
>  	}
> 
> +	if (num_configs) {
> +		ret = rzg2l_map_add_config(&maps[idx], name,
> +					   PIN_MAP_TYPE_CONFIGS_GROUP,
> +					   configs, num_configs);
> +		if (ret < 0)
> +			goto done;
> +
> +		idx++;
> +	}
> +
>  	mutex_lock(&pctrl->mutex);
> 
>  	/* Register a single pin group listing all the pins we read from DT */
> @@ -474,16 +484,6 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev
> *pctldev,
>  	maps[idx].data.mux.function = name;
>  	idx++;
        ^^^^^

> 
> -	if (num_configs) {
> -		ret = rzg2l_map_add_config(&maps[idx], name,
> -					   PIN_MAP_TYPE_CONFIGS_GROUP,
> -					   configs, num_configs);
> -		if (ret < 0)
> -			goto remove_group;
> -
> -		idx++;
> -	}

Does the ordering of the maps[] not matter?

> -
>  	dev_dbg(pctrl->dev, "Parsed %pOF with %d pins\n", np, num_pinmux);
>  	ret = 0;
>  	goto done;
> 
> Would you mind doing it like this?
> 
> Please, let me know if you want me to handle it.

Either way is fine.  Whatever is easiest.

regards,
dan carpenter