linux-kernel - Re: [PATCH v2 5/9] usb: don't use PREPARE_DELAYED

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, 22 Feb 2014 10:14:48 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Tejun Heo <tj@...nel.org>
cc:	laijs@...fujitsu.com, <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linux-usb@...r.kernel.org>
Subject: Re: [PATCH v2 5/9] usb: don't use PREPARE_DELAYED_WORK

On Sat, 22 Feb 2014, Tejun Heo wrote:

> Hello,
> 
> If this is actually safe, let's do it from the get-go.
> 
> Thanks!
> ------- 8< -------
> PREPARE_[DELAYED_]WORK() are being phased out.  They have few users
> and a nasty surprise in terms of reentrancy guarantee as workqueue
> considers work items to be different if they don't have the same work
> function.
> 
> usb_hub->init_work is multiplexed with multiple work functions;
> however, the work item is never queued while in-flight, so we can
> simply use INIT_DELAYED_WORK() before each queueing.
> 
> It would probably be best to route this with other related updates
> through the workqueue tree.
> 
> Lightly tested.
> 
> v2: Greg and Alan confirm that the work item is never queued while
>     in-flight.  Simply use INIT_DELAYED_WORK().
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Alan Stern <stern@...land.harvard.edu>
> Cc: linux-usb@...r.kernel.org
> ---
>  drivers/usb/core/hub.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1040,7 +1040,7 @@ static void hub_activate(struct usb_hub
>  		 */
>  		if (type == HUB_INIT) {
>  			delay = hub_power_on(hub, false);
> -			PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2);
> +			INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
>  			schedule_delayed_work(&hub->init_work,
>  					msecs_to_jiffies(delay));
>  
> @@ -1194,7 +1194,7 @@ static void hub_activate(struct usb_hub
>  
>  		/* Don't do a long sleep inside a workqueue routine */
>  		if (type == HUB_INIT2) {
> -			PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func3);
> +			INIT_DELAYED_WORK(&hub->init_work, hub_init_func3);
>  			schedule_delayed_work(&hub->init_work,
>  					msecs_to_jiffies(delay));
>  			return;		/* Continues at init3: below */
> 

This should work okay.  But while you're making these changes, you 
should remove the INIT_DELAYED_WORK(&hub->init_work, NULL) call in 
hub_probe().  It is now unnecessary.

Is the cancel_delayed_work_sync(&hub->init_work) call in hub_quiesce()
going to get confused by all this?

It's worth mentioning that the only reason for the hub_init_func3 stuff
is, as the comment says, to avoid a long sleep (100 ms) inside a work
routine.  With all the changes to the work queue infrastructure, maybe
this doesn't matter so much any more.  If we got rid of it then there
wouldn't be any multiplexing, and this whole issue would become moot.

Alan Stern

--
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/