I have been testing a cgrulesengd daemon and I noticed it fails to

change the cgroup of child occasionally. I tested it by following
configulation file:

/etc/cgrules.conf:
    user01          cpuset          group01/user01
    %               memory          group01/user01

A cpuset subsystem and a memory subsystem are mounted on different
mount points, and a cgrulesengd daemon manages each subsystem.
I login this environment as a user "user01", and each susbystem's
tasks file is the following:

 # cat /mnt/cgroups/cpuset/group01/user01/tasks
 31801
 31805
 31806
 #
 # cat /mnt/cgroups/memory/group01/user01/tasks
 31801
 31805
 #
 # pstree -p 32105
 sshd(31801)---sshd(31805)---bash(31806)
 #

They should be the same, but they are different. I investigated this
problem, and I found the cause. The reason is that the process(31806)
was forked just after writing the process(31805) to a cpuset subsystem's
tasks file:

 <1> The UID/GID CHANGE event of the process 31805 happens.
 <2> The daemon writes "31805" to a cpuset subsystem's tasks file.
 <3> The process 31806 is forked, and it appears on a cpuset subsystem's
     tasks file.
 <4> The daemon writes "31805" to a memory subsystem's tasks file.
 <5> The process 31806 does not appears on a memory subsystem's tasks file.

For solving this problem, I propose the following sequence.
 1. Store both the timestamp and the process-id when the step <4>.
 2. If receiving a PROC_EVENT_FORK packet, check its parent-pid and its
    timestamp.
 3. If its parent-pid and the stored process-id are same and its timestamp
    is older than the stored timestamp, change the cgroup of forked process.

Changelog of v2:
 * Change only [PATCH 2/2] and there is not any changes in [PATCH 1/2].

This patch adds the method for getting euid/egid from /proc/<pid>/status
file.

For changing the cgroup of a forked process, the method is usefull because
a PROC_EVENT_FORK packet does not inform of its euid and its egid.

Signed-off-by: Ken'ichi Ohmichi <oomichi@mxs.nes.nec.co.jp>
Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>

diff --git a/src/daemon/cgrulesengd.c b/src/daemon/cgrulesengd.c
index 4e00e184cb67d4f99c4c3c481d301db038a028da..74d12e12b70bdb535514fbaa5ac5771e9ccadd74 100644 (file)
--- a/src/daemon/cgrulesengd.c
+++ b/src/daemon/cgrulesengd.c
@@ -132,15 +132,7 @@ void flog(int level, const char *format, ...)
        }
 }
 
