[patch 3/4] Refactor do_syslog interface

2006-12-24 Thread Zack Weinberg
This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.  I've added
kerneldoc comments for all the new functions in printk.c.  The control
flow in sys_syslog is different in style but not in substance -- I
like direct returns rather than setting a local variable and returning
at the end; it lets me put a BUG() after the switch statement to catch
missed cases.  [gcc4 at least can optimize it out when there aren't
any.]

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were after the changes in the "map
permissions" patch (i.e. kmsg_close() doesn't do a permission check).

Finally, I fixed some minor bugs. Error and partial read handling in
klog_read/klog_readhist were slightly off: if __put_user returns an
error, that character should not be consumed from the kernel buffer;
if it returns an error after some characters have already been copied
successfully, the read operation should return the count of
already-copied characters, not the error code; if less than the entire
buffer is read with KLOG_READ_CLEAR_HIST, we should only clear the
part that was successfully read.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce
that.

proc/kmsg.c declares the kernel/printk.c interfaces itself, instead of
getting them from klog.h which people want to be purely userspace-
visible constants.  kmsg.c has always had private declarations of
printk.c functions (before, there were declarations of do_syslog and a
wait queue there); as it is unlikely that more users of these
functions will appear, I think this will do fine.  (It might be
reasonable to put declarations in console.h.)

It was possible to prune down the set of headers included by kmsg.c
quite substantially.

zw

Index: linux-2.6/fs/proc/kmsg.c
===
--- linux-2.6.orig/fs/proc/kmsg.c   2006-12-24 11:43:14.0 -0800
+++ linux-2.6/fs/proc/kmsg.c2006-12-24 11:43:18.0 -0800
@@ -5,47 +5,42 @@
  *
  */
 
-#include 
-#include 
-#include 
-#include 
-#include 
 #include 
-#include 
-
-#include 
-#include 
-
-extern wait_queue_head_t log_wait;
+#include 
+#include 
 
