linux-kernel - Re: [PATCH] usb: hub: Minor refactoring in usb_hub

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: <20190923071145.GB2746429@kroah.com>
Date:   Mon, 23 Sep 2019 09:11:45 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Austin Kim <austindh.kim@...il.com>
Cc:     stern@...land.harvard.edu, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org, mathias.nyman@...ux.intel.com,
        Thinh.Nguyen@...opsys.com, nsaenzjulienne@...e.de,
        jflat@...omium.org, malat@...ian.org, dianders@...omium.org
Subject: Re: [PATCH] usb: hub: Minor refactoring in usb_hub_init()

On Mon, Sep 23, 2019 at 03:19:21PM +0900, Austin Kim wrote:
> Normally when creation of workqueue fails, exception handling takes place
> after the call to alloc_workqueue() is made.
> 
> But looking into usb_hub_init() function, 'return 0' statement is executed,
> when alloc_workqueue() returns valid workqueue pointer.
>        if (hub_wq)
>                return 0;
> 
> This might make other Linux driver developers get confused 
> because they could deduce that this is exceptional handling routine.
> 
> So perform minor refactoring by adding NULL pointer dereference check
> routine right after the call to alloc_workqueue() is made.
> 
> Signed-off-by: Austin Kim <austindh.kim@...il.com>
> ---
>  drivers/usb/core/hub.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e8ebacc..0ddbfe6 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5530,9 +5530,12 @@ int usb_hub_init(void)
>  	 * over to the companion full-speed controller.
>  	 */
>  	hub_wq = alloc_workqueue("usb_hub_wq", WQ_FREEZABLE, 0);
> -	if (hub_wq)
> -		return 0;
> +	if (unlikely(!hub_wq))

Only ever use likely/unlikely if you can measure the difference without
it.  Otherwise the compiler and cpu will almost always do this better
than you.

So please remove this.

> +		goto err_workqueue;
> +
> +	return 0;
>  
> +err_workqueue:
>  	/* Fall through if kernel_thread failed */

This comment is now incorrect.

But really, there is nothing wrong with the original code here.  It
works properly, and while it is not identical to normal code "style"
here, there's nothing wrong with it that I can see.

thanks,

greg k-h