-/**
- * Process an event from the kernel, and determine the correct UID/GID/PID to
- * pass to libcgroup.  Then, libcgroup will decide the cgroup to move the PID
- * to, if any.
- *     @param ev The event to process
- *     @param type The type of event to process (part of ev)
- *     @return 0 on success, > 0 on failure
- */
-int cgre_process_event(const struct proc_event *ev, const int type)
+static int cgre_get_euid_egid_from_status(pid_t pid, uid_t *euid, gid_t *egid)
 {
        /* Handle for the /proc/PID/status file */
        FILE *f;
@@ -149,19 +141,13 @@ int cgre_process_event(const struct proc_event *ev, const int type)
        char path[FILENAME_MAX];
 
        /* Temporary buffer */
-       char *buf = NULL;
+       char buf[4092];
 
        /* UID data */
-       uid_t ruid, euid, suid, fsuid, log_uid = 0;
+       uid_t ruid, suid, fsuid;
 
        /* GID data */
-       gid_t rgid, egid, sgid, fsgid, log_gid = 0;
-
-       /* PID, just for logging */
-       pid_t log_pid = 0;
-
-       /* Return codes */
-       int ret = 0;
+       gid_t rgid, sgid, fsgid;
 
        /*
         * First, we need to open the /proc/PID/status file so that we can
@@ -169,76 +155,79 @@ int cgre_process_event(const struct proc_event *ev, const int type)
         * on.  This process is probably not us, so we can't just call
         * geteuid() or getegid().
         */
-       sprintf(path, "/proc/%d/status", ev->event_data.id.process_pid);
+       sprintf(path, "/proc/%d/status", pid);
        f = fopen(path, "r");
        if (!f) {
                flog(LOG_WARNING, "Failed to open %s", path);
-               goto finished;
+               return 1;
        }
 
-       /* Now, we need to find either the eUID or the eGID of the process. */
-       buf = calloc(4096, sizeof(char));
-       if (!buf) {
-               flog(LOG_WARNING, "Failed to process event, out of"
-                               "memory?  Error: %s",
-                               strerror(errno));
-               ret = errno;
-               fclose(f);
-               goto finished;
+       /* Have the eUID, need to find the eGID. */
+       memset(buf, '\0', sizeof(buf));
+       while (fgets(buf, sizeof(buf), f)) {
+               if (!strncmp(buf, "Uid:", 4)) {
+                       sscanf((buf + 5), "%d%d%d%d", &ruid, euid,
+                               &suid, &fsuid);
+                       break;
+               } else if (!strncmp(buf, "Gid:", 4)) {
+                       sscanf((buf + 5), "%d%d%d%d", &rgid, egid,
+                               &sgid, &fsgid);
+                       break;
+               }
+               memset(buf, '\0', sizeof(buf));
        }
+       fclose(f);
+       return 0;
+}
+
+/**
+ * Process an event from the kernel, and determine the correct UID/GID/PID to
+ * pass to libcgroup.  Then, libcgroup will decide the cgroup to move the PID
+ * to, if any.
+ *     @param ev The event to process
+ *     @param type The type of event to process (part of ev)
+ *     @return 0 on success, > 0 on failure
+ */
+int cgre_process_event(const struct proc_event *ev, const int type)
+{
+       pid_t pid = 0, log_pid = 0;
+       uid_t euid, log_uid = 0;
+       gid_t egid, log_gid = 0;
+
+       int ret = 0;
+
        switch (type) {
        case PROC_EVENT_UID:
-               /* Have the eUID, need to find the eGID. */
-               while (fgets(buf, 4096, f)) {
-                       if (!strncmp(buf, "Gid:", 4)) {
-                               sscanf((buf + 5), "%d%d%d%d", &rgid, &egid,
-                                       &sgid, &fsgid);
-                               break;
-                       }
-                       memset(buf, '\0', 4096);
-               }
-               break;
        case PROC_EVENT_GID:
-               /* Have the eGID, need to find the eUID. */
-               while (fgets(buf, 4096, f)) {
-                       if (!strncmp(buf, "Uid:", 4)) {
-                               sscanf((buf + 5), "%d%d%d%d", &ruid, &euid,
-                                       &suid, &fsuid);
-                               break;
-                       }
-                       memset(buf, '\0', 4096);
-               }
+               pid = ev->event_data.id.process_pid;
                break;
        default:
-               flog(LOG_WARNING, "For some reason, we're processing a"
-                               " non-UID/GID event. Something is wrong!");
                break;
        }
-       free(buf);
-       fclose(f);
+       if (cgre_get_euid_egid_from_status(pid, &euid, &egid))
+               /* cgre_get_euid_egid_from_status() returns 1 if it fails to
+                * open /proc/<pid>/status file and that is not a problem. */
+               return 0;
 
        /*
         * Now that we have the UID, the GID, and the PID, we can make a call
         * to libcgroup to change the cgroup for this PID.
         */
+       log_pid = pid;
        switch (type) {
        case PROC_EVENT_UID:
                log_uid = ev->event_data.id.e.euid;
                log_gid = egid;
-               log_pid = ev->event_data.id.process_pid;
                ret = cgroup_change_cgroup_uid_gid_flags(
                                        ev->event_data.id.e.euid,
-                                       egid, ev->event_data.id.process_pid,
-                                       CGFLAG_USECACHE);
+                                       egid, pid, CGFLAG_USECACHE);
                break;
        case PROC_EVENT_GID:
                log_uid = euid;
                log_gid = ev->event_data.id.e.egid;
-               log_pid = ev->event_data.id.process_pid;
                ret = cgroup_change_cgroup_uid_gid_flags(euid,
                                        ev->event_data.id.e.egid,
-                                       ev->event_data.id.process_pid,
-                                       CGFLAG_USECACHE);
+                                       pid, CGFLAG_USECACHE);
                break;
        default:
                break;
@@ -256,8 +245,6 @@ int cgre_process_event(const struct proc_event *ev, const int type)
                flog(LOG_INFO, "Cgroup change for PID: %d, UID: %d, GID: %d OK",
                        log_pid, log_uid, log_gid);
        }
-
-finished:
        return ret;
 }