Process Events Connector

This patch adds a connector that reports fork, exec, id change, and
exit
events for all processes to userspace.

...

include/linux/cn_proc.h new file +16 -0

@@ -0,0 +1,125 @@
+/*
+ * cn_proc.h - process events connector
+ *
+ * Copyright (C) Matt Helsley, IBM Corp. 2005
+ * Based on cn_fork.h by Nguyen Anh Quynh and Guillaume Thouvenin
+ * Original copyright notice follows:
+ * Copyright (C) 2005 Nguyen Anh Quynh <[email protected]>
+ * Copyright (C) 2005 Guillaume Thouvenin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

include/linux/cn_proc.h new file +56 -0

+/*
+ * From the user's point of view, the process
+ * ID is the thread group ID and thread ID is the internal
+ * kernel "pid". So, fields are assigned as follow:
+ *
+ *  In user space     -  In  kernel space
+ *
+ * parent process ID  =  parent->tgid
+ * parent thread  ID  =  parent->pid
+ * child  process ID  =  child->tgid
+ * child  thread  ID  =  child->pid
+ */
+
+typedef int __bitwise proc_event_what_t;
+struct proc_event {
+	enum what {
+		/* Use successive bits so the enums can be used to record
+		 * sets of events as well
+		 */
+		PROC_EVENT_NONE = (__force proc_event_what_t)0x00000000,
+		PROC_EVENT_FORK = (__force proc_event_what_t)0x00000001,
+		PROC_EVENT_EXEC = (__force proc_event_what_t)0x00000002,
+		PROC_EVENT_UID  = (__force proc_event_what_t)0x00000004,
+		PROC_EVENT_GID  = (__force proc_event_what_t)0x00000040,
+		/* "next" should be 0x00000400 */
+		/* "last" is the last process event: exit */
+		PROC_EVENT_EXIT = (__force proc_event_what_t)0x80000000
+	} what;
+	int cpu;
+	union { /* must be last field of proc_event struct */
+		struct {
+			int err;
+		} ack;
+
+		struct fork_proc_event {
+			pid_t parent_pid;
+			pid_t parent_tgid;
+			pid_t child_pid;
+			pid_t child_tgid;
+		} fork;
+
+		struct exec_proc_event {
+			pid_t process_pid;
+			pid_t process_tgid;
+		} exec;
+
+		struct id_proc_event {
+			pid_t process_pid;
+			pid_t process_tgid;
+			union {
+				uid_t ruid; /* current->uid */
+				gid_t rgid; /* current->gid */
+			};
+			union {
+				uid_t euid;
+				gid_t egid;

+static inline void get_seq (__u32 *ts, int *cpu)

+static void cn_proc_ack (int err, int rcvd_seq, int rcvd_ack)

include/linux/cn_proc.h new file +31 -0

+	msg = (struct cn_msg*)buffer;
+	ev = (struct proc_event*)msg->data;
+
+	msg->seq = rcvd_seq;
+	ev->cpu = -1;
+	ev->what = PROC_EVENT_NONE;
+	ev->ack.err = err;
+	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
+	msg->ack = rcvd_ack + 1;
+	msg->len = sizeof(*ev);
+	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
+}
+
...
+/**
+ * cn_proc_mcast_ctl
+ * @data: message sent from userspace via the connector
+ */
+static void cn_proc_mcast_ctl(void *data)
+{
+	struct cn_msg *msg = data;
+	enum proc_cn_mcast_op *mc_op = NULL;
+	int err = 0;
+
+	if (!(cn_already_initialized &&
+	    (msg->len == sizeof(*mc_op))))
+		return;
+
+	mc_op = (enum proc_cn_mcast_op*)msg->data;
+	switch (*mc_op) {
+		case PROC_CN_MCAST_LISTEN:
+			atomic_inc(&proc_event_num_listeners);

include/linux/cn_proc.h new file +6 -0

+/*
+ * cn_proc_init - initialization entry point
+ *
+ * Adds the connector callback to the connector driver.
+ */
+static int __init cn_proc_init(void)

+__initcall(cn_proc_init);

Quoted 4 lines · Matt Helsley

This patch adds a connector that reports fork, exec, id change, and
exit
events for all processes to userspace.

Looks sane enough to me. There are of course hooks, but they're minimal.

Quoted 20 lines

...

include/linux/cn_proc.h new file +16 -0

@@ -0,0 +1,125 @@
+/*
+ * cn_proc.h - process events connector
+ *
+ * Copyright (C) Matt Helsley, IBM Corp. 2005
+ * Based on cn_fork.h by Nguyen Anh Quynh and Guillaume Thouvenin
+ * Original copyright notice follows:
+ * Copyright (C) 2005 Nguyen Anh Quynh <[email protected]>
+ * Copyright (C) 2005 Guillaume Thouvenin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

include/linux/cn_proc.h line 26

Quoted 10 lines

include/linux/cn_proc.h new file +10 -0

+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef CN_PROC_H
+#define CN_PROC_H 1

We normally just do

#define CN_PROC_H

include/linux/cn_proc.h line 35

Quoted 8 lines

include/linux/cn_proc.h new file +8 -0

+#include <linux/types.h>
+#include <linux/connector.h>
+
+typedef int __bitwise proc_cn_mcast_op_t;
+enum proc_cn_mcast_op {
+	PROC_CN_MCAST_LISTEN = (__force proc_cn_mcast_op_t)1,
+	PROC_CN_MCAST_IGNORE = (__force proc_cn_mcast_op_t)2
+};

Why is the __bitwise thingy in here?

Given that you've carefully used `#ifdef __KERNEL__' below, I assume that this definition is supposed to be shared with userspace, yes? If so, what is userspace to make of __bitwise and __force?

Please remove the typedef - just use enum proc_cn_mcast_op' in those places where the code requires, umm, an enum proc_cn_mcast_op'.

Quoted 61 lines

include/linux/cn_proc.h new file +61 -0

+/*
+ * From the user's point of view, the process
+ * ID is the thread group ID and thread ID is the internal
+ * kernel "pid". So, fields are assigned as follow:
+ *
+ *  In user space     -  In  kernel space
+ *
+ * parent process ID  =  parent->tgid
+ * parent thread  ID  =  parent->pid
+ * child  process ID  =  child->tgid
+ * child  thread  ID  =  child->pid
+ */
+
+typedef int __bitwise proc_event_what_t;
+struct proc_event {
+	enum what {
+		/* Use successive bits so the enums can be used to record
+		 * sets of events as well
+		 */
+		PROC_EVENT_NONE = (__force proc_event_what_t)0x00000000,
+		PROC_EVENT_FORK = (__force proc_event_what_t)0x00000001,
+		PROC_EVENT_EXEC = (__force proc_event_what_t)0x00000002,
+		PROC_EVENT_UID  = (__force proc_event_what_t)0x00000004,
+		PROC_EVENT_GID  = (__force proc_event_what_t)0x00000040,
+		/* "next" should be 0x00000400 */
+		/* "last" is the last process event: exit */
+		PROC_EVENT_EXIT = (__force proc_event_what_t)0x80000000
+	} what;
+	int cpu;
+	union { /* must be last field of proc_event struct */
+		struct {
+			int err;
+		} ack;
+
+		struct fork_proc_event {
+			pid_t parent_pid;
+			pid_t parent_tgid;
+			pid_t child_pid;
+			pid_t child_tgid;
+		} fork;
+
+		struct exec_proc_event {
+			pid_t process_pid;
+			pid_t process_tgid;
+		} exec;
+
+		struct id_proc_event {
+			pid_t process_pid;
+			pid_t process_tgid;
+			union {
+				uid_t ruid; /* current->uid */
+				gid_t rgid; /* current->gid */
+			};
+			union {
+				uid_t euid;
+				gid_t egid;
+			};
+		} id;
+
+		struct exit_proc_event {
+			pid_t process_pid;

include/linux/cn_proc.h line 102

Quoted 5 lines

include/linux/cn_proc.h new file +5 -0

+			pid_t process_tgid;
+			int exit_code, exit_signal;
+		} exit;
+	};
+};

We avoid using anonymous unions because older gcc's do not support them. Please give this union a name.

This structure is supposed to be passed to userspace, yes? Are you sure that it doesn't need conversion for 64-bit kernel/32-bit userspace?

include/linux/cn_proc.h line 105

Quoted 2 lines

include/linux/cn_proc.h new file +2 -0

+#ifdef __KERNEL__
+#if defined(CONFIG_PROC_EVENTS) || defined(CONFIG_PROC_EVENTS_MODULE)

I don't think this code can possibly work as a module??

include/linux/cn_proc.h line 109

Quoted 4 lines

include/linux/cn_proc.h new file +4 -0

+void proc_fork_connector(struct task_struct *task);
+void proc_exec_connector(struct task_struct *task);
+void proc_id_connector (struct task_struct *task, int which_id);
+void proc_exit_connector(struct task_struct *task);

Coding style nit: three of the above are correct, one isn't.

Quoted 4 lines

include/linux/cn_proc.h new file +4 -0

+#else
+static inline void proc_fork_connector(struct task_struct *task)
+{}
+

include/linux/cn_proc.h line 119

Quoted 6 lines

include/linux/cn_proc.h new file +6 -0

+static inline void proc_exec_connector(struct task_struct *task)
+{}
+
+static inline void proc_id_connector  (struct task_struct *task,
+				       int which_id)
+{}

Two spaces this time ;) Zero is correct.

include/linux/cn_proc.h

Quoted 5 lines

...
+ * cn_proc.c - process events connector
...

include/linux/cn_proc.h new file +2 -0

+
+extern int cn_already_initialized;

Please never put extern decls in .c files. Put it in a header file which is shared by the definition and by all users.

include/linux/cn_proc.h

Quoted 5 lines

include/linux/cn_proc.h new file +5 -0

+static atomic_t proc_event_num_listeners = 0;
+struct cb_id cn_proc_event_id = { CN_IDX_PROC, CN_VAL_PROC };
+
+/* proc_counts is used as the sequence number of the netlink message */
+DEFINE_PER_CPU(__u32, proc_event_counts);

It used to be the case that DEFINE_PER_CPU had to include an initialisation, to work around a compiler/toolchain bug in older gcc's. I have a vague feeling that this got fixed, but we cannot remember how. It would be safest here to do

DEFINE_PER_CPU(__u32, proc_event_counts) = { 0 };

I think proc_event_counts can have static scope?

Quoted 1 line

+static inline void get_seq (__u32 *ts, int *cpu)

Coding style.

Quoted 1 line

+static void cn_proc_ack (int err, int rcvd_seq, int rcvd_ack)

Coding style.

include/linux/cn_proc.h

Quoted 7 lines

include/linux/cn_proc.h new file +7 -0

+{
+	struct cn_msg *msg;
+	struct proc_event *ev;
+	__u8 buffer[CN_PROC_MSG_SIZE];
+
+	if (atomic_read(proc_event_num_listeners) < 1)
+		return;

What happens if there's a race, and proc_event_num_listeners falls to zero right here?

Quoted 36 lines

include/linux/cn_proc.h new file +35 -0

+	msg = (struct cn_msg*)buffer;
+	ev = (struct proc_event*)msg->data;
+
+	msg->seq = rcvd_seq;
+	ev->cpu = -1;
+	ev->what = PROC_EVENT_NONE;
+	ev->ack.err = err;
+	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
+	msg->ack = rcvd_ack + 1;
+	msg->len = sizeof(*ev);
+	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
+}
+
...
+/**
+ * cn_proc_mcast_ctl
+ * @data: message sent from userspace via the connector
+ */
+static void cn_proc_mcast_ctl(void *data)
+{
+	struct cn_msg *msg = data;
+	enum proc_cn_mcast_op *mc_op = NULL;
+	int err = 0;
+
+	if (!(cn_already_initialized &&
+	    (msg->len == sizeof(*mc_op))))
+		return;
+
+	mc_op = (enum proc_cn_mcast_op*)msg->data;
+	switch (*mc_op) {
+		case PROC_CN_MCAST_LISTEN:
+			atomic_inc(&proc_event_num_listeners);
+			break;
+		case PROC_CN_MCAST_IGNORE:
+			atomic_dec(&proc_event_num_listeners);
+			break;

include/linux/cn_proc.h

Quoted 6 lines

include/linux/cn_proc.h new file +6 -0

+		default:
+			err = EINVAL;
+			break;
+	}
+	cn_proc_ack(err, msg->seq, msg->ack);
+}

We usually indent the body of a switch statement one tabstop the left.

Quoted 10 lines

include/linux/cn_proc.h new file +10 -0

+/*
+ * cn_proc_init - initialization entry point
+ *
+ * Adds the connector callback to the connector driver.
+ */
+static int __init cn_proc_init(void)
+{
+	int err;
+
+	if ((err = cn_add_callback(&cn_proc_event_id, "cn_proc",

include/linux/cn_proc.h

Quoted 6 lines

include/linux/cn_proc.h new file +6 -0

+	 			   &cn_proc_mcast_ctl))) {
+		printk(KERN_WARNING "cn_proc failed to register\n");
+		return err;
+	}
+	return 0;
+}

Is KERN_WARNING sufficiently stern?

include/linux/cn_proc.h

Quoted 4 lines

include/linux/cn_proc.h new file +4 -0

+static void __exit cn_proc_fini(void)
+{
+	cn_del_callback(&cn_proc_event_id);
+}

This has no callers?

Quoted 1 line

+__initcall(cn_proc_init);

Using __initcall is a bit old-fashioned. module_init() here would be more typical.

include/linux/connector.h line 32

Quoted 13 lines

Index: linux-2.6.14-rc4/include/linux/connector.h
===================================================================

include/linux/connector.h +3 -0

@@ -25,10 +25,13 @@
 #include <asm/types.h>
 
 #define CN_IDX_CONNECTOR		0xffffffff
 #define CN_VAL_CONNECTOR		0xffffffff
 
+#define CN_IDX_PROC                    0x1
+#define CN_VAL_PROC                    0x1
+

It'd be nice to have a comment explaining what these are.

include/linux/connector.h line 35

Quoted 3 lines

include/linux/connector.h +3 -0

+config PROC_EVENTS
+	boolean "Report process events to userspace"
+	depends on CONNECTOR=y

Yup, I agree - we can forget about module support.

Quoted 9 lines

include/linux/connector.h +0 -0

 cn-y				+= cn_queue.o connector.o

Index: linux-2.6.14-rc4/fs/exec.c
===================================================================

fs/exec.c +0 -0

@@ -49,10 +49,11 @@
 #include <linux/rmap.h>
 #include <linux/acct.h>
 

fs/exec.c line 55

Quoted 4 lines

fs/exec.c +1 -0

 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
+#include <linux/cn_proc.h>
 

We normally place the linux/ includes before the asm/ includes.

Quoted 10 lines

Index: linux-2.6.14-rc4/kernel/fork.c
===================================================================

kernel/fork.c +0 -0

@@ -48,10 +48,12 @@
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 

kernel/fork.c line 54

Quoted 2 lines

kernel/fork.c +2 -0

+#include <linux/cn_proc.h>
+

Ditto.

Quoted 4 lines

kernel/sys.c +0 -0

@@ -34,10 +34,12 @@
 

kernel/sys.c line 40

Quoted 6 lines

kernel/sys.c +2 -0

 #include <asm/uaccess.h>
 #include <asm/io.h>
 #include <asm/unistd.h>
 
+#include <linux/cn_proc.h>
+

Ditto.

The Process Events Connector uses this symbol to determine if it
should respond to commands from userspace. However the it fails to link without the EXPORT_SYMBOL_GPL() macro.

Signed-off-by Matt Helsley <matthltc @ us.ibm.com>

--

Resent with the subject line fixed.

---

Index: linux-2.6.14-rc4/drivers/connector/connector.c
===================================================================

drivers/connector/connector.c +1 -0

@@ -45,10 +45,11 @@ static DECLARE_MUTEX(notify_lock);
 static LIST_HEAD(notify_list);
 
 static struct cn_dev cdev;
 
 int cn_already_initialized = 0;
+EXPORT_SYMBOL_GPL(cn_already_initialized);
 
 /*
  * msg->seq and msg->ack are used to determine message genealogy.
  * When someone sends message it puts there locally unique sequence
  * and random acknowledge numbers.  Sequence number may be copied into

Andrew, all,

Is there any reason this patch could not go for a spin in a -mm tree?
It's similar to Guillaume's fork connector patch which did appear in -mm at one point. It replaces the fork_advisor patch that ELSA is currently using, can be used by userspace CKRM code, and in general is useful for anything that may wish to monitor changes in all processes.

Quoted 7 lines · Matt Helsley

Andrew, all,

Is there any reason this patch could not go for a spin in a -mm tree?
It's similar to Guillaume's fork connector patch which did appear in -mm at one point. It replaces the fork_advisor patch that ELSA is currently using, can be used by userspace CKRM code, and in general is useful for anything that may wish to monitor changes in all processes.

Why can't you use a lsm module for this instead? It looks like you are wanting to hook things in pretty much the same places we currently have the lsm hooks at.

thanks,

greg k-h

Quoted 13 lines · Greg KH

Quoted 7 lines · Matt Helsley

Andrew, all,

Is there any reason this patch could not go for a spin in a -mm tree?
It's similar to Guillaume's fork connector patch which did appear in -mm at one point. It replaces the fork_advisor patch that ELSA is currently using, can be used by userspace CKRM code, and in general is useful for anything that may wish to monitor changes in all processes.

Why can't you use a lsm module for this instead? It looks like you are wanting to hook things in pretty much the same places we currently have the lsm hooks at.

Quoted 3 lines

thanks,

greg k-h

Guillaume apparently tried to use LSM for his fork connector and was
told "this doesn't belong here":

http://www.ussg.iu.edu/hypermail/linux/kernel/0502.2/1000.html

This patch does not affect whether or not these operations succeed and
hence is a poor match for LSM even though it hooks into the same places in the kernel.

There has been some discussion on lse-tech about 'task_notifiers' that
would allow multiple modules to hook into these paths without polluting the paths themselves. I modified the patch with these proposals in mind. Then, assuming such an interface developed, I could submit a small patch which would convert to using the new interface.

Would you still rather see the patch as an LSM module?

Thanks,
-Matt Helsley
< matthltc @ us.ibm.com >

This patch does not affect whether or not these operations succeed and
hence is a poor match for LSM even though it hooks into the same places in the kernel.

Quoted 3 lines · * Matt Helsley ([email protected]) wrote:

This patch does not affect whether or not these operations succeed and
hence is a poor match for LSM even though it hooks into the same places in the kernel.

That's correct. There's no access control happening here, so it's a poor fit. There's all that ELSA, PAGG, CKRM, etc that want this kind of stuff. I'd recommended numerous times to find a common piece for those users.

thanks,
-chris

It seems to me that this is the consensus here and on LSE-Tech.
This patch addresses the needs of ELSA and CKRM and is amenable to using the patches recently proposed on lse-tech to pull out the common piece.

Quoted 3 lines · * Matt Helsley ([email protected]) wrote:

It seems to me that this is the consensus here and on LSE-Tech.
This patch addresses the needs of ELSA and CKRM and is amenable to using the patches recently proposed on lse-tech to pull out the common piece.

Sounds good. What about the SGI needs (for PAGG)? They just posted
pnotify pretty recently. Or is that what you mean by consensus and
possible use of 'task notifiers'?

thanks,
-chris

Quoted 2 lines · Chris Wright

* Matt Helsley ([email protected]) wrote:

Quoted 8 lines

Quoted 4 lines

It seems to me that this is the consensus here and on LSE-Tech.
This patch addresses the needs of ELSA and CKRM and is amenable to using the patches recently proposed on lse-tech to pull out the common piece.

Sounds good. What about the SGI needs (for PAGG)? They just posted
pnotify pretty recently. Or is that what you mean by consensus and
possible use of 'task notifiers'?

Quoted 3 lines

thanks,
-chris

If this patch used pnotify it would be much more complicated and it
would need to attach small pieces of data to each task. However there have been some alternative proposals. The 'task_notifier' patch posted by Jack Steiner was much closer to what this patch needs:
http://marc.theaimsgroup.com/?l=lse-tech&m=112869558116290&w=2

'task_notifier' is smaller and easier to review but still requires
per-task data that would complicate the Process Events Connector patch. There has been some discussion in the above thread on how to address the per-task data/notification needs of PAGG and the all-task notification needs of ELSA and CKRM.

Cheers,
-Matt Helsley

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
ckrm-tech mailing list
https://lists.sourceforge.net/lists/listinfo/ckrm-tech