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>

1 files changed, 47 insertions, 60 deletions

diff --git a/src/daemon/cgrulesengd.c b/src/daemon/cgrulesengd.c
index 4e00e18..74d12e1 100644
--- 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;
 }