[heads-up] Re: [PATCH] reset: Further simplify locking with guard()
From: Al Viro <viro@zeniv.linux.org.uk>
Date: 2024-09-28 22:27:05
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
@@ -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.