[libvirt] [PATCH v2 2/2] host-validate: Be more careful when checking for cgroup mounts

[Andrea Bolognani]

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