linux-kernel - Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

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:	Tue, 16 Sep 2014 17:17:28 +0200
From:	Petr Mládek <pmladek@...e.cz>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Tejun Heo <tj@...nel.org>,
	Joe Lawrence <joe.lawrence@...atus.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

On Tue 16-09-14 11:10:31, Petr Mládek wrote:
> Anyway, the solution for the race between kick_hub_wq() and
> hub_event() might be to get the reference already in kick_hub_wq().
> I mean something like:
> 
> static void kick_hub_wq(struct usb_hub *hub)
> {
> 	if (hub->disconnected || work_pending(&hub->events))
> 		return;
> 	/*
> 	 * Suppress autosuspend until the event is proceed.
> 	 *
> 	 * Be careful and make sure that the symmetric operation is
> 	 * always called. We are here only when there is no pending
> 	 * work for this hub. Therefore put the interface either when
> 	 * the new work is called or when it is canceled.
> 	 */
> 	usb_autopm_get_interface_no_resume(to_usb_interface(hub->intfdev));
> 	kref_get(&hub->kref);
> 
> 	if (queue_work(hub_wq, &hub->events))
> 		return;
> 
> 	/* the work could not be scheduled twice */
> 	kref_put(&hub->kref, hub_release);
> 	usb_autopm_put_interface_no_suspend(intf);
> }

I have just realized that I was working in the Linus' three where the
commit c605f3cdff53a743f6d87 ("usb: hub: take hub->hdev reference when
processing from eventlist") was missing :-(

It means that we would need to call also usb_get_dev(hdev) in
kick_hub_wq() to make sure that everything is valid when the work
hub_event() is proceed.

Hmm, it stopped being nice. So, I was looking for another solution and
found an interesting behavior of cancel_work_sync() that is used in
hub_disconnect(). It either removes the work item from the queue or it
waits until the work is done. Anyway, no work is pending when the
function returns. It means that struct usb_hub never will be released
when hub_event() is in progress and we do not need to get the
references in advance at all.

I am really sorry for sending so many mails. I am still getting familiar with
kernel hacking. I am often surprised how complex it is. I do not want
to spam and lose your interest, definitely. So, I am going to calm
down and be more patient when doing the homework.

Best Regards,
Petr
--
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/