[libvirt] [PATCH v2 0/2] host-validate: Be more careful when checking for cgroups
Changes from [v1]: * use strchr() instead of strstr() * make sure that the mount type is 'cgroup' before looking for mount options Both suggested by Thomas Huth. Cheers. [v1] https://www.redhat.com/archives/libvir-list/2016-April/msg00136.html Andrea Bolognani (2): host-validate: Be more careful when checking for cgroup support host-validate: Be more careful when checking for cgroup mounts tools/virt-host-validate-common.c | 69 +++++++++++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 13 deletions(-) -- 2.5.5
Simply checking whether the cgroup name appears somewhere inside /proc/self/cgroup is enough most of the time, but there are some corner cases that require a more mindful parsing. As a bonus, after the rewrite 'line' is no longer leaked. --- tools/virt-host-validate-common.c | 49 +++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 8ebf73e..56c6b56 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -265,18 +265,53 @@ static int virHostValidateCGroupSupport(const char *hvname, goto error; while ((ret = getline(&line, &len, fp)) >= 0 && !matched) { - char *tmp = strstr(line, cg_name); - if (!tmp) + char **cgroups; + char *start; + char *end; + size_t ncgroups; + size_t i; + + /* Each line in this file looks like + * + * 4:cpu,cpuacct:/machine.slice/machine-qemu\x2dtest.scope/emulator + * + * Since multiple cgroups can be part of the same line and some cgroup + * names can appear as part of other cgroup names (eg. 'cpu' is a + * prefix for both 'cpuacct' and 'cpuset'), it's not enough to simply + * check whether the cgroup name is present somewhere inside the file + */ + + /* Look for the first colon. + * The part we're interested in starts right after it */ + if (!(start = strchr(line, ':'))) { + VIR_FREE(line); continue; + } + start++; - tmp += strlen(cg_name); - if (*tmp != ',' && - *tmp != ':') + /* Look for the second colon. + * The part we're interested in ends exactly there */ + if (!(end = strchr(start, ':'))) { + VIR_FREE(line); continue; + } + *end = '\0'; - matched = true; + if (!(cgroups = virStringSplitCount(start, ",", 0, &ncgroups))) { + VIR_FREE(line); + continue; + } + + /* Look for the matching cgroup */ + for (i = 0; i < ncgroups; i++) { + if (STREQ(cgroups[i], cg_name)) + matched = true; + } + + virStringFreeListCount(cgroups, ncgroups); + VIR_FREE(line); } - VIR_FREE(line); + VIR_FORCE_FCLOSE(fp); if (!matched) goto error; -- 2.5.5
On Tue, Apr 05, 2016 at 03:08:40PM +0200, Andrea Bolognani wrote:
Simply checking whether the cgroup name appears somewhere inside /proc/self/cgroup is enough most of the time, but there are some corner cases that require a more mindful parsing.
As a bonus, after the rewrite 'line' is no longer leaked. --- tools/virt-host-validate-common.c | 49 +++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 8ebf73e..56c6b56 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -265,18 +265,53 @@ static int virHostValidateCGroupSupport(const char *hvname, goto error;
while ((ret = getline(&line, &len, fp)) >= 0 && !matched) { - char *tmp = strstr(line, cg_name); - if (!tmp) + char **cgroups; + char *start; + char *end; + size_t ncgroups; + size_t i; + + /* Each line in this file looks like + * + * 4:cpu,cpuacct:/machine.slice/machine-qemu\x2dtest.scope/emulator + *
+ * Since multiple cgroups can be part of the same line and some cgroup + * names can appear as part of other cgroup names (eg. 'cpu' is a + * prefix for both 'cpuacct' and 'cpuset'), it's not enough to simply + * check whether the cgroup name is present somewhere inside the file
Also, they could be present in the path.
+ */ + + /* Look for the first colon. + * The part we're interested in starts right after it */ + if (!(start = strchr(line, ':'))) {
+ VIR_FREE(line); continue;
This pattern keeps repeating. How about calling VIR_FREE the first thing in the loop and moving the getline call right after it? ACK either way. Jan
On Tue, 2016-04-05 at 17:26 +0200, Ján Tomko wrote:
+ * Since multiple cgroups can be part of the same line and some cgroup + * names can appear as part of other cgroup names (eg. 'cpu' is a + * prefix for both 'cpuacct' and 'cpuset'), it's not enough to simply + * check whether the cgroup name is present somewhere inside the file Also, they could be present in the path.
Right. I'll clarify that.
+ */ + + /* Look for the first colon. + * The part we're interested in starts right after it */ + if (!(start = strchr(line, ':'))) { + VIR_FREE(line); continue; This pattern keeps repeating. How about calling VIR_FREE the first thing in the loop and moving the getline call right after it?
Actually, after reading getline(3), I realized the leak was never there in the first place. The first time getline() is called, it is passed a NULL pointer for the buffer and 0 for the length, which instructs it to allocate an appropriately-sized buffer itself. Subsequent calls reuse the same buffer, growing it as needed. So we only really need a single VIR_FREE() call, right after the loop - exactly where it was before. Will post a respin shortly. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Simply checking whether the cgroup name appears somewhere inside /proc/self/cgroup is enough most of the time, but there are some corner cases that require a more mindful parsing. --- tools/virt-host-validate-common.c | 43 +++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 95ae64a..e182d0c 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -303,17 +303,48 @@ static int virHostValidateCGroupSupport(const char *hvname, goto error; while ((ret = getline(&line, &len, fp)) >= 0 && !matched) { - char *tmp = strstr(line, cg_name); - if (!tmp) + char **cgroups; + char *start; + char *end; + size_t ncgroups; + size_t i; + + /* Each line in this file looks like + * + * 4:cpu,cpuacct:/machine.slice/machine-qemu\x2dtest.scope/emulator + * + * Since multiple cgroups can be part of the same line and some cgroup + * names can appear as part of other cgroup names (eg. 'cpu' is a + * prefix for both 'cpuacct' and 'cpuset'), it's not enough to simply + * check whether the cgroup name is present somewhere inside the file. + * + * Moreover, there's nothing stopping the cgroup name from appearing + * in an unrelated mount point name as well */ + + /* Look for the first colon. + * The part we're interested in starts right after it */ + if (!(start = strchr(line, ':'))) + continue; + start++; + + /* Look for the second colon. + * The part we're interested in ends exactly there */ + if (!(end = strchr(start, ':'))) continue; + *end = '\0'; - tmp += strlen(cg_name); - if (*tmp != ',' && - *tmp != ':') + if (!(cgroups = virStringSplitCount(start, ",", 0, &ncgroups))) continue; - matched = true; + /* Look for the matching cgroup */ + for (i = 0; i < ncgroups; i++) { + if (STREQ(cgroups[i], cg_name)) + matched = true; + } + + virStringFreeListCount(cgroups, ncgroups); } + VIR_FREE(line); VIR_FORCE_FCLOSE(fp); if (!matched) -- 2.5.5
On Fri, Apr 08, 2016 at 02:13:10PM +0200, Andrea Bolognani wrote:
Simply checking whether the cgroup name appears somewhere inside /proc/self/cgroup is enough most of the time, but there are some corner cases that require a more mindful parsing. --- tools/virt-host-validate-common.c | 43 +++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-)
ACK Jan
On Fri, 2016-04-08 at 18:53 +0200, Ján Tomko wrote:
On Fri, Apr 08, 2016 at 02:13:10PM +0200, Andrea Bolognani wrote:
Simply checking whether the cgroup name appears somewhere inside /proc/self/cgroup is enough most of the time, but there are some corner cases that require a more mindful parsing. --- tools/virt-host-validate-common.c | 43 +++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-)
ACK
Pushed, thanks :) -- Andrea Bolognani Software Engineer - Virtualization Team
The existing code was built with the assumption that no cgroup name could appear as part of another cgroup name. Rewrite it to handle such cases correctly. --- tools/virt-host-validate-common.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 56c6b56..d448e5c 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -340,16 +340,24 @@ static int virHostValidateCGroupMount(const char *hvname, goto error; while (getmntent_r(fp, &ent, mntbuf, sizeof(mntbuf)) && !matched) { - char *tmp = strstr(ent.mnt_opts, cg_name); - if (!tmp) + char **opts; + size_t nopts; + size_t i; + + /* Ignore non-cgroup mounts */ + if (STRNEQ(ent.mnt_type, "cgroup")) continue; - tmp += strlen(cg_name); - if (*tmp != ',' && - *tmp != '\0') + if (!(opts = virStringSplitCount(ent.mnt_opts, ",", 0, &nopts))) continue; - matched = true; + /* Look for a mount option matching the cgroup name */ + for (i = 0; i < nopts; i++) { + if (STREQ(opts[i], cg_name)) + matched = true; + } + + virStringFreeListCount(opts, nopts); } endmntent(fp); if (!matched) -- 2.5.5
On Tue, Apr 05, 2016 at 03:08:41PM +0200, Andrea Bolognani wrote:
The existing code was built with the assumption that no cgroup name could appear as part of another cgroup name.
This is not true - the code checked if there is a comma or a null character after the cgroup name.
Rewrite it to handle such cases correctly. --- tools/virt-host-validate-common.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 56c6b56..d448e5c 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -340,16 +340,24 @@ static int virHostValidateCGroupMount(const char *hvname, goto error;
while (getmntent_r(fp, &ent, mntbuf, sizeof(mntbuf)) && !matched) { - char *tmp = strstr(ent.mnt_opts, cg_name); - if (!tmp) + char **opts; + size_t nopts; + size_t i; +
+ /* Ignore non-cgroup mounts */ + if (STRNEQ(ent.mnt_type, "cgroup")) continue;
This hunk makes sense, but I think it is unlikely to have a mount option matching a cgroup name on a non-cgroup mount. ACK to this hunk. The conversion to virStringSplit is IMO not worth it. Jan
On Tue, 2016-04-05 at 17:32 +0200, Ján Tomko wrote:
On Tue, Apr 05, 2016 at 03:08:41PM +0200, Andrea Bolognani wrote:
The existing code was built with the assumption that no cgroup name could appear as part of another cgroup name. This is not true - the code checked if there is a comma or a null character after the cgroup name.
But not whether there was a comma *before* the cgroup name. So an ipotetic cgroup called 'fakecpu' would match when looking for the 'cpu' cgroup. More importantly, only the first match is taken into account: on my system, the 'cpu' and 'cpuacct' are mounted to the same location and the corresponding mtab entry looks like cgroup /sys/fs/cgroup/cpu,cpuacct \ cgroup rw,nosuid,nodev,noexec,relatime,cpu,cpuacct 0 0 but it's been reported that, on other systems, the 'cpuacct' mount flag is listed *before* the 'cpu' mount flag. When that's the case, strstr() will match the first occurrence of 'cpu', which happens to be that at the beginning of 'cpuacct', and the subsequent check for that match be followed by a comma will fail. To sum up: the commit message is not good enough and should be improved, but the change itself (or a variation thereof) is still needed to make the cgroup mount point detection robust.
+ /* Ignore non-cgroup mounts */ + if (STRNEQ(ent.mnt_type, "cgroup")) continue; This hunk makes sense, but I think it is unlikely to have a mount option matching a cgroup name on a non-cgroup mount.
Yeah, I tend to agree. Still, better safe than sorry, right? And we can avoid calling virStringSplit() if we know we're not processing a cgroup mount. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Tue, Apr 05, 2016 at 06:11:40PM +0200, Andrea Bolognani wrote:
On Tue, 2016-04-05 at 17:32 +0200, Ján Tomko wrote:
On Tue, Apr 05, 2016 at 03:08:41PM +0200, Andrea Bolognani wrote:
The existing code was built with the assumption that no cgroup name could appear as part of another cgroup name. This is not true - the code checked if there is a comma or a null character after the cgroup name.
But not whether there was a comma *before* the cgroup name. So an ipotetic cgroup called 'fakecpu' would match when looking for the 'cpu' cgroup.
More importantly, only the first match is taken into account: on my system, the 'cpu' and 'cpuacct' are mounted to the same location and the corresponding mtab entry looks like
cgroup /sys/fs/cgroup/cpu,cpuacct \ cgroup rw,nosuid,nodev,noexec,relatime,cpu,cpuacct 0 0
but it's been reported that, on other systems, the 'cpuacct' mount flag is listed *before* the 'cpu' mount flag.
When that's the case, strstr() will match the first occurrence of 'cpu', which happens to be that at the beginning of 'cpuacct', and the subsequent check for that match be followed by a comma will fail.
To sum up: the commit message is not good enough and should be improved, but the change itself (or a variation thereof) is still needed to make the cgroup mount point detection robust.
Right. ACK to the patch as-is. Jan
On Fri, 2016-04-08 at 09:18 +0200, Ján Tomko wrote:
To sum up: the commit message is not good enough and should be improved, but the change itself (or a variation thereof) is still needed to make the cgroup mount point detection robust.
Right.
ACK to the patch as-is.
Pushed with an improved commit message. Thanks :) -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Ján Tomko