linux-kernel - Re: [PATCH] firewire: core: send bus reset promptly on gap count error

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-next>] [day] [month] [year] [list]

Message-ID: <20240127083730.GA159729@workstation.local>
Date: Sat, 27 Jan 2024 17:37:30 +0900
From: Takashi Sakamoto <o-takashi@...amocchi.jp>
To: Adam Goldman <adamg@...ox.com>
Cc: linux1394-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firewire: core: send bus reset promptly on gap count
 error

Hi,

Thanks for the patch. I would like to check some points about the
change.

On Tue, Jan 23, 2024 at 12:11:40AM -0800, Adam Goldman wrote:
> If we are bus manager and the bus has inconsistent gap counts, send a 
> bus reset immediately instead of trying to read the root node's config 
> ROM first. Otherwise, we could spend a lot of time trying to read the 
> config ROM but never succeeding.
> 
> This eliminates a 50+ second delay before the FireWire bus is usable 
> after a newly connected device is powered on in certain circumstances.

At first, would I request you to explain about the certain
circumstances in the patch comment? It is really helpful to understand
the change itself.

> Signed-off-by: Adam Goldman <adamg@...ox.com>
> Link: https://sourceforge.net/p/linux1394/mailman/message/58727806/
> ---
> 
> --- linux-source-6.1.orig/drivers/firewire/core-card.c	2023-09-23 02:11:13.000000000 -0700
> +++ linux-source-6.1/drivers/firewire/core-card.c	2024-01-22 04:23:06.000000000 -0800
> @@ -435,6 +435,16 @@
>  		 * config rom.  In either case, pick another root.
>  		 */
>  		new_root_id = local_id;
> +	} else if (card->gap_count == 0) {
> +		/* 
> +		 * If self IDs have inconsistent gap counts, do a
> +		 * bus reset ASAP. The config rom read might never
> +		 * complete, so don't wait for it. However, still
> +		 * send a PHY configuration packet prior to the bus
> +		 * reset, as permitted by IEEE 1394-2008 8.4.5.2.
> +		 */
> +		new_root_id = local_id;
> +		card->bm_retries = 0;
>  	} else if (!root_device_is_running) {
>  		/*
> 		 * If we haven't probed this device yet, bail out now

Next, after the condition branches, we can see below lines:

```
	/*
	 * Finally, figure out if we should do a reset or not.  If we have
	 * done less than 5 resets with the same physical topology and we
	 * have either a new root or a new gap count setting, let's do it.
	 */

	if (card->bm_retries++ < 5 &&
	    (card->gap_count != gap_count || new_root_id != root_id))
		do_reset = true;
```

When the value of "card->gap_count" is zero, it would hit the condition of
"card->gap_count != gap_count". I think the transmission of phy config
packet and scheduling of short bus reset would be done, regardless of the
change. Would I ask the main intention to the additional branch?


Thanks

Takashi Sakamoto