Bug 620368

This issue has been proposed when we are only considering blocker
issues in the current Red Hat Enterprise Linux release.

** If you would still like this issue considered for the current
release, ask your support representative to file as a blocker on
your behalf. Otherwise ask that it be considered for the next
Red Hat Enterprise Linux release. **

(In reply to comment #0)
> Description of problem:
> 
> service cgconfig:
> 1) no pid file when cgconfig is running
> 2) pid file exists, but service not started (Expected 1, got 3)

There is no deamon running, therefore there is no pid file.

cgconfig service just creates the groups on service start. It does not start any daemon which would need a pid file.

> 3) lock file exists, but service not started (Expected 2, got 0)

dtto, there is no daemon running, which we could check. The lock file is the only way, how to see if the service was started or not. If you remove the lock file, the service looses its only indicator and it assumes it was not started.

> 4) Insufficient rights, like user `testcgconfigqa', restarting resrvice under
> nonprivileged user must fail (Expected 4, got 1)

It is impossible to do it in init script. cgconfig service does not know, if it failed because of insufficient privileges or because e.g. the config file cannot be parsed. Many other services fail similar way, IMHO exit code 1 is acceptable in this case.

> 5) Testing proper return code when nonexisting function, like `service cgconfig
> nonexistfunc' (Expected 2, got 1)
> 6) When no arguments added to service, it must fail with proper return code 
> (Expected 2, got 1)

I'll fix those two.


> service cgred:
> 1) Insufficient rights, restarting resrvice under nonprivileged user must fail
> (Expected 4, got 1)

same as cgconfig service... 1 is IMHO acceptable.

> 2) Testing proper return code when nonexisting function (Expected 2, got 0)
> 3) When no arguments added to service, it must fail with proper return code 
> (Expected 2, got 0)

I'll fix those two.

Created attachment 436189 [details]
patch against insufficient privilege use to run service

(In reply to comment #3)
> 
> > 3) lock file exists, but service not started (Expected 2, got 0)
> 
> dtto, there is no daemon running, which we could check. The lock file is the
> only way, how to see if the service was started or not. If you remove the lock
> file, the service looses its only indicator and it assumes it was not started.

OK, I'll remove this test for now.

> 
> > 4) Insufficient rights, like user `testcgconfigqa', restarting resrvice under
> > nonprivileged user must fail (Expected 4, got 1)
> 
> It is impossible to do it in init script. cgconfig service does not know, if it
> failed because of insufficient privileges or because e.g. the config file
> cannot be parsed. Many other services fail similar way, IMHO exit code 1 is
> acceptable in this case.

How about the attached patch? check the current user id before run the service. These codes can be found in rdma and some other service packages.

> > service cgred:
> > 1) Insufficient rights, restarting resrvice under nonprivileged user must fail
> > (Expected 4, got 1)
> 
> same as cgconfig service... 1 is IMHO acceptable.

ditto. the patch attached.

Thanks,
Caspar

(In reply to comment #4)
> (In reply to comment #3)
> > > 4) Insufficient rights, like user `testcgconfigqa', restarting resrvice under
> > > nonprivileged user must fail (Expected 4, got 1)
> > 
> > It is impossible to do it in init script. cgconfig service does not know, if it
> > failed because of insufficient privileges or because e.g. the config file
> > cannot be parsed. Many other services fail similar way, IMHO exit code 1 is
> > acceptable in this case.
> 
> How about the attached patch? check the current user id before run the service.
> These codes can be found in rdma and some other service packages.

