sysinfo compatibility

Hi Eric and Alan,

sysinfo does use a new mem_unit field if ram+swap > MAX_ULONG. That breaks 2.2 compatibility for a lot machines.

I think it is more resonable to use the mem_unit field only if one of ram or swap is bigger than MAX_ULONG. (And 2.2 was only broken in that case)

The appended patch implements that (and makes the logic a little bit easier)

Greetings
Christoph

diff -uNr 2.4.9/kernel/info.c 2.4.9-sysinfo/kernel/info.c
@@ -16,6 +16,7 @@
 asmlinkage long sys_sysinfo(struct sysinfo *info)
 {
 	struct sysinfo val;
+	unsigned int mem_unit;
 
 	memset((char *)&val, 0, sizeof(struct sysinfo));
 

@@ -32,47 +33,36 @@
 	si_meminfo(&val);
 	si_swapinfo(&val);
 
-	{
-		unsigned long mem_total, sav_total;
-		unsigned int mem_unit, bitcount;
-
-		/* If the sum of all the available memory (i.e. ram + swap)
-		 * is less than can be stored in a 32 bit unsigned long then
-		 * we can be binary compatible with 2.2.x kernels.  If not,
-		 * well, in that case 2.2.x was broken anyways...
-		 *
-		 *  -Erik Andersen <[email protected]> */
-
-		mem_total = val.totalram + val.totalswap;
-		if (mem_total < val.totalram || mem_total < val.totalswap)
-			goto out;
-		bitcount = 0;
-		mem_unit = val.mem_unit;
-		while (mem_unit > 1) {
-			bitcount++;
-			mem_unit >>= 1;
-			sav_total = mem_total;
-			mem_total <<= 1;
-			if (mem_total < sav_total)
-				goto out;
-		}
-
-		/* If mem_total did not overflow, multiply all memory values by
-		 * val.mem_unit and set it to 1.  This leaves things compatible
-		 * with 2.2.x, and also retains compatibility with earlier 2.4.x
-		 * kernels...  */
+	/*
+	 * If the the available memory or swap is less than can be
+	 * stored in a 32 bit unsigned long then we can be binary
+	 * compatible with 2.2.x kernels.  If not, well, in that case
+	 * 2.2.x was broken anyways...
+	 *
+	 *  -Erik Andersen <[email protected]> 
+	 */
+
+	mem_unit = val.mem_unit;
+	if (val.totalram  * mem_unit > val.totalram &&
+	    val.totalswap * mem_unit > val.totalswap) {
+
+		/*
+		 * If mem_total did not overflow, multiply all memory
+		 * values by val.mem_unit and set it to 1.  This
+		 * leaves things compatible with 2.2.x, and also
+		 * retains compatibility with earlier 2.4.x kernels...
+		 */
 
 		val.mem_unit = 1;
-		val.totalram <<= bitcount;
-		val.freeram <<= bitcount;
-		val.sharedram <<= bitcount;
-		val.bufferram <<= bitcount;
-		val.totalswap <<= bitcount;
-		val.freeswap <<= bitcount;
-		val.totalhigh <<= bitcount;
-		val.freehigh <<= bitcount;
+		val.totalram  *= mem_unit;
+		val.freeram   *= mem_unit;
+		val.sharedram *= mem_unit;
+		val.bufferram *= mem_unit;
+		val.totalswap *= mem_unit;
+		val.freeswap  *= mem_unit;
+		val.totalhigh *= mem_unit;
+		val.freehigh  *= mem_unit;
 	}
-out:
 	if (copy_to_user(info, &val, sizeof(struct sysinfo)))
 		return -EFAULT;
 	return 0;

It's arguable. When I went there a few months back, I was a little surprised by the way it adds ram+swap (it mistakenly added in more before) to make that decision; but concluded it was helping callers who might well go on to add ram+swap, and felt no overriding reason to change that. But you can certainly argue that's inappropriate for the kernel to do, that it should only guard the validity of the actual numbers it returns to the caller. No strong feelings.

Alan, please don't apply. The patch made the logic a lot easier, but sadly wrong: try what happens to totalswap 0x00120000 with mem_unit 0x1000 - wrong decision since 0x20000000 > 0x00120000.

Hugh

There are callers who did add ram + swap

Hi Alan,

And that's a reason to break compatibility?

Greetings
Christoph

We had to break compatibility anyway for 2.4

Hi Alan,

Only for machines with more than 4G swap or ram, not swap + ram. My patch addresses only this case where we did not fix anything (for the kernel space) but broke something. Please keep in mind that 2-4 GB machines where quite common also for 2.2.

Greetings
Christoph

Hi Hugh,

I think it's not the kernels task to fix simple user space errors by breaking compatibility.

And I have somewhat harder feelings since we get a lot of bug reports that our installer only detects 0M RAM when running on a 2.4 machine while it works with the 2.2 kernel. We are talking about an ABI which is directly imported into user space programs.

Uh, oh. Try this one instead:

Greetings
Christoph

@@ -16,6 +16,7 @@
 asmlinkage long sys_sysinfo(struct sysinfo *info)
 {
 	struct sysinfo val;
+	unsigned int mem_unit;
 
 	memset((char *)&val, 0, sizeof(struct sysinfo));
 

@@ -32,47 +33,36 @@
 	si_meminfo(&val);
 	si_swapinfo(&val);
 
-	{
-		unsigned long mem_total, sav_total;
-		unsigned int mem_unit, bitcount;
-
-		/* If the sum of all the available memory (i.e. ram + swap)
-		 * is less than can be stored in a 32 bit unsigned long then
-		 * we can be binary compatible with 2.2.x kernels.  If not,
-		 * well, in that case 2.2.x was broken anyways...
-		 *
-		 *  -Erik Andersen <[email protected]> */
-
-		mem_total = val.totalram + val.totalswap;
-		if (mem_total < val.totalram || mem_total < val.totalswap)
-			goto out;
-		bitcount = 0;
-		mem_unit = val.mem_unit;
-		while (mem_unit > 1) {
-			bitcount++;
-			mem_unit >>= 1;
-			sav_total = mem_total;
-			mem_total <<= 1;
-			if (mem_total < sav_total)
-				goto out;
-		}
-
-		/* If mem_total did not overflow, multiply all memory values by
-		 * val.mem_unit and set it to 1.  This leaves things compatible
-		 * with 2.2.x, and also retains compatibility with earlier 2.4.x
-		 * kernels...  */
+	/*
+	 * If the the available memory or swap is less than can be
+	 * stored in a 32 bit unsigned long then we can be binary
+	 * compatible with 2.2.x kernels.  If not, well, in that case
+	 * 2.2.x was broken anyways...
+	 *
+	 *  -Erik Andersen <[email protected]> 
+	 */
+
+	mem_unit = val.mem_unit;
+	if (val.totalram  <= ULONG_MAX / mem_unit &&
+	    val.totalswap <= ULONG_MAX / mem_unit) {
+
+		/*
+		 * If mem_total did not overflow, multiply all memory
+		 * values by val.mem_unit and set it to 1.  This
+		 * leaves things compatible with 2.2.x, and also
+		 * retains compatibility with earlier 2.4.x kernels...
+		 */
 
 		val.mem_unit = 1;
-		val.totalram <<= bitcount;
-		val.freeram <<= bitcount;
-		val.sharedram <<= bitcount;
-		val.bufferram <<= bitcount;
-		val.totalswap <<= bitcount;
-		val.freeswap <<= bitcount;
-		val.totalhigh <<= bitcount;
-		val.freehigh <<= bitcount;
+		val.totalram  *= mem_unit;
+		val.freeram   *= mem_unit;
+		val.sharedram *= mem_unit;
+		val.bufferram *= mem_unit;
+		val.totalswap *= mem_unit;
+		val.freeswap  *= mem_unit;
+		val.totalhigh *= mem_unit;
+		val.freehigh  *= mem_unit;
 	}
-out:
 	if (copy_to_user(info, &val, sizeof(struct sysinfo)))
 		return -EFAULT;
 	return 0;

This code, either way, is all about trying to keep compatibility with 2.2. You know some instances which are broken by the current code, Alan knows some instances which would be broken by your code.

So fix your installer to work with either, isn't that what really needs to be done? However you play with 2.4's sysinfo, your current 2.2 installer will give wrong results on some RAM sizes, won't it?

I think this one is correct (and much clearer than what's currently there). But whether it should be applied is for Alan to decide...

Hugh

Its your lucky day. Put something like this in your installer, and your problems will go away:

/* Include our own copy of struct sysinfo to avoid binary compatibility
* problems with Linux 2.4, which changed things. Grumble, grumble. */
struct sysinfo {
long uptime; /* Seconds since boot */
unsigned long loads[3]; /* 1, 5, and 15 minute load averages */
unsigned long totalram; /* Total usable main memory size */
unsigned long freeram; /* Available memory size */
unsigned long sharedram; /* Amount of shared memory */
unsigned long bufferram; /* Memory used by buffers */
unsigned long totalswap; /* Total swap space size */
unsigned long freeswap; /* swap space still available */
unsigned short procs; /* Number of current processes */
unsigned short pad; /* Padding needed for m68k */
unsigned long totalhigh; /* Total high memory size */
unsigned long freehigh; /* Available high memory size */
unsigned int mem_unit; /* Memory unit size in bytes */
char _f[20-2*sizeof(long)-sizeof(int)]; /* Padding: libc5 uses this.. */
};
extern int sysinfo (struct sysinfo* info);

/* How much memory does this machine have?
Units are kBytes to avoid overflow on 4GB machines */
static int check_free_memory()

{
    struct sysinfo info;
    unsigned int result, u, s=10;

    if (sysinfo(&info) != 0) {
        fprintf(stderr,"Error checking free memory");
        return -1;
    }

    /* Kernels 2.0.x and 2.2.x return info.mem_unit==0 with values in bytes.
     * Kernels 2.4.0 return info.mem_unit in bytes. */
    u = info.mem_unit;
    if (u==0) u=1;
    while ( (u&1) == 0 && s > 0 ) { u>>=1; s--; }
    result = (info.totalram>>s) + (info.totalswap>>s);
    result = result*u;
    if (result < 0) result = INT_MAX;
    return result;
}

-Erik

--
Erik B. Andersen email: [email protected], [email protected]
--This message was written using 73% post-consumer electrons--

Hi Erik,

We fixed the installer for our upcoming release. But we cannot change the CDs which were assembled and delivered some time ago. And yes, I know how to use LD_PRELOAD...

BTW I appreciate the basics of the change for 2.4, but I don't agree that we should break cases which worked before. (And the comment in the sources is plain wrong that 2.2 failed in these cases)

Greetings
Christoph

But 2.2 _did_ fail. If you take a linux 2.2.x system, add 4 Gigs of swap, and then use sysinfo(), the sizes you get back are junk...

-Erik

--
Erik B. Andersen email: [email protected], [email protected]
--This message was written using 73% post-consumer electrons--

Hi Erik,

But if you add 3.9GB it is ok. Also with 3.9GB RAM. And that's a quite common machine in our environment.

If one of these overflows I agree that the 2.4 scheme is better. But we should keep compatibility as long as we have no single field which overflows.

And that's what my patch implements.

Greetings
Christoph