cgroup fixes for v4.15-rc7

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

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

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

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

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

From ceb2d2b2e496f180be95adb670337bb254f89323 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
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.

---
 include/linux/string.h | 2 +-
 kernel/cgroup/cgroup.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

index cfd83eb..96115bf 100644
@@ -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 *);

index 2cf06c2..480f2a7 100644
@@ -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

Hello, Linus.

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

Thanks.

tejun

Applied to cgroup/for-4.16.

Thanks.

tejun

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

Hello,

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

Thanks.

tejun

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