real_root_dev

2000-10-19 Thread Geert Uytterhoeven


`real_root_dev' must be `int', not `kdev_t'.

The reason for this is that we still have (in ):

typedef unsigned short kdev_t;

while kernel/sysctl.c has the following line:

{KERN_REALROOTDEV, "real-root-dev", &real_root_dev, sizeof(int), 0644, NULL, 
&proc_dointvec},

This causes a nice endianness problem on big-endian machines. It can even
cause weird results on little-endian machines where the alignment rule for int
is a 2 byte-boundary.

We've had this patch in the m68k tree since ages (at least 2.0.x), so it's time
to share it to the world :-) I think it was originally by Andreas Schwab.

--- linux-2.4.0-test10-pre3/init/main.c Sat Sep 23 17:31:22 2000
+++ geert-real_root_dev-2.4.0-test10-pre3/init/main.c   Thu Oct 19 21:39:07 2000
@@ -124,7 +124,7 @@
 int rows, cols;
 
 #ifdef CONFIG_BLK_DEV_INITRD
-kdev_t real_root_dev;
+int real_root_dev; /* MUST be int for sysctl! */
 #endif
 
 int root_mountflags = MS_RDONLY;
@@ -717,7 +717,7 @@
 
 #ifdef CONFIG_BLK_DEV_INITRD
