Re: [PATCH] mm: prevent divide error for small values of vm_dirty_bytes

[Andrea Righi <righi.andrea@gmail.com>]

On Wed, Apr 29, 2009 at 02:46:55PM -0700, Andrew Morton wrote:
> On Wed, 29 Apr 2009 11:34:51 +0200
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > --- a/Documentation/sysctl/vm.txt
> > +++ b/Documentation/sysctl/vm.txt
> > @@ -90,6 +90,10 @@ will itself start writeback.
> >  If dirty_bytes is written, dirty_ratio becomes a function of its value
> >  (dirty_bytes / the amount of dirtyable system memory).
> >  
> > +Note: the minimum value allowed for dirty_bytes is two pages (in bytes); any
> > +value lower than this limit will be ignored and the old configuration will be
> > +retained.
> 
> Well.  This implies that the write to the procfs file would appear to
> succeed.  One hopes that the write would in fact return -EINVAL or
> such?

I definitely agree. Just tested the following patch and it looks much
better with the error code.

-Andrea

---
sysctl: return error code if values are not within a valid range

Currently __do_proc_doulongvec_minmax(), as well as
__do_proc_dointvec(), simply skip the invalid values instead of return
-EINVAL.

A more correct behaviour is to report to the userspace that some values
were invalid and they couldn't be written instead of silently drop
them.

For example (vm_dirty_bytes must be greater or equal than 2*PAGE_SIZE):
- before:
  # cat /proc/sys/vm/dirty_bytes
  0
  # /bin/echo 1 > /proc/sys/vm/dirty_bytes
  # cat /proc/sys/vm/dirty_bytes
  0
  # /bin/echo 8192 > /proc/sys/vm/dirty_bytes
  # cat /proc/sys/vm/dirty_bytes
  8192

- after:
  # cat /proc/sys/vm/dirty_bytes
  0
  # /bin/echo 1 > /proc/sys/vm/dirty_bytes
  /bin/echo: write error: Invalid argument
  # cat /proc/sys/vm/dirty_bytes
  0
  # /bin/echo 8192 > /proc/sys/vm/dirty_bytes
  # cat /proc/sys/vm/dirty_bytes
  8192

As a bonus do few minor coding style fixup.

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 kernel/sysctl.c |   47 +++++++++++++++++++++++++++++++----------------
 1 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ea78fa1..92e56cf 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2243,19 +2243,19 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 		  void *data)
 {
 #define TMPBUFLEN 21
-	int *i, vleft, first=1, neg, val;
+	int *i, vleft, first = 1, neg, val, ret = 0;
 	unsigned long lval;
 	size_t left, len;
-	
+
 	char buf[TMPBUFLEN], *p;
 	char __user *s = buffer;
-	
+
 	if (!tbl_data || !table->maxlen || !*lenp ||
 	    (*ppos && !write)) {
 		*lenp = 0;
 		return 0;
 	}
-	
+
 	i = (int *) tbl_data;
 	vleft = table->maxlen / sizeof(*i);
 	left = *lenp;
@@ -2288,26 +2288,31 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 				neg = 1;
 				p++;
 			}
-			if (*p < '0' || *p > '9')
+			if (*p < '0' || *p > '9') {
+				ret = -EINVAL;
 				break;
+			}
 
 			lval = simple_strtoul(p, &p, 0);
 
 			len = p-buf;
-			if ((len < left) && *p && !isspace(*p))
+			if ((len < left) && *p && !isspace(*p)) {
+				ret = -EINVAL;
 				break;
+			}
 			if (neg)
 				val = -val;
 			s += len;
 			left -= len;
 
-			if (conv(&neg, &lval, i, 1, data))
+			ret = conv(&neg, &lval, i, 1, data);
+			if (ret)
 				break;
 		} else {
 			p = buf;
 			if (!first)
 				*p++ = '\t';
-	
+
 			if (conv(&neg, &lval, i, 0, data))
 				break;
 
@@ -2339,6 +2344,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 	}
 	if (write && first)
 		return -EINVAL;
+	if (write && ret)
+		return ret;
 	*lenp -= left;
 	*ppos += *lenp;
 	return 0;
@@ -2477,23 +2484,23 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 {
 #define TMPBUFLEN 21
 	unsigned long *i, *min, *max, val;
-	int vleft, first=1, neg;
+	int vleft, first = 1, neg, ret = 0;
 	size_t len, left;
 	char buf[TMPBUFLEN], *p;
 	char __user *s = buffer;
-	
+
 	if (!data || !table->maxlen || !*lenp ||
 	    (*ppos && !write)) {
 		*lenp = 0;
 		return 0;
 	}
-	
+
 	i = (unsigned long *) data;
 	min = (unsigned long *) table->extra1;
 	max = (unsigned long *) table->extra2;
 	vleft = table->maxlen / sizeof(unsigned long);
 	left = *lenp;
-	
+
 	for (; left && vleft--; i++, min++, max++, first=0) {
 		if (write) {
 			while (left) {
@@ -2519,12 +2526,16 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 				neg = 1;
 				p++;
 			}
-			if (*p < '0' || *p > '9')
+			if (*p < '0' || *p > '9') {
+				ret = -EINVAL;
 				break;
+			}
 			val = simple_strtoul(p, &p, 0) * convmul / convdiv ;
 			len = p-buf;
-			if ((len < left) && *p && !isspace(*p))
+			if ((len < left) && *p && !isspace(*p)) {
+				ret = -EINVAL;
 				break;
+			}
 			if (neg)
 				val = -val;
 			s += len;
@@ -2532,8 +2543,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 
 			if(neg)
 				continue;
-			if ((min && val < *min) || (max && val > *max))
-				continue;
+			if ((min && val < *min) || (max && val > *max)) {
+				ret = -EINVAL;
+				break;
+			}
 			*i = val;
 		} else {
 			p = buf;
@@ -2567,6 +2580,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 	}
 	if (write && first)
 		return -EINVAL;
+	if (write && ret)
+		return ret;
 	*lenp -= left;
 	*ppos += *lenp;
 	return 0;