-extern int do_syslog(int type, char __user *bug, int count);
+/* interfaces from kernel/printk.c */
+extern int klog_read(char __user *, int, int);
+extern unsigned int klog_poll(struct file *, poll_table *);
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-   return do_syslog(KLOG_OPEN, NULL, 0);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-   (void) do_syslog(KLOG_CLOSE, NULL, 0);
return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 size_t count, loff_t *ppos)
 {
-   if ((file->f_flags & O_NONBLOCK)
-   && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return -EAGAIN;
-   return do_syslog(KLOG_READ, buf, count);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-   poll_wait(file, _wait, wait);
-   if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return POLLIN | POLLRDNORM;
-   return 0;
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===
--- linux-2.6.orig/include/linux/klog.h 2006-12-24 11:43:14.0 -0800
+++ linux-2.6/include/linux/klog.h  2006-12-24 11:43:18.0 -0800
@@ -21,6 +21,8 @@
 
KLOG_GET_UNREAD  =  9, /* return number of unread characters */
KLOG_GET_SIZE= 10, /* return size of log buffer */
+   KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+   * -- new in 2.6.20 */
 
KLOG_OPCODE_MAX
 };
Index: linux-2.6/kernel/printk.c
===
--- linux-2.6.orig/kernel/printk.c  2006-12-24 11:43:16.0 -0800
+++ linux-2.6/kernel/printk.c   2006-12-24 11:43:18.0 -0800
@@ -33,6 +33,7 @@
 #include 

[patch 3/4] Refactor do_syslog interface

2006-12-24 Thread Zack Weinberg
This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.  I've added
kerneldoc comments for all the new functions in printk.c.  The control
flow in sys_syslog is different in style but not in substance -- I
like direct returns rather than setting a local variable and returning
at the end; it lets me put a BUG() after the switch statement to catch
missed cases.  [gcc4 at least can optimize it out when there aren't
any.]

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were after the changes in the map
permissions patch (i.e. kmsg_close() doesn't do a permission check).

Finally, I fixed some minor bugs. Error and partial read handling in
klog_read/klog_readhist were slightly off: if __put_user returns an
error, that character should not be consumed from the kernel buffer;
if it returns an error after some characters have already been copied
successfully, the read operation should return the count of
already-copied characters, not the error code; if less than the entire
buffer is read with KLOG_READ_CLEAR_HIST, we should only clear the
part that was successfully read.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce
that.

proc/kmsg.c declares the kernel/printk.c interfaces itself, instead of
getting them from klog.h which people want to be purely userspace-
visible constants.  kmsg.c has always had private declarations of
printk.c functions (before, there were declarations of do_syslog and a
wait queue there); as it is unlikely that more users of these
functions will appear, I think this will do fine.  (It might be
reasonable to put declarations in console.h.)

It was possible to prune down the set of headers included by kmsg.c
quite substantially.

zw

Index: linux-2.6/fs/proc/kmsg.c
===
--- linux-2.6.orig/fs/proc/kmsg.c   2006-12-24 11:43:14.0 -0800
+++ linux-2.6/fs/proc/kmsg.c2006-12-24 11:43:18.0 -0800
@@ -5,47 +5,42 @@
  *
  */
 
-#include linux/types.h
-#include linux/errno.h
-#include linux/time.h
-#include linux/kernel.h
-#include linux/poll.h
 #include linux/fs.h
-#include linux/klog.h
-
-#include asm/uaccess.h
-#include asm/io.h
-
-extern wait_queue_head_t log_wait;
+#include linux/poll.h
+#include linux/security.h
 
-extern int do_syslog(int type, char __user *bug, int count);
+/* interfaces from kernel/printk.c */
+extern int klog_read(char __user *, int, int);
+extern unsigned int klog_poll(struct file *, poll_table *);
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-   return do_syslog(KLOG_OPEN, NULL, 0);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-   (void) do_syslog(KLOG_CLOSE, NULL, 0);
return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 size_t count, loff_t *ppos)
 {
-   if ((file-f_flags  O_NONBLOCK)
-!do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return -EAGAIN;
-   return do_syslog(KLOG_READ, buf, count);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_read(buf, count, !(file-f_flags  O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-   poll_wait(file, log_wait, wait);
-   if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return POLLIN | POLLRDNORM;
-   return 0;
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===
--- linux-2.6.orig/include/linux/klog.h 2006-12-24 11:43:14.0 -0800
+++ linux-2.6/include/linux/klog.h  2006-12-24 11:43:18.0 -0800
@@ -21,6 +21,8 @@
 
KLOG_GET_UNREAD  =  9, /* return number of unread characters */
KLOG_GET_SIZE= 10, /* return size of log buffer */
+   KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+   * -- new in 2.6.20 */
 
KLOG_OPCODE_MAX
 };
Index: linux-2.6/kernel/printk.c
===
--- linux-2.6.orig/kernel/printk.c  

[patch 3/4] Refactor do_syslog interface

2006-12-14 Thread Zack Weinberg
This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were with one exception: neither
kmsg_close() nor sys_syslog(KLOG_CLOSE, ...) calls security_syslog
at all anymore (close operations should never fail).

Finally, I fixed a couple of minor bugs.  __put_user error handling in
klog_read was slightly off: if __put_user returns an error, that
character should not be consumed from the kernel buffer; if it returns
an error after some characters have already been copied successfully,
the read operation should return the count of already-copied
characters, not the error code.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce that.

Change from previous version of patch: proc/kmsg.c declares the
kernel/printk.c interfaces itself, instead of getting them from klog.h
which people want to be purely userspace-visible constants.  kmsg.c has
always had private declarations of printk.c functions (before, there were
declarations of do_syslog and a wait queue there); as it is unlikely that
more users of these functions will appear, I think this will do fine.
(It might be reasonable to put declarations in console.h.)

zw

Index: linux-2.6/fs/proc/kmsg.c
===
--- linux-2.6.orig/fs/proc/kmsg.c   2006-12-13 16:04:46.0 -0800
+++ linux-2.6/fs/proc/kmsg.c2006-12-13 16:36:56.0 -0800
@@ -12,40 +12,43 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
 
-extern wait_queue_head_t log_wait;
-
-extern int do_syslog(int type, char __user *bug, int count);
+/* interfaces from kernel/printk.c */
+extern int klog_read(char __user *, int, int);
+extern unsigned int klog_poll(struct file *, poll_table *);
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-   return do_syslog(KLOG_OPEN,NULL,0);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-   (void) do_syslog(KLOG_CLOSE,NULL,0);
return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 size_t count, loff_t *ppos)
 {
-   if ((file->f_flags & O_NONBLOCK)
-   && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return -EAGAIN;
-   return do_syslog(KLOG_READ, buf, count);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-   poll_wait(file, _wait, wait);
-   if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return POLLIN | POLLRDNORM;
-   return 0;
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===
--- linux-2.6.orig/include/linux/klog.h 2006-12-13 16:12:43.0 -0800
+++ linux-2.6/include/linux/klog.h  2006-12-13 16:33:09.0 -0800
@@ -20,7 +20,9 @@
* printed to console */
 
KLOG_GET_UNREAD  =  9, /* return number of unread characters */
-   KLOG_GET_SIZE= 10  /* return size of log buffer */
+   KLOG_GET_SIZE= 10, /* return size of log buffer */
+   KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+   * -- new in 2.6.20 */
 };
 
 #endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===
--- linux-2.6.orig/kernel/printk.c  2006-12-13 16:08:30.0 -0800
+++ linux-2.6/kernel/printk.c   2006-12-13 16:39:24.0 -0800
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -45,7 +46,7 @@
 #define MINIMUM_CONSOLE_LOGLEVEL 1 /* Minimum loglevel we let people use */
 #define DEFAULT_CONSOLE_LOGLEVEL 7 /* anything MORE serious than KERN_DEBUG */
 
-DECLARE_WAIT_QUEUE_HEAD(log_wait);
+static DECLARE_WAIT_QUEUE_HEAD(log_wait);
 
 int console_printk[4] = {
DEFAULT_CONSOLE_LOGLEVEL,   /* console_loglevel */
@@ -164,116 +165,142 @@
 
 __setup("log_buf_len=", 

[patch 3/4] Refactor do_syslog interface

2006-12-14 Thread Zack Weinberg
This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were with one exception: neither
kmsg_close() nor sys_syslog(KLOG_CLOSE, ...) calls security_syslog
at all anymore (close operations should never fail).

Finally, I fixed a couple of minor bugs.  __put_user error handling in
klog_read was slightly off: if __put_user returns an error, that
character should not be consumed from the kernel buffer; if it returns
an error after some characters have already been copied successfully,
the read operation should return the count of already-copied
characters, not the error code.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce that.

Change from previous version of patch: proc/kmsg.c declares the
kernel/printk.c interfaces itself, instead of getting them from klog.h
which people want to be purely userspace-visible constants.  kmsg.c has
always had private declarations of printk.c functions (before, there were
declarations of do_syslog and a wait queue there); as it is unlikely that
more users of these functions will appear, I think this will do fine.
(It might be reasonable to put declarations in console.h.)

zw

Index: linux-2.6/fs/proc/kmsg.c
===
--- linux-2.6.orig/fs/proc/kmsg.c   2006-12-13 16:04:46.0 -0800
+++ linux-2.6/fs/proc/kmsg.c2006-12-13 16:36:56.0 -0800
@@ -12,40 +12,43 @@
 #include linux/poll.h
 #include linux/fs.h
 #include linux/klog.h
+#include linux/security.h
 
 #include asm/uaccess.h
 #include asm/io.h
 
-extern wait_queue_head_t log_wait;
-
-extern int do_syslog(int type, char __user *bug, int count);
+/* interfaces from kernel/printk.c */
+extern int klog_read(char __user *, int, int);
+extern unsigned int klog_poll(struct file *, poll_table *);
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-   return do_syslog(KLOG_OPEN,NULL,0);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-   (void) do_syslog(KLOG_CLOSE,NULL,0);
return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 size_t count, loff_t *ppos)
 {
-   if ((file-f_flags  O_NONBLOCK)
-!do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return -EAGAIN;
-   return do_syslog(KLOG_READ, buf, count);
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_read(buf, count, !(file-f_flags  O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-   poll_wait(file, log_wait, wait);
-   if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-   return POLLIN | POLLRDNORM;
-   return 0;
+   int error = security_syslog(LSM_KLOG_READ);
+   if (error)
+   return error;
+   return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===
--- linux-2.6.orig/include/linux/klog.h 2006-12-13 16:12:43.0 -0800
+++ linux-2.6/include/linux/klog.h  2006-12-13 16:33:09.0 -0800
@@ -20,7 +20,9 @@
* printed to console */
 
KLOG_GET_UNREAD  =  9, /* return number of unread characters */
-   KLOG_GET_SIZE= 10  /* return size of log buffer */
+   KLOG_GET_SIZE= 10, /* return size of log buffer */
+   KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+   * -- new in 2.6.20 */
 };
 
 #endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===
--- linux-2.6.orig/kernel/printk.c  2006-12-13 16:08:30.0 -0800
+++ linux-2.6/kernel/printk.c   2006-12-13 16:39:24.0 -0800
@@ -33,6 +33,7 @@
 #include linux/syscalls.h
 #include linux/jiffies.h
 #include linux/klog.h
+#include linux/poll.h
 
 #include asm/uaccess.h
 
@@ -45,7 +46,7 @@
 #define MINIMUM_CONSOLE_LOGLEVEL 1 /* Minimum loglevel we let people use */
 #define DEFAULT_CONSOLE_LOGLEVEL 7 /* anything MORE serious than KERN_DEBUG */
 
-DECLARE_WAIT_QUEUE_HEAD(log_wait);
+static DECLARE_WAIT_QUEUE_HEAD(log_wait);
 
 int