linux-kernel - Re: [heads-up] Re: [PATCH] reset: Further simplify locking with guard()

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: <97b2b218-b46e-41a8-9379-09b76569f3f1@linaro.org>
Date: Sun, 29 Sep 2024 20:48:49 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Al Viro <viro@...iv.linux.org.uk>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 kernel@...gutronix.de, Linus Torvalds <torvalds@...ux-foundation.org>,
 Peter Zijlstra <peterz@...radead.org>
Subject: Re: [heads-up] Re: [PATCH] reset: Further simplify locking with
 guard()

On 29/09/2024 00:27, Al Viro wrote:
> On Fri, Sep 27, 2024 at 04:02:32PM +0200, Philipp Zabel wrote:
>> Use guard(mutex) to automatically unlock mutexes when going out of
>> scope. Simplify error paths by removing a goto and manual mutex
>> unlocking in multiple places.
> 
> And that, folks, is a live example of the reasons why guard() is an
> attractive nuisance.  We really need a very loud warning on
> cleanup.h stuff - otherwise such patches from well-meaning folks
> will keep coming.
> 
>> @@ -1041,29 +1036,27 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
>>  		}
>>  	}
>>  
>> -	mutex_lock(&reset_list_mutex);
>> +	guard(mutex)(&reset_list_mutex);
>>  	rcdev = __reset_find_rcdev(&args, gpio_fallback);
>>  	if (!rcdev) {
>>  		rstc = ERR_PTR(-EPROBE_DEFER);
>> -		goto out_unlock;
>> +		goto out_put;
>>  	}
>>  
>>  	if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
>>  		rstc = ERR_PTR(-EINVAL);
>> -		goto out_unlock;
>> +		goto out_put;
>>  	}
>>  
>>  	rstc_id = rcdev->of_xlate(rcdev, &args);
>>  	if (rstc_id < 0) {
>>  		rstc = ERR_PTR(rstc_id);
>> -		goto out_unlock;
>> +		goto out_put;
>>  	}
>>  
>>  	/* reset_list_mutex also protects the rcdev's reset_control list */
>>  	rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
>>  
>> -out_unlock:
>> -	mutex_unlock(&reset_list_mutex);
>>  out_put:
>>  	of_node_put(args.np);
> 
> Guess what happens if you take goto out_put prior to the entire thing,
> in
>                 ret = __reset_add_reset_gpio_device(&args);
> 		if (ret) {
> 			rstc = ERR_PTR(ret);
> 			goto out_put;
> 		}
> That patch adds implicit mutex_unlock() at the points where we leave
> the scope.  Which extends to the end of function.  In other words, there is
> one downstream of out_put, turning any goto out_put upstream of guard() into
> a bug.
> 

cleanup.h also mentions that one should do not mix cleanup with existing
goto, because of possibility of above issue.

But except careful review, this patch should have been simply compile
tested which would point to the issue above. Any guard/scope works must
be checked with clang W=1, which reports jumps over init.

Best regards,
Krzysztof