[PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[Bart Samwel]

Hi Andrew,

Here's a threesome of patches to fix up some issues with the following 
sysctl values:

/proc/sys/vm/laptop_mode
/proc/sys/vm/dirty_writeback_centisecs
/proc/sys/vm/dirty_expire_centisecs

The issues:
1. The values are not range checked when they are set. They all have
a range smaller than the full integer range.
2. Conversion from these centisecond/second values is done on-the-fly 
wherever they are used. This wastes some resources.
3. The conversions are done badly. Conversion from USER_HZ to HZ is done 
by doing "value * USER_HZ / HZ". One day expressed in centiseconds 
already causes an overflow at HZ = 250. This should use 
clock_t_to_jiffies() instead.

The approach:
1. Represent everything in jiffies internally.
2. Do the conversion and range checking in the sysctl interface.

Cheers,
Bart

Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[Andrew Morton]

Bart Samwel <bart@samwel.tk> wrote:
>
>  Here's a threesome of patches
>

All of which were space-stuffed by your (mozilla-derived) email client and
hence are unusable by users of non-MS-wannabe email clients.  They may also
be unusable by users of mozilla-based email clients, too - I don't know.

As far as I know there's no way to prevent mailnews-derived mail clients
from performing space-stuffing.  I've had a bug report in against it for at
least two years and all they've done is fartarse around with it.

IOW: please switch mail clients or use text/plain attachments.

Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[Randy.Dunlap]

On Fri, 27 Jan 2006 19:55:39 -0800 Andrew Morton wrote:

> Bart Samwel <bart@samwel.tk> wrote:
> >
> >  Here's a threesome of patches
> >
> 
> All of which were space-stuffed by your (mozilla-derived) email client and
> hence are unusable by users of non-MS-wannabe email clients.  They may also
> be unusable by users of mozilla-based email clients, too - I don't know.
> 
> As far as I know there's no way to prevent mailnews-derived mail clients
> from performing space-stuffing.  I've had a bug report in against it for at
> least two years and all they've done is fartarse around with it.
> 
> IOW: please switch mail clients or use text/plain attachments.

Someone kept saying that tbird would work and I kept asking how.
What I finally got was:

a.  enable html-formatted email (unintuirive, I had disabled it)
b.  for the body text, select Fixed Width
c.  copy-and-paste the patch

I tested it one time and it worked.
But as long as Andrew accepts text/plain attachments, that
works for him.  Makes it difficult to review them in some
mail clients...

---
~Randy

Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[Nish Aravamudan]

On 1/27/06, Andrew Morton <akpm@osdl.org> wrote:
> Bart Samwel <bart@samwel.tk> wrote:
> >
> >  Here's a threesome of patches
> >
>
> All of which were space-stuffed by your (mozilla-derived) email client and
> hence are unusable by users of non-MS-wannabe email clients.  They may also
> be unusable by users of mozilla-based email clients, too - I don't know.
>
> As far as I know there's no way to prevent mailnews-derived mail clients
> from performing space-stuffing.  I've had a bug report in against it for at
> least two years and all they've done is fartarse around with it.

I think this thread claims that there is a way (although a PITA):
http://lkml.org/lkml/2005/12/27/191

I don't use TBird, personally, but Randy D. seemed satisfied with the
results in that thread.

> IOW: please switch mail clients or use text/plain attachments.

Also viable (and maybe preferred) solutions.

Thanks,
Nish

Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[Paul Jackson]

A third alternative for sending patches, a patch-bomb script,
such as one I maintain:

  http://www.speakeasy.org/~pj99/sgi/sendpatchset

Read the usage in the script for its documentation.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[Bart Samwel]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

Andrew Morton wrote:
> Bart Samwel <bart@samwel.tk> wrote:
>>  Here's a threesome of patches
> 
> All of which were space-stuffed by your (mozilla-derived) email client and
> hence are unusable by users of non-MS-wannabe email clients.  They may also
> be unusable by users of mozilla-based email clients, too - I don't know.

Yes, they are. Damn thunderbird... I've fallen for that trap once before,
thought I'd covered my ass by disabling line wrapping, but apparently that
wasn't all. :-/

Here are the patches as attachments.

--Bart

[-- Attachment #2: laptop_mode_in_jiffies.patch --]
[-- Type: text/plain, Size: 1470 bytes --]


Make that the internal value for /proc/sys/vm/laptop_mode
is stored as jiffies instead of seconds. Let the sysctl
interface do the conversions, instead of doing on-the-fly
conversions every time the value is used.

Add a description of the fact that laptop_mode doubles as a
flag and a timeout to the comment above the laptop_mode variable. 


Signed-off-by: Bart Samwel <bart@samwel.tk>

--- linux-2.6.16-rc1.orig/kernel/sysctl.c	2006-01-28 02:09:32.000000000 +0100
+++ linux-2.6.16-rc1/kernel/sysctl.c	2006-01-28 02:13:10.000000000 +0100
@@ -823,9 +823,8 @@
 		.data		= &laptop_mode,
 		.maxlen		= sizeof(laptop_mode),
 		.mode		= 0644,
-		.proc_handler	= &proc_dointvec,
-		.strategy	= &sysctl_intvec,
-		.extra1		= &zero,
+		.proc_handler	= &proc_dointvec_jiffies,
+		.strategy	= &sysctl_jiffies,
 	},
 	{
 		.ctl_name	= VM_BLOCK_DUMP,
--- linux-2.6.16-rc1.orig/mm/page-writeback.c	2006-01-28 01:47:24.000000000 +0100
+++ linux-2.6.16-rc1/mm/page-writeback.c	2006-01-28 02:11:52.000000000 +0100
@@ -88,7 +88,8 @@
 int block_dump;
 
 /*
- * Flag that puts the machine in "laptop mode".
+ * Flag that puts the machine in "laptop mode". Doubles as a timeout in jiffies:
+ * a full sync is triggered after this time elapses without any disk activity.
  */
 int laptop_mode;
 
@@ -467,7 +468,7 @@
  */
 void laptop_io_completion(void)
 {
-	mod_timer(&laptop_mode_wb_timer, jiffies + laptop_mode * HZ);
+	mod_timer(&laptop_mode_wb_timer, jiffies + laptop_mode);
 }
 
 /*

[-- Attachment #3: check_sysctl_jiffies.patch --]
[-- Type: text/plain, Size: 1043 bytes --]


When (integer) sysctl values are in either seconds or centiseconds,
but represented internally as jiffies, the allowable value range
is decreased. This patch adds range checks to the conversion
routines.

For values in seconds: maximum LONG_MAX / HZ.

For values in centiseconds: maximum (LONG_MAX / HZ) * USER_HZ.


(BTW, does anyone else feel that an interface in seconds should not be
accepting negative values?) 


Signed-off-by: Bart Samwel <bart@samwel.tk>


--- linux-2.6.16-rc1.orig/kernel/sysctl.c	2006-01-28 01:47:24.000000000 +0100
+++ linux-2.6.16-rc1/kernel/sysctl.c	2006-01-28 02:03:14.000000000 +0100
@@ -2008,6 +2008,8 @@
 					 int write, void *data)
 {
 	if (write) {
+		if (*lvalp > LONG_MAX / HZ)
+			return 1;		
 		*valp = *negp ? -(*lvalp*HZ) : (*lvalp*HZ);
 	} else {
 		int val = *valp;
@@ -2029,6 +2031,8 @@
 						int write, void *data)
 {
 	if (write) {
+		if (USER_HZ < HZ && *lvalp > (LONG_MAX / HZ) * USER_HZ)
+			return 1;
 		*valp = clock_t_to_jiffies(*negp ? -*lvalp : *lvalp);
 	} else {
 		int val = *valp;

[-- Attachment #4: dirty_centisecs_jiffies.patch --]
[-- Type: text/plain, Size: 4656 bytes --]



Make that the internal values for:

/proc/sys/vm/dirty_writeback_centisecs
/proc/sys/vm/dirty_expire_centisecs

are stored as jiffies instead of centiseconds. Let the sysctl
interface do the conversions with full precision using
clock_t_to_jiffies, instead of doing overflow-sensitive on-the-fly
conversions every time the values are used.


Cons: apparent precision loss if HZ is not a multiple of 100,
because of conversion back and forth. This is a common problem
for all sysctl values that use proc_dointvec_userhz_jiffies.
(There is only one other in-tree use, in net/core/neighbour.c.)


Signed-off-by: Bart Samwel <bart@samwel.tk>

--- linux-2.6.16-rc1.orig/mm/page-writeback.c	2006-01-28 01:44:32.000000000 +0100
+++ linux-2.6.16-rc1/mm/page-writeback.c	2006-01-28 01:43:25.000000000 +0100
@@ -75,12 +75,12 @@
  * The interval between `kupdate'-style writebacks, in centiseconds
  * (hundredths of a second)
  */
-int dirty_writeback_centisecs = 5 * 100;
+int dirty_writeback_interval = 5 * HZ;
 
 /*
  * The longest number of centiseconds for which data is allowed to remain dirty
  */
-int dirty_expire_centisecs = 30 * 100;
+int dirty_expire_interval = 30 * HZ;
 
 /*
  * Flag that makes the machine dump writes/reads and block dirtyings.
@@ -379,8 +379,8 @@
  * just walks the superblock inode list, writing back any inodes which are
  * older than a specific point in time.
  *
- * Try to run once per dirty_writeback_centisecs.  But if a writeback event
- * takes longer than a dirty_writeback_centisecs interval, then leave a
+ * Try to run once per dirty_writeback_interval.  But if a writeback event
+ * takes longer than a dirty_writeback_interval interval, then leave a
  * one-second gap.
  *
  * older_than_this takes precedence over nr_to_write.  So we'll only write back
@@ -405,9 +405,9 @@
 	sync_supers();
 
 	get_writeback_state(&wbs);
-	oldest_jif = jiffies - (dirty_expire_centisecs * HZ) / 100;
+	oldest_jif = jiffies - dirty_expire_interval;
 	start_jif = jiffies;
-	next_jif = start_jif + (dirty_writeback_centisecs * HZ) / 100;
+	next_jif = start_jif + dirty_writeback_interval;
 	nr_to_write = wbs.nr_dirty + wbs.nr_unstable +
 			(inodes_stat.nr_inodes - inodes_stat.nr_unused);
 	while (nr_to_write > 0) {
@@ -424,7 +424,7 @@
 	}
 	if (time_before(next_jif, jiffies + HZ))
 		next_jif = jiffies + HZ;
-	if (dirty_writeback_centisecs)
+	if (dirty_writeback_interval)
 		mod_timer(&wb_timer, next_jif);
 }
 
@@ -434,11 +434,11 @@
 int dirty_writeback_centisecs_handler(ctl_table *table, int write,
 		struct file *file, void __user *buffer, size_t *length, loff_t *ppos)
 {
-	proc_dointvec(table, write, file, buffer, length, ppos);
-	if (dirty_writeback_centisecs) {
+	proc_dointvec_userhz_jiffies(table, write, file, buffer, length, ppos);
+	if (dirty_writeback_interval) {
 		mod_timer(&wb_timer,
-			jiffies + (dirty_writeback_centisecs * HZ) / 100);
-	} else {
+			jiffies + dirty_writeback_interval);
+		} else {
 		del_timer(&wb_timer);
 	}
 	return 0;
@@ -543,7 +543,7 @@
 		if (vm_dirty_ratio <= 0)
 			vm_dirty_ratio = 1;
 	}
-	mod_timer(&wb_timer, jiffies + (dirty_writeback_centisecs * HZ) / 100);
+	mod_timer(&wb_timer, jiffies + dirty_writeback_interval);
 	set_ratelimit();
 	register_cpu_notifier(&ratelimit_nb);
 }
--- linux-2.6.16-rc1.orig/include/linux/writeback.h	2006-01-28 01:44:32.000000000 +0100
+++ linux-2.6.16-rc1/include/linux/writeback.h	2006-01-28 01:34:03.000000000 +0100
@@ -88,8 +88,8 @@
 /* These are exported to sysctl. */
 extern int dirty_background_ratio;
 extern int vm_dirty_ratio;
-extern int dirty_writeback_centisecs;
-extern int dirty_expire_centisecs;
+extern int dirty_writeback_interval;
+extern int dirty_expire_interval;
 extern int block_dump;
 extern int laptop_mode;
 
--- linux-2.6.16-rc1.orig/kernel/sysctl.c	2006-01-28 01:44:32.000000000 +0100
+++ linux-2.6.16-rc1/kernel/sysctl.c	2006-01-28 01:34:18.000000000 +0100
@@ -717,18 +717,18 @@
 	{
 		.ctl_name	= VM_DIRTY_WB_CS,
 		.procname	= "dirty_writeback_centisecs",
-		.data		= &dirty_writeback_centisecs,
-		.maxlen		= sizeof(dirty_writeback_centisecs),
+		.data		= &dirty_writeback_interval,
+		.maxlen		= sizeof(dirty_writeback_interval),
 		.mode		= 0644,
 		.proc_handler	= &dirty_writeback_centisecs_handler,
 	},
 	{
 		.ctl_name	= VM_DIRTY_EXPIRE_CS,
 		.procname	= "dirty_expire_centisecs",
-		.data		= &dirty_expire_centisecs,
-		.maxlen		= sizeof(dirty_expire_centisecs),
+		.data		= &dirty_expire_interval,
+		.maxlen		= sizeof(dirty_expire_interval),
 		.mode		= 0644,
-		.proc_handler	= &proc_dointvec,
+		.proc_handler	= &proc_dointvec_userhz_jiffies,
 	},
 	{
 		.ctl_name	= VM_NR_PDFLUSH_THREADS,

Re: [PATCH 0/3] Fix overflow issues with sysctl values in centiseconds/seconds

[Ingo Oeser]

Randy Dunlap wrote:
> I tested it one time and it worked.
> But as long as Andrew accepts text/plain attachments, that
> works for him.  Makes it difficult to review them in some
> mail clients...

ReVIEW ist easy. 

Just citing and commenting patches is difficult on mail clients not pasting
together all text/plain attachments.

That's the only technical reason so far.


Regards

Ingo Oeser