alpha fp-emu vs module refcounting

alpha fp-emu vs module refcounting

[Christoph Hellwig]

arch/alpha/math-emu/math.c still uses MOD_{INC,DEC}_USE_COUNT to protect
from unloading which is

 a) unsafe
 b) in the way of nuking them as soon as 2.7 is out

any chance we could either do a try_module_get on a struct module
*fp_emul_module in alpha core code before or completely disallow module
unloading for it (by removing the cleanup_module() callback)?

Re: alpha fp-emu vs module refcounting

[Ivan Kokshaysky]

On Fri, May 07, 2004 at 01:02:17PM +0200, Christoph Hellwig wrote:
> any chance we could either do a try_module_get on a struct module
> *fp_emul_module in alpha core code before or completely disallow module
> unloading for it (by removing the cleanup_module() callback)?

How about this (also fixes modular build of math-emu)?

Ivan.

--- 2.6/arch/alpha/math-emu/math.c	Sun Apr  4 07:38:23 2004
+++ linux/arch/alpha/math-emu/math.c	Fri May  7 18:21:15 2004
@@ -51,7 +51,9 @@ extern void alpha_write_fp_reg_s (unsign
 
 #ifdef MODULE
 
+MODULE_AUTHOR("Richard Henderson <rth@twiddle.net>");
 MODULE_DESCRIPTION("FP Software completion module");
+MODULE_LICENSE("GPL");
 
 extern long (*alpha_fp_emul_imprecise)(struct pt_regs *, unsigned long);
 extern long (*alpha_fp_emul) (unsigned long pc);
@@ -106,7 +108,7 @@ alpha_fp_emul (unsigned long pc)
 	__u32 insn;
 	long si_code;
 
-	MOD_INC_USE_COUNT;
+	BUG_ON(!try_module_get(THIS_MODULE));
 
 	get_user(insn, (__u32*)pc);
 	fc     = (insn >>  0) & 0x1f;	/* destination register */
@@ -320,7 +322,7 @@ done:
 			if (_fex & IEEE_TRAP_ENABLE_INV) si_code = FPE_FLTINV;
 		}
 
-		MOD_DEC_USE_COUNT;
+		module_put(THIS_MODULE);
 		return si_code;
 	}
 
@@ -328,13 +330,13 @@ done:
 	   requires that the result *always* be written... so we do the write
 	   immediately after the operations above.  */
 
-	MOD_DEC_USE_COUNT;
+	module_put(THIS_MODULE);
 	return 0;
 
 bad_insn:
 	printk(KERN_ERR "alpha_fp_emul: Invalid FP insn %#x at %#lx\n",
 	       insn, pc);
-	MOD_DEC_USE_COUNT;
+	module_put(THIS_MODULE);
 	return -1;
 }
 