root_mountflags = real_root_mountflags;
-   if (mount_initrd && ROOT_DEV != real_root_dev
+   if (mount_initrd && ROOT_DEV != (kdev_t)real_root_dev
&& MAJOR(ROOT_DEV) == RAMDISK_MAJOR && MINOR(ROOT_DEV) == 0) {
int error;
int i, pid;
@@ -725,9 +725,9 @@
pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
if (pid>0)
while (pid != wait(&i));
-   if (MAJOR(real_root_dev) != RAMDISK_MAJOR
-    || MINOR(real_root_dev) != 0) {
-   error = change_root(real_root_dev,"/initrd");
+   if (MAJOR((kdev_t)real_root_dev) != RAMDISK_MAJOR
+    || MINOR((kdev_t)real_root_dev) != 0) {
+   error = change_root((kdev_t)real_root_dev,"/initrd");
if (error)
printk(KERN_ERR "Change root to /initrd: "
    "error %d\n",error);
--- linux-2.4.0-test10-pre3/include/linux/devfs_fs_kernel.h Tue Jul 18 14:07:34 
2000
+++ geert-real_root_dev-2.4.0-test10-pre3/include/linux/devfs_fs_kernel.h   Thu 
+Oct 19 21:38:06 2000
@@ -46,7 +46,7 @@
 
 
 #ifdef CONFIG_BLK_DEV_INITRD
-#  define ROOT_DEVICE_NAME ((real_root_dev ==ROOT_DEV) ? root_device_name:NULL)
+#  define ROOT_DEVICE_NAME (((kdev_t)real_root_dev ==ROOT_DEV) ? 
+root_device_name:NULL)
 #else
 #  define ROOT_DEVICE_NAME root_device_name
 #endif
--- linux-2.4.0-test10-pre3/include/linux/fs.h  Sat Oct 14 19:03:33 2000
+++ geert-real_root_dev-2.4.0-test10-pre3/include/linux/fs.hThu Oct 19 21:38:21 
+2000
@@ -1228,7 +1228,7 @@
 extern void mount_root(void);
 
 #ifdef CONFIG_BLK_DEV_INITRD
-extern kdev_t real_root_dev;
+extern int real_root_dev;
 extern int change_root(kdev_t, const char *);
 #endif
 
--- linux-2.4.0-test10-pre3/drivers/block/rd.c  Tue Jul 18 14:08:53 2000
+++ geert-real_root_dev-2.4.0-test10-pre3/drivers/block/rd.cThu Oct 19 21:37:58 
+2000
@@ -704,7 +704,7 @@
 static void __init rd_load_disk(int n)
 {
 #ifdef CONFIG_BLK_DEV_INITRD
-   extern kdev_t real_root_dev;
+   extern int real_root_dev;
 #endif
 
if (rd_doload == 0)
@@ -712,7 +712,7 @@
 
if (MAJOR(ROOT_DEV) != FLOPPY_MAJOR
 #ifdef CONFIG_BLK_DEV_INITRD
-   && MAJOR(real_root_dev) != FLOPPY_MAJOR
+   && MAJOR((kdev_t)real_root_dev) != FLOPPY_MAJOR
 #endif
)
return;
@@ -724,8 +724,8 @@
 #ifdef CONFIG_MAC_FLOPPY
if(MAJOR(ROOT_DEV) == FLOPPY_MAJOR)
swim3_fd_eject(MINOR(ROOT_DEV));
-   else if(MAJOR(real_root_dev) == FLOPPY_MAJOR)
-   swim3_fd_eject(MINOR(real_root_dev));
+   else if(MAJOR((kdev_t)real_root_dev) == FLOPPY_MAJOR)
+   swim3_fd_eject(MINOR((kdev_t)real_root_dev));
 #endif
printk(KERN_NOTICE
   "VFS: Insert root floppy disk to be loaded into RAM disk and 
press ENTER\n");

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [EMAIL PROTECTED]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/



Re: real_root_dev

2000-11-27 Thread Andries Brouwer

On Mon, Nov 27, 2000 at 10:27:00PM +0100, Marcus Sundberg wrote:

> This reminded me of an old bug which apparently still hasn't been
> fixed (not in 2.2 at least). In init/main.c we have:
> 
> extern int rd_image_start;/* starting block # of image */
> #ifdef CONFIG_BLK_DEV_INITRD
> kdev_t real_root_dev;
> #endif
> #endif
> 
> int root_mountflags = MS_RDONLY;
> 
> and then in kernel/sysctl.c:
> 
> #ifdef CONFIG_BLK_DEV_INITRD
>   {KERN_REALROOTDEV, "real-root-dev", &real_root_dev, sizeof(int),
>0644, NULL, &proc_dointvec},
> #endif
> 
> Because rd_image_start and root_mountflags are both int-aligned,
> this happens to work on little endian platforms. On big endian
> platforms however writing a value in the range 0-65535 to 
> /proc/sys/kernel/real-root-dev will place 0 in real_root_dev,
> and the actual value in the two padding bytes...
> 
> Unfortunately proc_dointvec() doesn't support shorts, so what is
> the correct fix? Changing:
> kdev_t real_root_dev;
> into
> int real_root-dev;
> is a perfectly working solution, but is it acceptable?

If you compile the kernel and use an integral type for kdev_t, perhaps.
On the other hand, I usually use a pointer type for kdev_t, and then
this entire sysctl construction is broken.

Andries
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/



Re: real_root_dev

2000-10-20 Thread Andries Brouwer

On Thu, Oct 19, 2000 at 09:50:48PM +0200, Geert Uytterhoeven wrote:
> 
> `real_root_dev' must be `int', not `kdev_t'.
> 
> - if (MAJOR(real_root_dev) != RAMDISK_MAJOR
> + if (MAJOR((kdev_t)real_root_dev) != RAMDISK_MAJOR

Ach, Geert, how painful to behold!

Never forget: a kdev_t is a pointer to a structure,
and MAJOR takes a field of this structure.
Casting an integer to a structure is ridiculous.
There are functions to_kdev_t etc to do the conversion
(and these may involve lookup in a hash table).

Please keep the source as much as possible kdev_t clean.
At some point in time, I hope 2.5.1, we must change,
and all such cruft would have to be fixed again.

Andries
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/



Re: real_root_dev

2000-10-21 Thread Geert Uytterhoeven

On Fri, 20 Oct 2000, Andries Brouwer wrote:
> On Thu, Oct 19, 2000 at 09:50:48PM +0200, Geert Uytterhoeven wrote:
> > 
> > `real_root_dev' must be `int', not `kdev_t'.
> > 
> > -   if (MAJOR(real_root_dev) != RAMDISK_MAJOR
> > +   if (MAJOR((kdev_t)real_root_dev) != RAMDISK_MAJOR
> 
> Ach, Geert, how painful to behold!
> 
> Never forget: a kdev_t is a pointer to a structure,
> and MAJOR takes a field of this structure.
> Casting an integer to a structure is ridiculous.
> There are functions to_kdev_t etc to do the conversion
> (and these may involve lookup in a hash table).

Well, that's what the _comments_ in  say:

| However, for the time being we let kdev_t be almost the same as dev_t:
|
| typedef struct { unsigned short major, minor; } kdev_t;

But the actual definition is

| typedef unsigned short kdev_t;

Which is incompatible with taking the address of a kdev_t object and assuming
it has the same size as an int, which doesn't equal to any of the `admissible
operations on an object of type kdev_t', as per .

> Please keep the source as much as possible kdev_t clean.
> At some point in time, I hope 2.5.1, we must change,
> and all such cruft would have to be fixed again.

So what do you suggest to fix the bug related to sysctl of real_root_dev?
Just disable it completely?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [EMAIL PROTECTED]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/