8:08 a.m.
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
8:08 a.m.
New subject: [libvirt] [PATCH v2 1/2] host-validate: Be more careful when checking for cgroup support
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
10:26 a.m.
New subject: [libvirt] [PATCH v2 1/2] host-validate: Be more careful when checking for cgroup support
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
10:57 a.m.
New subject: [libvirt] [PATCH v2 1/2] host-validate: Be more careful when checking for cgroup support
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
7:13 a.m.
New subject: [libvirt] [PATCH v3] host-validate: Be more careful when checking for cgroup support
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
11:53 a.m.
New subject: [libvirt] [PATCH v3] host-validate: Be more careful when checking for cgroup support
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
2:38 a.m.
New subject: [libvirt] [PATCH v3] host-validate: Be more careful when checking for cgroup support
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
8:08 a.m.
New subject: [libvirt] [PATCH v2 2/2] host-validate: Be more careful when checking for cgroup mounts
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
10:32 a.m.
New subject: [libvirt] [PATCH v2 2/2] host-validate: Be more careful when checking for cgroup mounts
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
11:11 a.m.
New subject: [libvirt] [PATCH v2 2/2] host-validate: Be more careful when checking for cgroup mounts
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
2:18 a.m.
New subject: [libvirt] [PATCH v2 2/2] host-validate: Be more careful when checking for cgroup mounts
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
6:11 a.m.
New subject: [libvirt] [PATCH v2 2/2] host-validate: Be more careful when checking for cgroup mounts
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
3148
days inactive
3154
days old
11 comments
2 participants
participants (2)
-
Andrea Bolognani -
Ján Tomko