@@ -344,7 +346,7 @@ alpha_fp_emul_imprecise (struct pt_regs 
 	unsigned long trigger_pc = regs->pc - 4;
 	unsigned long insn, opcode, rc, si_code = 0;
 
-	MOD_INC_USE_COUNT;
+	BUG_ON(!try_module_get(THIS_MODULE));
 
 	/*
 	 * Turn off the bits corresponding to registers that are the
@@ -403,6 +405,6 @@ alpha_fp_emul_imprecise (struct pt_regs 
 	}
 
 egress:
-	MOD_DEC_USE_COUNT;
+	module_put(THIS_MODULE);
 	return si_code;
 }
--- 2.6/arch/alpha/math-emu/Makefile	Sun Apr  4 07:37:23 2004
+++ linux/arch/alpha/math-emu/Makefile	Fri May  7 17:52:22 2004
@@ -4,4 +4,6 @@
 
 EXTRA_CFLAGS := -w
 
-obj-$(CONFIG_MATHEMU) += math.o qrnnd.o
+obj-$(CONFIG_MATHEMU) += math-emu.o
+
+math-emu-objs := math.o qrnnd.o

Re: alpha fp-emu vs module refcounting

[Christoph Hellwig]

On Fri, May 07, 2004 at 06:32:08PM +0400, Ivan Kokshaysky wrote:
> -	MOD_INC_USE_COUNT;
> +	BUG_ON(!try_module_get(THIS_MODULE));

This is broken.  If you know you already have a reference somewhere -
and I can't find how that would work - you'd have to use __module_get,
but if it's not you can get a failure from try_module_get legally and
you should do the try_module_get outside the module, before it's code
is exectuted.

Re: alpha fp-emu vs module refcounting

[Ivan Kokshaysky]

On Fri, May 07, 2004 at 04:35:12PM +0200, Christoph Hellwig wrote:
> On Fri, May 07, 2004 at 06:32:08PM +0400, Ivan Kokshaysky wrote:
> > -	MOD_INC_USE_COUNT;
> > +	BUG_ON(!try_module_get(THIS_MODULE));
> 
> This is broken.  If you know you already have a reference somewhere -
> and I can't find how that would work - you'd have to use __module_get,
> but if it's not you can get a failure from try_module_get legally and
> you should do the try_module_get outside the module, before it's code
> is exectuted.

Ok, I realize - this seems to be confusing. I'll try to clarify:
- First of all, mere mortals are _not_ allowed to compile mandatory
  alpha IEEE fp emulation code as a module. Which is documented
  in arch/alpha/Kconfig.
- Roughly speaking, the fp emu _module_ code intercepts the fp traps,
  so races vs module loading/unloading are fundamentally unavoidable.
  These refcounting attempts just narrow the window.

And no, try_module_get should never fail here.

Alternatively, we could just drop _all_ module related stuff from
alpha/math-emu...

Ivan.

Re: alpha fp-emu vs module refcounting

[Christoph Hellwig]

On Sat, May 08, 2004 at 02:37:17AM +0400, Ivan Kokshaysky wrote:
> Ok, I realize - this seems to be confusing. I'll try to clarify:
> - First of all, mere mortals are _not_ allowed to compile mandatory
>   alpha IEEE fp emulation code as a module. Which is documented
>   in arch/alpha/Kconfig.
> - Roughly speaking, the fp emu _module_ code intercepts the fp traps,
>   so races vs module loading/unloading are fundamentally unavoidable.
>   These refcounting attempts just narrow the window.
> 
> And no, try_module_get should never fail here.
> 
> Alternatively, we could just drop _all_ module related stuff from
> alpha/math-emu...

either that or just marking it unloadable by removing the cleanup_module
handler sound like the simplest solution I guess.

Re: alpha fp-emu vs module refcounting

[Ivan Kokshaysky]

On Sat, May 08, 2004 at 12:41:04AM +0200, Christoph Hellwig wrote:
> either that or just marking it unloadable by removing the cleanup_module
> handler sound like the simplest solution I guess.

Or leave it as it is for now - 'CONFIG_MATHEMU=m' won't compile. ;-)

Ivan.

Re: alpha fp-emu vs module refcounting

[Christoph Hellwig]

On Sat, May 08, 2004 at 02:51:42AM +0400, Ivan Kokshaysky wrote:
> On Sat, May 08, 2004 at 12:41:04AM +0200, Christoph Hellwig wrote:
> > either that or just marking it unloadable by removing the cleanup_module
> > handler sound like the simplest solution I guess.
> 
> Or leave it as it is for now - 'CONFIG_MATHEMU=m' won't compile. ;-)

Well, still false positives in grep.  What about this patch to simply
remove any traces of CONFIG_MATHEMU and modular math emulation?


--- 1.36/arch/alpha/Kconfig	Sat Mar 20 19:29:54 2004
+++ edited/arch/alpha/Kconfig	Sun May 16 11:14:44 2004
@@ -625,14 +625,6 @@
 	  Say Y here if you are developing drivers or trying to debug and
 	  identify kernel problems.
 
-config MATHEMU
-	tristate "Kernel FP software completion" if DEBUG_KERNEL
-	default y if !DEBUG_KERNEL
-	help
-	  This option is required for IEEE compliant floating point arithmetic
-	  on the Alpha. The only time you would ever not say Y is to say M in
-	  order to debug the code. Say Y unless you know what you are doing.
-
 config DEBUG_SLAB
 	bool "Debug memory allocations"
 	depends on DEBUG_KERNEL
===== arch/alpha/kernel/alpha_ksyms.c 1.37 vs edited =====
--- 1.37/arch/alpha/kernel/alpha_ksyms.c	Thu Mar 18 01:58:10 2004
+++ edited/arch/alpha/kernel/alpha_ksyms.c	Sun May 16 11:15:01 2004
@@ -169,13 +169,6 @@
 EXPORT_SYMBOL(csum_partial_copy_from_user);
 EXPORT_SYMBOL(csum_ipv6_magic);
 
-#ifdef CONFIG_MATHEMU_MODULE
-extern long (*alpha_fp_emul_imprecise)(struct pt_regs *, unsigned long);
-extern long (*alpha_fp_emul) (unsigned long pc);
-EXPORT_SYMBOL(alpha_fp_emul_imprecise);
-EXPORT_SYMBOL(alpha_fp_emul);
-#endif
-
 #ifdef CONFIG_ALPHA_BROKEN_IRQ_MASK
 EXPORT_SYMBOL(__min_ipl);
 #endif
===== arch/alpha/kernel/traps.c 1.29 vs edited =====
--- 1.29/arch/alpha/kernel/traps.c	Thu Apr 22 10:40:31 2004
+++ edited/arch/alpha/kernel/traps.c	Sun May 16 11:15:09 2004
@@ -191,16 +191,8 @@
 	do_exit(SIGSEGV);
 }
 
-#ifndef CONFIG_MATHEMU
-static long dummy_emul(void) { return 0; }
-long (*alpha_fp_emul_imprecise)(struct pt_regs *regs, unsigned long writemask)
-  = (void *)dummy_emul;
-long (*alpha_fp_emul) (unsigned long pc)
-  = (void *)dummy_emul;
-#else
 long alpha_fp_emul_imprecise(struct pt_regs *regs, unsigned long writemask);
 long alpha_fp_emul (unsigned long pc);
-#endif
 
 asmlinkage void
 do_entArith(unsigned long summary, unsigned long write_mask,
===== arch/alpha/math-emu/Makefile 1.9 vs edited =====
--- 1.9/arch/alpha/math-emu/Makefile	Sun Feb 23 02:34:29 2003
+++ edited/arch/alpha/math-emu/Makefile	Sun May 16 11:15:21 2004
@@ -4,4 +4,4 @@
 
 EXTRA_CFLAGS := -w
 
-obj-$(CONFIG_MATHEMU) += math.o qrnnd.o
+obj-y += math.o qrnnd.o
===== arch/alpha/math-emu/math.c 1.5 vs edited =====
--- 1.5/arch/alpha/math-emu/math.c	Fri Apr  4 00:49:57 2003
+++ edited/arch/alpha/math-emu/math.c	Sun May 16 11:15:57 2004
@@ -48,43 +48,6 @@
 extern unsigned long alpha_read_fp_reg_s (unsigned long reg);
 extern void alpha_write_fp_reg_s (unsigned long reg, unsigned long val);
 
-
-#ifdef MODULE
-
-MODULE_DESCRIPTION("FP Software completion module");
-
-extern long (*alpha_fp_emul_imprecise)(struct pt_regs *, unsigned long);
-extern long (*alpha_fp_emul) (unsigned long pc);
-
-static long (*save_emul_imprecise)(struct pt_regs *, unsigned long);
-static long (*save_emul) (unsigned long pc);
-
-long do_alpha_fp_emul_imprecise(struct pt_regs *, unsigned long);
-long do_alpha_fp_emul(unsigned long);
-
-int init_module(void)
-{
-	save_emul_imprecise = alpha_fp_emul_imprecise;
-	save_emul = alpha_fp_emul;
-	alpha_fp_emul_imprecise = do_alpha_fp_emul_imprecise;
-	alpha_fp_emul = do_alpha_fp_emul;
-	return 0;
-}
-
-void cleanup_module(void)
-{
-	alpha_fp_emul_imprecise = save_emul_imprecise;
-	alpha_fp_emul = save_emul;
-}
-
-#undef  alpha_fp_emul_imprecise
-#define alpha_fp_emul_imprecise		do_alpha_fp_emul_imprecise
-#undef  alpha_fp_emul
-#define alpha_fp_emul			do_alpha_fp_emul
-
-#endif /* MODULE */
-
-
 /*
  * Emulate the floating point instruction at address PC.  Returns -1 if the
  * instruction to be emulated is illegal (such as with the opDEC trap), else
@@ -106,8 +69,6 @@
 	__u32 insn;
 	long si_code;
 
-	MOD_INC_USE_COUNT;
-
 	get_user(insn, (__u32*)pc);
 	fc     = (insn >>  0) & 0x1f;	/* destination register */
 	fb     = (insn >> 16) & 0x1f;
@@ -320,7 +281,6 @@
 			if (_fex & IEEE_TRAP_ENABLE_INV) si_code = FPE_FLTINV;
 		}
 
