Coding style questions for patches..

[Valdis . Kletnieks]

I'm currently updating the security patches I posted a while ago, and have a
few questions (a *partial* sample patch is appended - the dangling parts not
shown are why I'm posting this.. ;)

1) Is the patch below more acceptable than the original, where I had the #ifdef
in the middle of the kernel/pid.c code?

Currently, the #ifdef is done via security/Kconfig (this part ISN'T included
below).  The problem is that currently, the variable security_enable_randpid
lives in a new security/*.o file that gets built.  It's easy enough to make
this all work as part of a large patch that provides an LSM as well, but the
problem is that I'd like to push this one (and several other things that
similarly can't be done via LSM) for inclusion.

2) Where should the 'extern int' actually be if this is a separate patch?
security/Kconfig makes sense to me for the CONFIG variables, but I suspect that
the various flags need to live someplace (I'm currently looking at 1 for PIDs
below, and on the order of 4 to 5 'int' flags in the TCP/UDP code, and one in
the RPC code....)

3) Currently, the LSM file does all the sysctl setup - should I break that into
a part that has the variables used by the LSM, and another patch against kernel/
sysctl.c that does the non-LSM flags?

3a) Alternatively, do people think I should just punt the sysctl support, in
which case I can just lose the variable and all the associated worry?

3b) If no sysctl support, should I have support for setting it at boot time?

4) I have a series of 4 or 5 similar patches which are *logically* independent
(for instance, randomizing the TCP ephemeral source port doesn't depend on the
below patch for randomizing process IDs).  However, for things like sysctl.c,
the code for the patches will end up close enough to cause merging problems.

Should I submit 2 patches, both against a clean source tree, knowing that the
second won't apply if the first is applied?  Or should I submit the first patch
against the clean tree, and a second that deltas off that, knowing that it
won't apply if the first one *isn't* accepted?

Straw-man sample code for illustration:

--- include/linux/pid.h.vt	2003-09-27 20:50:13.000000000 -0400
+++ include/linux/pid.h	2004-06-12 15:29:02.507247676 -0400
@@ -61,4 +61,22 @@ extern void switch_exec_pids(struct task
 			elem = elem->next, prefetch(elem->next), 	\
 			task = pid_task(elem, type))
 
+#ifdef CONFIG_SECURITY_RANDPID
+#include <linux/random.h>
+extern int security_enable_randpid;
+
+static inline int alloc_next_pid(int last_pid) {
+	int next;
+	if (security_enable_randpid && (last_pid >= RESERVED_PIDS)) {
+		pid = (get_random_long() % (pid_max - RESERVED_PIDS)) + RESERVED_PIDS + 1;
+	}
+	else next = last_pid + 1;
+	return next;
+}
+#else
+static inline int alloc_next_pid(int last_pid) {
+	return last_pid + 1;
+}
+#endif
+
 #endif /* _LINUX_PID_H */
--- kernel/pid.c.vt	2004-06-10 00:37:49.000000000 -0400
+++ kernel/pid.c	2004-06-12 15:26:24.148058356 -0400
@@ -102,7 +102,7 @@ int alloc_pidmap(void)
 	int pid, offset, max_steps = PIDMAP_ENTRIES + 1;
 	pidmap_t *map;
 
-	pid = last_pid + 1;
+	pid = alloc_next_pid(last_pid);
 	if (pid >= pid_max)
 		pid = RESERVED_PIDS;

Attachment: pgp00000.pgp
Description: PGP signature