[GIT PULL] cgroup fixes for v4.15-rc7

[GIT PULL] cgroup fixes for v4.15-rc7

[Tejun Heo]

Hello, Linus.

Sorry about the late pull request.  I forgot to send them out before
the holidays.

This pull request contains fixes for the following two non-trivial
issues.

* The task iterator got broken while adding thread mode support for
  v4.14.  It was less visible because it only triggers when both
  cgroup1 and cgroup2 hierarchies are in use.  The recent versions of
  systemd uses cgroup2 for process management even when cgroup1 is
  used for resource control exposing this issue.

* cpuset CPU hotplug path could deadlock when racing against exits.

There also are two patches to replace unlimited strcpy() usages with
strlcpy().

Thanks.

The following changes since commit c2f31b79d510ec1a27138bdcf2d0ece1080be85e:

  cgroup: add warning about RT not being supported on cgroup2 (2017-12-05 11:47:17 -0800)

are available in the git repository at:

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

for you to fetch changes up to 74d0833c659a8a54735e5efdd44f4b225af68586:

  cgroup: fix css_task_iter crash on CSS_TASK_ITER_PROC (2017-12-20 07:09:19 -0800)

----------------------------------------------------------------
Arnd Bergmann (1):
      cgroup: use strlcpy() instead of strscpy() to avoid spurious warning

Ma Shimiao (1):
      cgroup: avoid copying strings longer than the buffers

Prateek Sood (1):
      cgroup: Fix deadlock in cpu hotplug path

Tejun Heo (1):
      cgroup: fix css_task_iter crash on CSS_TASK_ITER_PROC

 kernel/cgroup/cgroup-v1.c |  6 +++++-
 kernel/cgroup/cgroup.c    | 20 +++++++++-----------
 2 files changed, 14 insertions(+), 12 deletions(-)

-- 
tejun

Re: [GIT PULL] cgroup fixes for v4.15-rc7

[Linus Torvalds]

On Mon, Jan 8, 2018 at 6:52 AM, Tejun Heo <tj@kernel.org> wrote:
>
> There also are two patches to replace unlimited strcpy() usages with
> strlcpy().

Christ, people.

strlcpy() is crap. It doesn't fix anything. If you have a untrusted
source string, then strlcpy() does "strlen()" on the source, so it
will overrun the source.

And if you don't have an untrusted source, why the hell are you using
that function in the first place?

The interface was designed by morons on crack.

Use "strscpy()", which got this _right_.

We should probably just remove that sh*t-for-brains strlcpy()
function. It's broken garbage.

                Linus

Re: [GIT PULL] cgroup fixes for v4.15-rc7

[Linus Torvalds]

On Mon, Jan 8, 2018 at 11:20 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Use "strscpy()", which got this _right_.

Oh Christ.

It actually did that initially. And then it was changed to the broken
code due to some idiotic broken warning by tools that don't do the
right thing.

              Linus

Re: [GIT PULL] cgroup fixes for v4.15-rc7

[Tejun Heo]

On Mon, Jan 08, 2018 at 11:23:03AM -0800, Linus Torvalds wrote:
> On Mon, Jan 8, 2018 at 11:20 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Use "strscpy()", which got this _right_.
> 
> Oh Christ.
> 
> It actually did that initially. And then it was changed to the broken
> code due to some idiotic broken warning by tools that don't do the
> right thing.

Dang, I was reading the different str*cpy() functions and suggested
strscpy() and then forgot about it and blindly applied strlcpy()
blindly after the build warning report.  It'd be great to weed out the
useless ones.  These are places where truncation is fine.  Would the
right thing to do be adding (void) in front, which is kinda ugly?

Thanks.

-- 
tejun

Re: [GIT PULL] cgroup fixes for v4.15-rc7

[Linus Torvalds]

On Mon, Jan 8, 2018 at 11:28 AM, Tejun Heo <tj@kernel.org> wrote:
>
> Dang, I was reading the different str*cpy() functions and suggested
> strscpy() and then forgot about it and blindly applied strlcpy()
> blindly after the build warning report.  It'd be great to weed out the
> useless ones.  These are places where truncation is fine.  Would the
> right thing to do be adding (void) in front, which is kinda ugly?

Yeah, I don't know. The actual code looks fine, and I pulled it, but I
really hate strlcpy().

Having us have extra warnings for strscpy() that turns people off from
using the right function is all kinds of sad.

I do think KASAN was unhappy about strscpy() too, because of the
word-at-a-time optimization (even if that one is very careful to not
cross pages etc).

Ho humm. I don't know what the right fix is. But I do suspect we
could/should just remove the __must_check from strscpy(), because one
of the points of that function really is that it always does the right
thing regardless of what you pass into it.

But some people absolutely love those idiotic warnings, even when they
demonstrably result in people writing worse code due to them.

Because "not checking error returns is a bug". Bah, humbug.

If just removing the __must_check makes cgroup happy, I would suggest
you send me a patch with that removal and the strlcpy turned back into
strscpy. Because I definitely _will_ apply such a patch.

                Linus

[PATCH] string: drop __must_check from strscpy() and restore strscpy() usages in cgroup

[Tejun Heo]

>From ceb2d2b2e496f180be95adb670337bb254f89323 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Tue, 9 Jan 2018 07:00:29 -0800
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

e7fd37ba1217 ("cgroup: avoid copying strings longer than the buffers")
converted possibly unsafe strncpy() usages in cgroup to strscpy().
However, although the callsites are completely fine with truncated
copied, because strscpy() is marked __must_check, it led to the
following warnings.

  kernel/cgroup/cgroup.c: In function ‘cgroup_file_name’:
  kernel/cgroup/cgroup.c:1400:10: warning: ignoring return value of ‘strscpy’, declared with attribute warn_unused_result [-Wunused-result]
     strscpy(buf, cft->name, CGROUP_FILE_NAME_MAX);
	       ^

