linux-kernel - Re: [PATCH] edd: fix error paths in module

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]

Date:	Sat, 24 May 2008 00:22:05 -0500
From:	Matt Domsch <Matt_Domsch@...l.com>
To:	Akinobu Mita <akinobu.mita@...il.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] edd: fix error paths in module_init

On Sat, May 24, 2008 at 11:03:23AM +0900, Akinobu Mita wrote:
> This patch fixes error handlings when kzalloc() or edd_device_register()
> failed in module_init. It needs to clean registered edd_devices before
> return error.
> 
> Also this patch fixes return value of module_init. module_init should not
> return positive value.

Thanks for these.  You caught me on holiday; I'll take a more thorough
look when I'm back next week.

 
> Signed-off-by: Akinobu Mita <akinobu.mita@...il.com>
> Cc: Matt Domsch <Matt_Domsch@...l.com>
> ---
>  drivers/firmware/edd.c |   30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> Index: 2.6-git/drivers/firmware/edd.c
> ===================================================================
> --- 2.6-git.orig/drivers/firmware/edd.c
> +++ 2.6-git/drivers/firmware/edd.c
> @@ -718,8 +718,7 @@ edd_device_register(struct edd_device *e
>  {
>  	int error;
>  
> -	if (!edev)
> -		return 1;
> +	BUG_ON(!edev);

Wouldn't WARN_ON() and return failure be sufficient?  I hate crashing
the system when loading a driver if I can avoid it.


>  	edd_dev_set_info(edev, i);
>  	edev->kobj.kset = edd_kset;
>  	error = kobject_init_and_add(&edev->kobj, &edd_ktype, NULL,
> @@ -744,8 +743,8 @@ static inline int edd_num_devices(void)
>  static int __init
>  edd_init(void)
>  {
> -	unsigned int i;
> -	int rc=0;
> +	int i;
> +	int rc;
>  	struct edd_device *edev;
>  
>  	printk(KERN_INFO "BIOS EDD facility v%s %s, %d devices found\n",
> @@ -753,29 +752,36 @@ edd_init(void)
>  
>  	if (!edd_num_devices()) {
>  		printk(KERN_INFO "EDD information not available.\n");
> -		return 1;
> +		return -ENODEV;
>  	}
>  
>  	edd_kset = kset_create_and_add("edd", NULL, firmware_kobj);
>  	if (!edd_kset)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < edd_num_devices() && !rc; i++) {
> +	for (i = 0; i < edd_num_devices(); i++) {
>  		edev = kzalloc(sizeof (*edev), GFP_KERNEL);
> -		if (!edev)
> -			return -ENOMEM;
> +		if (!edev) {
> +			rc = -ENOMEM;
> +			goto out;
> +		}
>  
>  		rc = edd_device_register(edev, i);
>  		if (rc) {
>  			kfree(edev);
> -			break;
> +			goto out;
>  		}
>  		edd_devices[i] = edev;
>  	}
>  
> -	if (rc)
> -		kset_unregister(edd_kset);
> -	return rc;
> +	return 0;
> +out:
> +	while (--i >= 0)
> +		edd_device_unregister(edd_devices[i]);
> +
> +	kset_unregister(edd_kset);
> +
> +  	return rc;

I didn't really like my initial approach, but the question was: when
you hit a failure, do you try to back completely out (unregister
everything that had successfully registered until now), or do you
leave the things that have succeeded, and only fail the current and
future devices?  For my purposes, having even the first device be
reported, even if the others couldn't be, is useful.  Hence why I
didn't undo all the registrations on failure.


-- 
Matt Domsch
Linux Technology Strategist, Dell Office of the CTO
linux.dell.com & www.dell.com/linux
--
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/