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

[Al Viro]

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.

What's worse, that bug is not caught by gcc - it quietly generates bogus code
that will get unnoticed until we get an error from __reset_add_reset_gpio_device()
call.  At that point we'll look at the contents of uninitialized variable and,
if we are unlucky, call mutex_unlock() (with hell knows what pointer passed to it -
not that mutex_unlock(&reset_list_mutex) would do us any good at that point, since
we hadn't locked it in the first place).

Folks, that kind of cleanup patches is bloody dangerous; even something that
currently avoids that crap can easily grow that kind of quiet breakage later.