To avoid the warnings, 50034ed49645 ("cgroup: use strlcpy() instead of
strscpy() to avoid spurious warning") switched them to strlcpy().

strlcpy() is worse than strlcpy() because it unconditionally runs
strlen() on the source string, and the only reason we switched to
strlcpy() here was because it was lacking __must_check, which doesn't
reflect any material differences between the two function.  It's just
that someone added __must_check to strscpy() and not to strlcpy().

These basic string copy operations are used in variety of ways, and
one of not-so-uncommon use cases is safely handling truncated copies,
where the caller naturally doesn't care about the return value.  The
__must_check doesn't match the actual use cases and forces users to
opt for inferior variants which lack __must_check by happenstance or
spread ugly (void) casts.

Remove __must_check from strscpy() and restore strscpy() usages in
cgroup.

Signed-off-by: Tejun Heo <tj@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
---
 include/linux/string.h | 2 +-
 kernel/cgroup/cgroup.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index cfd83eb..96115bf 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -28,7 +28,7 @@ extern char * strncpy(char *,const char *, __kernel_size_t);
 size_t strlcpy(char *, const char *, size_t);
 #endif
 #ifndef __HAVE_ARCH_STRSCPY
-ssize_t __must_check strscpy(char *, const char *, size_t);
+ssize_t strscpy(char *, const char *, size_t);
 #endif
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 2cf06c2..480f2a7 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1397,7 +1397,7 @@ static char *cgroup_file_name(struct cgroup *cgrp, const struct cftype *cft,
 			 cgroup_on_dfl(cgrp) ? ss->name : ss->legacy_name,
 			 cft->name);
 	else
-		strlcpy(buf, cft->name, CGROUP_FILE_NAME_MAX);
+		strscpy(buf, cft->name, CGROUP_FILE_NAME_MAX);
 	return buf;
 }
 
@@ -1864,9 +1864,9 @@ void init_cgroup_root(struct cgroup_root *root, struct cgroup_sb_opts *opts)
 
 	root->flags = opts->flags;
 	if (opts->release_agent)
-		strlcpy(root->release_agent_path, opts->release_agent, PATH_MAX);
+		strscpy(root->release_agent_path, opts->release_agent, PATH_MAX);
 	if (opts->name)
-		strlcpy(root->name, opts->name, MAX_CGROUP_ROOT_NAMELEN);
+		strscpy(root->name, opts->name, MAX_CGROUP_ROOT_NAMELEN);
 	if (opts->cpuset_clone_children)
 		set_bit(CGRP_CPUSET_CLONE_CHILDREN, &root->cgrp.flags);
 }
-- 
2.9.5

Re: [PATCH] string: drop __must_check from strscpy() and restore strscpy() usages in cgroup

[Tejun Heo]

Hello, Linus.

On Tue, Jan 09, 2018 at 07:21:15AM -0800, Tejun Heo wrote:
> From ceb2d2b2e496f180be95adb670337bb254f89323 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 9 Jan 2018 07:00:29 -0800
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> e7fd37ba1217 ("cgroup: avoid copying strings longer than the buffers")
> converted possibly unsafe strncpy() usages in cgroup to strscpy().
> However, although the callsites are completely fine with truncated
> copied, because strscpy() is marked __must_check, it led to the
> following warnings.

Do you want to pick up this patch?  If not, I can route it through
cgroup tree.

Thanks.

-- 
tejun

Re: [PATCH] string: drop __must_check from strscpy() and restore strscpy() usages in cgroup

[Tejun Heo]

On Tue, Jan 09, 2018 at 07:21:15AM -0800, Tejun Heo wrote:
> From ceb2d2b2e496f180be95adb670337bb254f89323 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 9 Jan 2018 07:00:29 -0800
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit

Applied to cgroup/for-4.16.

Thanks.

-- 
tejun

Re: [PATCH] string: drop __must_check from strscpy() and restore strscpy() usages in cgroup

[Linus Torvalds]

On Fri, Jan 19, 2018 at 8:50 AM, Tejun Heo <tj@kernel.org> wrote:
>
> Applied to cgroup/for-4.16.

I did still have it queued up, but I was looking at other issues, and
hadn't gotten around to it. But if it's now in your git tree I can
just drop this from my queue.

Just as well. Thanks,

               Linus

Re: [PATCH] string: drop __must_check from strscpy() and restore strscpy() usages in cgroup

[Tejun Heo]

Hello,

On Fri, Jan 19, 2018 at 09:27:50AM -0800, Linus Torvalds wrote:
> On Fri, Jan 19, 2018 at 8:50 AM, Tejun Heo <tj@kernel.org> wrote:
> >
> > Applied to cgroup/for-4.16.
> 
> I did still have it queued up, but I was looking at other issues, and
> hadn't gotten around to it. But if it's now in your git tree I can
> just drop this from my queue.

I see.  I'll push it your way together with other cgroup changes once
the merge window opens.

Thanks.

-- 
tejun

Re: [PATCH] string: drop __must_check from strscpy() and restore strscpy() usages in cgroup

[Linus Torvalds]

On Fri, Jan 19, 2018 at 9:31 AM, Tejun Heo <tj@kernel.org> wrote:
>
> I see.  I'll push it your way together with other cgroup changes once
> the merge window opens.

NP. This was a fairly unusual late rc window. It's normally quiet this
period, but first I was distracted by some of the Spectre detail
discussions, and then we had a few other random odd things come in, so
instead of using this week for going through my queues, I was spending
it more on discussion.

But the strscpy patch makes just as much - or more - in the 5.16 merge
window anyway, so it's all good.

              Linus