Re: [GIT PULL] cgroup fixes for v5.6-rc5

[Tejun Heo <tj@kernel.org>]

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@kernel.org> 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