Hello, Linus.
On Tue, Mar 10, 2020 at 03:14:13PM -0700, Linus Torvalds wrote:
> On Tue, Mar 10, 2020 at 7:41 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
> >
> > * Empty release_agent handling fix.
>
> Pulled. However, I gagged a bit when I saw the code:
>
> if (!pathbuf || !agentbuf || !strlen(agentbuf))
Hahaha, yeah, I can see that. I think it might have been copied from
the commit it refers to - 64e90a8acb85 which contains the following
snippet.
/*
* If there is no binary for us to call, then just return and get out of
* here. This allows us to set STATIC_USERMODEHELPER_PATH to "" and
* disable all call_usermodehelper() calls.
*/
if (strlen(sub_info->path) == 0)
goto out;
> Also, wouldn't it be nice to test for the empty string before you even
> bother to kstrdup() it? Even before you
Let me restructure the code a bit.
> Finally, shouldn't we technically hold the release_agent_path_lock
> while looking at it?
The release_agent_path is protected by both locks - cgroup_mutex and
release_agent_path_lock, so readers can hold either cgroup_mutex or
the path_lock. Here, it's holding the mutex, so it should be fine.
IIRC, it used to be protected by cgroup_mutex (or whatever was
equivalent) and the extra lock was added to break some cyclic
dependency. Hmm... might as well drop the cgroup_mutex protection and
always use the spinlock.
> Small details, and I've taken the pull, but the lack of locking does
> seem to be an actual (if perhaps fairly theoretical) bug, no?
Will queue cleanup patches for the next window.
Thanks.
--
tejun