This could work in the old good times, where only root could mount filesystems. Now we have extended capabilities - not only root can mount things. In addition, we have SELinux - sometimes even root itself can not mount :(. Checking for UID=0 does not work in modern Linux :(.

My patches were accepted upstream, git commit be13bad0e41d8debde1e0c3c3345ee1f5158b72f

With the current libcgroup-0.37-1.el6 build and some updates to selinux policy we should be seeing built in a few days, we are now down to this failure:

::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::
:: [   LOG    ] :: cgred start - bad-configured
::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::

:: [   FAIL   ] :: Try start when cgconfig not running (Expected 7, got 1)
:: [   PASS   ] :: Running 'service cgred status'
:: [   PASS   ] :: Running 'rm -f /etc/cgrules.conf'
:: [   PASS   ] :: Try start when /etc/cgrules.conf not exist
:: [   PASS   ] :: Running 'service cgred status'
:: [   LOG    ] :: Duration: 1s
:: [   LOG    ] :: Assertions: 4 good, 1 bad
:: [   FAIL   ] :: RESULT: cgred start - bad-configured

in start() of the initscript we have:

        if ! grep "^cgroup" /proc/mounts &>/dev/null; then
                echo
                log_failure_msg $"Cannot find cgroups, is cgconfig service running?"
                return 1
        fi

When I read the LSB docs it seems to say that we should return 7 here (program is not running) where 1 is a generic error code.

 I am a bit concerned about that code because if some program other than cgconfig mounts the cgroup filesystem and happens to use the same name of cgroup then cgrulesengd will still start and might cause undesireable behavior if the groups it expects to see in /etc/cgrules.conf aren't actually there. I've confirmed the user can't confuse things with the mount command since those apperantly don't go into /proc/mounts so I don't know if we should really worry about this or not.

(In reply to comment #8)
> in start() of the initscript we have:
> 
>         if ! grep "^cgroup" /proc/mounts &>/dev/null; then
>                 echo
>                 log_failure_msg $"Cannot find cgroups, is cgconfig service
> running?"
>                 return 1
>         fi
> 
> When I read the LSB docs it seems to say that we should return 7 here (program
> is not running) where 1 is a generic error code.

Well, it depends which 'program' the LSB doc means. If you look at the LSB description of exit codes of 'service xxx status', 'program' clearly means the daemon which the service starts, therefore I think that 'program' refers to cgrulesengd in this context. And the scripts checks status of cgconfig service, not cgrulesengd.

I would expect exit code 7 e.g. on service cgred reload and the daemon not running.


>  I am a bit concerned about that code because if some program other than
> cgconfig mounts the cgroup filesystem and happens to use the same name of
> cgroup then cgrulesengd will still start and might cause undesireable behavior
> if the groups it expects to see in /etc/cgrules.conf aren't actually there.
> I've confirmed the user can't confuse things with the mount command since those
> apperantly don't go into /proc/mounts so I don't know if we should really worry
> about this or not.

cgrulesengd does not care who mounts the cgroups, if it is cgconfig service, different service or the user. It does not need cgconfig.conf at all, it finds the mount points by itself. I think my (simple) check is fine here.

I am confused regarding mounts created manually by mount command 'apperantly don't go into /proc/mounts'. Can you reproduce it? If so, it's serious kernel bug. *All* mounted filesystem, incl. cgroups, must be visible there, regardless how they were mounted. On the other hand, /etc/mtab may miss some, especially those mounted directly by applications (=cgconfig service) and not by /sbin/mount program.

(-> Moving back to ON_QA. Don't hesitate to move it back to me if you think I am wrong :)

re: comment 9.. I think I misspoke... meant to say whatever's in /proc/mounts wasn't necessarily getting in mtab/getting displayed by the mount command.. which is OK for things that get mounted by programs other than mount.

I think we do have an issue with the kernel somewhere where a cgroup doesn't always umount properly, but that's a separate issue that might affect libcgroup services but doesn't necessarily indicate the bug lies with libcgroup.

At any rate, I'll re-test once we get a beta tree.

Since no one feels strongly either way about what the return code should be (so long as it isn't zero) for comment 8. I think we can go ahead and consider this one fixed and the beaker test can be updated to allow 1 as a valid return code for that case.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0577.html