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

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] [day] [month] [year] [list]

Message-ID: <20200312192534.GI79873@mtj.duckdns.org>
Date:   Thu, 12 Mar 2020 15:25:34 -0400
From:   Tejun Heo <tj@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Cgroups <cgroups@...r.kernel.org>
Subject: Re: [GIT PULL] cgroup fixes for v5.6-rc5

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@...nel.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