-		MOD_DEC_USE_COUNT;
 		return si_code;
 	}
 
@@ -328,13 +288,11 @@
 	   requires that the result *always* be written... so we do the write
 	   immediately after the operations above.  */
 
-	MOD_DEC_USE_COUNT;
 	return 0;
 
 bad_insn:
 	printk(KERN_ERR "alpha_fp_emul: Invalid FP insn %#x at %#lx\n",
 	       insn, pc);
-	MOD_DEC_USE_COUNT;
 	return -1;
 }
 
@@ -344,8 +302,6 @@
 	unsigned long trigger_pc = regs->pc - 4;
 	unsigned long insn, opcode, rc, si_code = 0;
 
-	MOD_INC_USE_COUNT;
-
 	/*
 	 * Turn off the bits corresponding to registers that are the
 	 * target of instructions that set bits in the exception
@@ -403,6 +359,5 @@
 	}
 
 egress:
-	MOD_DEC_USE_COUNT;
 	return si_code;
 }

Re: alpha fp-emu vs module refcounting

[Ivan Kokshaysky]

On Sun, May 16, 2004 at 12:04:35PM +0200, Christoph Hellwig wrote:
> Well, still false positives in grep.  What about this patch to simply
> remove any traces of CONFIG_MATHEMU and modular math emulation?

Personally, I'm fine with it.

Yet another simple solution would be just to remove refcounting and
only allow modular build of math-emu when CONFIG_SMP=n, which is
safe vs module unload and still fine for debugging.

Richard?

Ivan.

Re: alpha fp-emu vs module refcounting

[Christoph Hellwig]

On Sun, May 16, 2004 at 04:44:20PM +0400, Ivan Kokshaysky wrote:
> On Sun, May 16, 2004 at 12:04:35PM +0200, Christoph Hellwig wrote:
> > Well, still false positives in grep.  What about this patch to simply
> > remove any traces of CONFIG_MATHEMU and modular math emulation?
> 
> Personally, I'm fine with it.
> 
> Yet another simple solution would be just to remove refcounting and
> only allow modular build of math-emu when CONFIG_SMP=n, which is
> safe vs module unload and still fine for debugging.

UP is not safe vs module unloading per se.  Especially not with
CONFIG_PREEMPT.  I'd say just apply the patch and if someone badly needs
modular mathemu for debugging he/she can apply some local hack.

Re: alpha fp-emu vs module refcounting

[Richard Henderson]

On Sun, May 16, 2004 at 04:44:20PM +0400, Ivan Kokshaysky wrote:
> Yet another simple solution would be just to remove refcounting and
> only allow modular build of math-emu when CONFIG_SMP=n, which is
> safe vs module unload and still fine for debugging.
> 
> Richard?

Seems reasonable.  We've already got it under the debugging
config section, right?


r~