linux-kernel - Re: [PATCH] staging: bcm: fix error handling in bcm

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: <20120902165130.GA3952@gmail.com>
Date:	Sun, 2 Sep 2012 12:51:31 -0400
From:	Kevin McKinney <klmckinney1@...il.com>
To:	Alexey Khoroshilov <khoroshilov@...ras.ru>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	devel@...verdev.osuosl.org, ldv-project@...ras.ru,
	linux-kernel@...r.kernel.org, klmckinney1@...il.com,
	Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH] staging: bcm: fix error handling in bcm_init()

On Sun, Sep 02, 2012 at 01:37:36AM +0400, Alexey Khoroshilov wrote:
> bcm_init() does not have proper error handling of usb_register().
> The patch implements one.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@...ras.ru>
> ---
>  drivers/staging/bcm/InterfaceInit.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/bcm/InterfaceInit.c b/drivers/staging/bcm/InterfaceInit.c
> index 8f85de6..0049890 100644
> --- a/drivers/staging/bcm/InterfaceInit.c
> +++ b/drivers/staging/bcm/InterfaceInit.c
> @@ -669,6 +669,8 @@ struct class *bcm_class;
>  
>  static __init int bcm_init(void)
>  {
> +	int retval;
> +
>  	printk(KERN_INFO "%s: %s, %s\n", DRV_NAME, DRV_DESCRIPTION, DRV_VERSION);
>  	printk(KERN_INFO "%s\n", DRV_COPYRIGHT);
>  
> @@ -678,7 +680,15 @@ static __init int bcm_init(void)
>  		return PTR_ERR(bcm_class);
>  	}
>  
> -	return usb_register(&usbbcm_driver);
> +	retval = usb_register(&usbbcm_driver);
> +	if (retval < 0) {
> +		printk(KERN_ERR DRV_NAME ": could not register usb driver\n");
> +		goto out_class;
> +	}
> +	return 0;
> +out_class:
> +	class_destroy(bcm_class);
> +	return retval;
>  }
>  
Hi Alexey,

Instead of using a goto statement, can we just: (1) destory the class and (2) return retval within the if statement? i.e:

if (retval < 0) {
	printk(KERN_ERR DRV_NAME ": could not register usb driver\n");
	+class_destroy(bcm_class);
	+return retval;
}

goto statements are very useful and handy when a function exits from multiple locations because they provide a 
common exit point. The compiler will translate this into a "jump" instruction. However in this case I am not sure
it is simplifying our logic because we only have one usage. lets just call the two lines of code directly within the
if statement.

In addition, while you are in this code can you replace the "printk" statements with the preferred "pr_info" in a separate patch?  

Thanks,
Kevin
--
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/