linux-kernel - Re: [PATCH v2] kernel/module.c: cleanup patch for looping, let return 'bool' value instead of real error number.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51919E64.8020409@asianux.com>
Date:	Tue, 14 May 2013 10:16:04 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
CC:	Linus Torvalds <torvalds@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] kernel/module.c: cleanup patch for looping, let return
 'bool' value instead of real error number.


Oh, sorry, my fix is incorrect !!

The return value is used by caller of caller ... it seems the user mode
can sense it !! So we still should return the correct error code to
outside when error occurs.

I think we can reference 'using compiler': it prints all errors and
warnings as far as it can, but the user usually mainly focus on the
first error or warning.

So I think the suitable fix is to still print all problems, but should
return the first error code (not return the last error code).

Please help check, thanks.


On 05/13/2013 08:24 PM, Chen Gang wrote:
> 
> For looping in simplify_symbols(), when multiple errors occur, the
> original error number will be override (the original related commit
> "1da177e Linux-2.6.12-rc").
> 
> We focus on printing all errors as much as possible, and not focus on
> the return error number (only know whether succeed or not is enough),
> 
> So when error occurs during looping, it is correct to only mark "need
> return failure" and continue looping.
> 
> Since simplify_symbols() is a static function (not an API to outside),
> it is better to change the function return type to 'bool', so that can
> skip processing error number details incorrectly.
> 
> 
> Signed-off-by: root <root@...p122.asianux.net>
> ---
>  kernel/module.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index b049939..445a960 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1956,14 +1956,18 @@ static int verify_export_symbols(struct module *mod)
>  	return 0;
>  }
>  
> -/* Change all symbols so that st_value encodes the pointer directly. */
> -static int simplify_symbols(struct module *mod, const struct load_info *info)
> +/*
> + * Change all symbols so that st_value encodes the pointer directly.
> + *
> + * Return true if succeed, or false if at least 1 failure occurs.
> + */
> +static bool simplify_symbols(struct module *mod, const struct load_info *info)
>  {
>  	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
>  	Elf_Sym *sym = (void *)symsec->sh_addr;
>  	unsigned long secbase;
>  	unsigned int i;
> -	int ret = 0;
> +	bool ret = true;
>  	const struct kernel_symbol *ksym;
>  
>  	for (i = 1; i < symsec->sh_size / sizeof(Elf_Sym); i++) {
> @@ -1976,7 +1980,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  			pr_debug("Common symbol: %s\n", name);
>  			printk("%s: please compile with -fno-common\n",
>  			       mod->name);
> -			ret = -ENOEXEC;
> +			/* Mark return result and continue looping */
> +			ret = false;
>  			break;
>  
>  		case SHN_ABS:
> @@ -1999,7 +2004,8 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
>  
>  			printk(KERN_WARNING "%s: Unknown symbol %s (err %li)\n",
>  			       mod->name, name, PTR_ERR(ksym));
> -			ret = PTR_ERR(ksym) ?: -ENOENT;
> +			/* Mark return result and continue looping */
> +			ret = false;
>  			break;
>  
>  		default:
> @@ -3267,8 +3273,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	setup_modinfo(mod, info);
>  
>  	/* Fix up syms, so that st_value is a pointer to location. */
> -	err = simplify_symbols(mod, info);
> -	if (err < 0)
> +	if (!simplify_symbols(mod, info))
>  		goto free_modinfo;
>  
>  	err = apply_relocations(mod, info);
> 


-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/