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

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

[Tejun Heo]

Hello, Linus.

* cgroup.procs listing related fixes. It didn't interlock properly
  with exiting tasks leaving a short window where a cgroup has empty
  cgroup.procs but still can't be removed and misbehaved on short
  reads.

* psi_show() crash fix on 32bit ino archs.

* Empty release_agent handling fix.

Thanks.

The following changes since commit 0bf999f9c5e74c7ecf9dafb527146601e5c848b9:

  linux/pipe_fs_i.h: fix kernel-doc warnings after @wait was split (2020-02-12 11:54:08 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-5.6-fixes

for you to fetch changes up to 2e5383d7904e60529136727e49629a82058a5607:

  cgroup1: don't call release_agent when it is "" (2020-03-04 11:53:33 -0500)

----------------------------------------------------------------
Michal Koutný (1):
      cgroup: Iterate tasks that did not finish do_exit()

Qian Cai (1):
      cgroup: fix psi_show() crash on 32bit ino archs

Tycho Andersen (1):
      cgroup1: don't call release_agent when it is ""

Vasily Averin (2):
      cgroup-v1: cgroup_pidlist_next should update position index
      cgroup: cgroup_procs_next should increase position index

 include/linux/cgroup.h    |  1 +
 kernel/cgroup/cgroup-v1.c |  3 ++-
 kernel/cgroup/cgroup.c    | 39 ++++++++++++++++++++++++++-------------
 3 files changed, 29 insertions(+), 14 deletions(-)

-- 
tejun

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

[Tejun Heo]

Hello, Linus.

* cgroup.procs listing related fixes. It didn't interlock properly
  with exiting tasks leaving a short window where a cgroup has empty
  cgroup.procs but still can't be removed and misbehaved on short
  reads.

* psi_show() crash fix on 32bit ino archs.

* Empty release_agent handling fix.

Thanks.

The following changes since commit 0bf999f9c5e74c7ecf9dafb527146601e5c848b9:

  linux/pipe_fs_i.h: fix kernel-doc warnings after @wait was split (2020-02-12 11:54:08 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-5.6-fixes

for you to fetch changes up to 2e5383d7904e60529136727e49629a82058a5607:

  cgroup1: don't call release_agent when it is "" (2020-03-04 11:53:33 -0500)

----------------------------------------------------------------
Michal Koutný (1):
      cgroup: Iterate tasks that did not finish do_exit()

Qian Cai (1):
      cgroup: fix psi_show() crash on 32bit ino archs

Tycho Andersen (1):
      cgroup1: don't call release_agent when it is ""

Vasily Averin (2):
      cgroup-v1: cgroup_pidlist_next should update position index
      cgroup: cgroup_procs_next should increase position index

 include/linux/cgroup.h    |  1 +
 kernel/cgroup/cgroup-v1.c |  3 ++-
 kernel/cgroup/cgroup.c    | 39 ++++++++++++++++++++++++++-------------
 3 files changed, 29 insertions(+), 14 deletions(-)

-- 
tejun

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

[Linus Torvalds]

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))

yeah, I really hope that the compiler is smart enough to just optimize
that, but we shouldn't assume that the compiler is that smart.

The proper way to test for "empty string" is to just check the first
character for NUL:

        if (!pathbuf || !agentbuf || !*agentbuf)

which doesn't require the compiler to have a pattern for "oh, I can
test for a zero strlen without actually calling strlen".

Also, wouldn't it be nice to test for the empty string before you even
bother to kstrdup() it? Even before you

Finally, shouldn't we technically hold the release_agent_path_lock
while looking at it?

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?

                 Linus

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

[Linus Torvalds]

On Tue, Mar 10, 2020 at 7:41 AM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> * Empty release_agent handling fix.

Pulled. However, I gagged a bit when I saw the code:

        if (!pathbuf || !agentbuf || !strlen(agentbuf))

yeah, I really hope that the compiler is smart enough to just optimize
that, but we shouldn't assume that the compiler is that smart.

The proper way to test for "empty string" is to just check the first
character for NUL:

        if (!pathbuf || !agentbuf || !*agentbuf)

which doesn't require the compiler to have a pattern for "oh, I can
test for a zero strlen without actually calling strlen".

Also, wouldn't it be nice to test for the empty string before you even
bother to kstrdup() it? Even before you

Finally, shouldn't we technically hold the release_agent_path_lock
while looking at it?

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?

                 Linus

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

[pr-tracker-bot]

The pull request you sent on Tue, 10 Mar 2020 10:41:07 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-5.6-fixes

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e941484541036ba3652b658ed5536c7bca5bdb89

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

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

[pr-tracker-bot-DgEjT+Ai2ygdnm+yROfE0A]

The pull request you sent on Tue, 10 Mar 2020 10:41:07 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-5.6-fixes

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e941484541036ba3652b658ed5536c7bca5bdb89

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

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

[Tejun Heo]

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

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

[Tejun Heo]

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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