linux-kernel - Re: lockdep false positive? -- firewire-core transaction timer vs. scsi-core host lock

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: <tkrat.2aa42a952e4df4b1@s5r6.in-berlin.de>
Date:	Wed, 18 Aug 2010 14:50:25 +0200 (CEST)
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Clemens Ladisch <clemens@...isch.de>
cc:	Peter Zijlstra <peterz@...radead.org>,
	Yong Zhang <yong.zhang0@...il.com>,
	Johannes Berg <johannes@...solutions.net>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux1394-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: lockdep false positive? -- firewire-core transaction timer vs.
 scsi-core host lock

Clemens Ladisch wrote:
> firewire: core: do not use del_timer_sync() in interrupt context
> 
> Because we might be in interrupt context, replace del_timer_sync() with
> del_timer().

OK.  And thanks for the pointers into the locking guide.

> If the timer is already running, we know that it will
> clean up the transaction, so we do not need to do any further processing
> in the normal transaction handler.
> 
> Many thanks to Yong Zhang for diagnosing this.
> 
> Reported-by: Stefan Richter <stefanr@...6.in-berlin.de>
> Signed-off-by: Clemens Ladisch <clemens@...isch.de>
> ---
>  drivers/firewire/core-transaction.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> --- a/drivers/firewire/core-transaction.c
> +++ b/drivers/firewire/core-transaction.c
> @@ -81,6 +81,8 @@ static int close_transaction(struct fw_transaction *transaction,
>  	spin_lock_irqsave(&card->lock, flags);
>  	list_for_each_entry(t, &card->transaction_list, link) {
>  		if (t == transaction) {
> +			if (!del_timer(&t->split_timeout_timer))
> +				goto timed_out;

+			if (!del_timer(&t->split_timeout_timer)) {
+				spin_unlock_irqrestore(&card->lock, flags);
+				goto timed_out;
+			}

>  			list_del_init(&t->link);
>  			card->tlabel_mask &= ~(1ULL << t->tlabel);
>  			break;
> @@ -89,11 +91,11 @@ static int close_transaction(struct fw_transaction *transaction,
>  	spin_unlock_irqrestore(&card->lock, flags);
>  
>  	if (&t->link != &card->transaction_list) {
> -		del_timer_sync(&t->split_timeout_timer);
>  		t->callback(card, rcode, NULL, 0, t->callback_data);
>  		return 0;
>  	}
>  
> +timed_out:
>  	return -ENOENT;
>  }
>  
> @@ -921,6 +923,8 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  	spin_lock_irqsave(&card->lock, flags);
>  	list_for_each_entry(t, &card->transaction_list, link) {
>  		if (t->node_id == source && t->tlabel == tlabel) {
> +			if (!del_timer(&t->split_timeout_timer))
> +				goto timed_out;

Ditto.

>  			list_del_init(&t->link);
>  			card->tlabel_mask &= ~(1ULL << t->tlabel);
>  			break;
> @@ -929,6 +933,7 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  	spin_unlock_irqrestore(&card->lock, flags);
>  
>  	if (&t->link == &card->transaction_list) {
> +timed_out:
>  		fw_notify("Unsolicited response (source %x, tlabel %x)\n",
>  			  source, tlabel);
>  		return;
> @@ -963,8 +968,6 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  		break;
>  	}
>  
> -	del_timer_sync(&t->split_timeout_timer);
> -
>  	/*
>  	 * The response handler may be executed while the request handler
>  	 * is still pending.  Cancel the request handler.

Shall I commit with the added spin_unlocks?  Or do you prefer to send an update?
-- 
Stefan Richter
-=====-==-=- =--- =--=-
http://arcgraph.de/sr/

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