Re: [PATCH 3/7] Ksplice: Make find_symbol return a struct kernel_symbol

[Rusty Russell <rusty@rustcorp.com.au>]

On Saturday 06 December 2008 10:33:56 Jeff Arnold wrote:
> From: Tim Abbott <tabbott@mit.edu>
> 
> Ksplice needs access to the kernel_symbol structure in order to support
> modifications to the exported symbol table.

Applied, with some fixes:

>  	preempt_disable();
> -	if (IS_ERR_VALUE(find_symbol(symbol, &owner, NULL, true, false)))
> +	if (find_symbol(symbol, &owner, NULL, true, false) == NULL)
>  		BUG();

I skipped the == NULL part; if (find_symbol... reads quite well.  Same with
three or so other places.

> -	ret = find_symbol(name, &owner, &crc,
> +	sym = find_symbol(name, &owner, &crc,
>  			  !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
> -	if (!IS_ERR_VALUE(ret)) {
> -		/* use_module can fail due to OOM,
> -		   or module initialization or unloading */
> -		if (!check_version(sechdrs, versindex, name, mod, crc) ||
> -		    !use_module(mod, owner))
> -			ret = -EINVAL;
> -	}
> -	return ret;
> +	/* use_module can fail due to OOM,
> +	   or module initialization or unloading */
> +	if (!check_version(sechdrs, versindex, name, mod, crc) ||
> +	    !use_module(mod, owner))
> +		sym = NULL;
> +	return sym;
>  }

If sym is NULL those other values aren't valid!  You can't take out the
branch....

> -	return (void *)value;
> +	return sym == NULL ? (void *)0 : (void *)sym->value;
>  }

One NULL too many, and one too few:
	return sym ? (void *)sym->value : NULL;

Here's the nett diff:

diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -860,7 +860,7 @@ void __symbol_put(const char *symbol)
 	struct module *owner;
 
 	preempt_disable();
-	if (find_symbol(symbol, &owner, NULL, true, false) == NULL)
+	if (!find_symbol(symbol, &owner, NULL, true, false))
 		BUG();
 	module_put(owner);
 	preempt_enable();
@@ -1023,7 +1023,7 @@ static inline int check_modstruct_versio
 {
 	const unsigned long *crc;
 
-	if (find_symbol("struct_module", NULL, &crc, true, false) == NULL)
+	if (!find_symbol("struct_module", NULL, &crc, true, false))
 		BUG();
 	return check_version(sechdrs, versindex, "struct_module", mod, crc);
 }
@@ -1077,9 +1077,11 @@ static const struct kernel_symbol *resol
 			  !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
 	/* use_module can fail due to OOM,
 	   or module initialization or unloading */
-	if (!check_version(sechdrs, versindex, name, mod, crc) ||
-	    !use_module(mod, owner))
-		sym = NULL;
+	if (sym) {
+		if (!check_version(sechdrs, versindex, name, mod, crc) ||
+		    !use_module(mod, owner))
+			sym = NULL;
+	}
 	return sym;
 }
 
@@ -1481,11 +1483,11 @@ void *__symbol_get(const char *symbol)
 
 	preempt_disable();
 	sym = find_symbol(symbol, &owner, NULL, true, true);
-	if (sym != NULL && strong_try_module_get(owner))
+	if (sym && strong_try_module_get(owner))
 		sym = NULL;
 	preempt_enable();
 
-	return sym == NULL ? (void *)0 : (void *)sym->value;
+	return sym ? (void *)sym->value : NULL;
 }
 EXPORT_SYMBOL_GPL(__symbol_get);
 
@@ -1513,8 +1515,7 @@ static int verify_export_symbols(struct 
 
 	for (i = 0; i < ARRAY_SIZE(arr); i++) {
 		for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) {
-			if (find_symbol(s->name, &owner, NULL, true, false)
-			    != NULL) {
+			if (find_symbol(s->name, &owner, NULL, true, false)) {
 				printk(KERN_ERR
 				       "%s: exports duplicate symbol %s"
 				       " (owned by %s)\n",
@@ -1561,7 +1562,7 @@ static int simplify_symbols(Elf_Shdr *se
 			ksym = resolve_symbol(sechdrs, versindex,
 					      strtab + sym[i].st_name, mod);
 			/* Ok if resolved.  */
-			if (ksym != NULL) {
+			if (ksym) {
 				sym[i].st_value = ksym->value;
 				break;
 			}