__get_user usage in mm/slab.c

__get_user usage in mm/slab.c

[Roman Zippel]

Hi,

The way __get_user is currently used in mm/slab.c is not portable. It
breaks on arch which have seperate user/kernel memory space. It still
works during boot or from kernel threads, but /proc/slabinfo shows only 
broken entries or if a module creates a slab cache, I got lots of
warnings.
We have to at least insert a "set_fs(get_fs())", but IMO a separate
interface would be better. Any opinions?

bye, Roman

Re: __get_user usage in mm/slab.c

[David S. Miller]

   From: Roman Zippel <zippel@linux-m68k.org>
   Date: Tue, 12 Mar 2002 12:58:53 +0100 (CET)

   We have to at least insert a "set_fs(get_fs())", but IMO a separate
   interface would be better. Any opinions?

Right, it is portable if set_fs(KERNEL_DS) is done around it.

This is how most arch syscall ABI translation layers work btw.

Because this way basically must work, I would prefer it gets fixed
like this instead of creating a new interface.

Re: __get_user usage in mm/slab.c

[vinolin]

On Tuesday 12 March 2002 17:28, Roman Zippel wrote:
> Hi,
>
> The way __get_user is currently used in mm/slab.c is not portable. It
> breaks on arch which have seperate user/kernel memory space. It still
> works during boot or from kernel threads, but /proc/slabinfo shows only
> broken entries or if a module creates a slab cache, I got lots of
> warnings.
> We have to at least insert a "set_fs(get_fs())", but IMO a separate
> interface would be better. Any opinions?
>
> bye, Roman

Hi !

I guess u need to read user space from kernel sapce.
You can use the sys_call_table functions for this.
For example, u can use the following code in your file to open a user space 
file from kernel space.

#define BEGIN_KMEM {mm_segment_t old_fs=get_fs();set_fs(get_ds());
#define END_KMEM set_fs(old_fs);}
int (*sample_open)(char *, int);

sample_open = sys_call_table[SYS_open];
BEGIN_KMEM
sample_open("/home/samplefile.txt",O_RDWR);
/* the same way close , read , write etc. can be  written */
END_KMEM

Regards,
Vinolin.

Re: __get_user usage in mm/slab.c

[Andi Kleen]

On Tue, Mar 12, 2002 at 12:58:53PM +0100, Roman Zippel wrote:
> Hi,
> 
> The way __get_user is currently used in mm/slab.c is not portable. It
> breaks on arch which have seperate user/kernel memory space. It still
> works during boot or from kernel threads, but /proc/slabinfo shows only 
> broken entries or if a module creates a slab cache, I got lots of
> warnings.
> We have to at least insert a "set_fs(get_fs())", but IMO a separate
> interface would be better. Any opinions?

I agree that a separate interface would be better, one that guarantees to
handle exceptions on the m68k and other archs with separate address spaces too.
I use that facility quite regularly in architecture specific code, sorry
for letting it slip into portable code. 
I guess set_fs(KERNEL_DS); __*_user() will not catch exceptions on m68k
currently, right? 

-Andi

Re: __get_user usage in mm/slab.c

[David S. Miller]

   From: Andi Kleen <ak@muc.de>
   Date: Tue, 12 Mar 2002 16:23:16 +0100

   I guess set_fs(KERNEL_DS); __*_user() will not catch exceptions on m68k
   currently, right? 

I, for one, think it should.

Re: __get_user usage in mm/slab.c

[Roman Zippel]

Hi,

On Tue, 12 Mar 2002, Andi Kleen wrote:

> I guess set_fs(KERNEL_DS); __*_user() will not catch exceptions on m68k
> currently, right? 

It will, so the patch below is enough. Linus, could you please it?
If we only have a few users of this, I agree with David, that this is
enough. Should it become more common in generic code, it should at least
be documented somewhere.

bye, Roman

Index: mm/slab.c
===================================================================
RCS file: /home/other/cvs/linux/linux-2.5/mm/slab.c,v
retrieving revision 1.1.1.5
diff -u -r1.1.1.5 slab.c
--- mm/slab.c	2002/03/12 13:28:52	1.1.1.5
+++ mm/slab.c	2002/03/12 15:25:00
@@ -839,6 +839,8 @@
 	down(&cache_chain_sem);
 	{
 		struct list_head *p;
+		mm_segment_t fs = get_fs();
+		set_fs(KERNEL_DS);
 
 		list_for_each(p, &cache_chain) {
 			kmem_cache_t *pc = list_entry(p, kmem_cache_t, next);
@@ -857,6 +859,7 @@
 				BUG(); 
 			}	
 		}
+		set_fs(fs);
 	}
 
 	/* There is no reason to lock our new cache before we
@@ -1964,8 +1967,11 @@
 	name = cachep->name; 
 	{
 	char tmp; 
+	mm_segment_t fs = get_fs();
+	set_fs(KERNEL_DS);
 	if (__get_user(tmp, name)) 
 		name = "broken"; 
+	set_fs(fs);
 	} 	
 
 	seq_printf(m, "%-17s %6lu %6lu %6u %4lu %4lu %4u",