Re: [PATCH v2 1/2] Bluetooth: hci_core: Add reset_type parameter to hdev->reset() callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] UAPI Breakage: Redefining '1' to mean 'deeper reset' breaks existing userspace scripts.

Pre-existing issues:
- [High] Data Race: Unguarded write to `dev_data->reset_type` in `btintel_pcie_hw_error()` clobbers concurrent sysfs resets.
- [High] TOCTOU Race Condition: Concurrent driver unbind causes NULL pointer dereference in `reset_store()`.
--

commit 4b14894786f1236a0de480b4c65121356669aba4
Author: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>

Bluetooth: hci_core: Add reset_type parameter to hdev->reset() callback

Add a u8 reset_type parameter to the hdev->reset() callback to allow
userspace to select the reset level via sysfs. Each driver interprets
the level according to its own capabilities.

> diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> index 36be024711747..fb445e20f9726 100644
> --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> @@ -3,7 +3,14 @@ Date:		14-Jan-2025
>  KernelVersion:	6.13
>  Contact:	linux-bluetooth@vger.kernel.org
>  Description: 	This write-only attribute allows users to trigger the vendor reset
> -		method on the Bluetooth device when arbitrary data is written.
> +		method on the Bluetooth device. The value written selects the
> +		reset level. Each driver interprets the level according to its
> +		own capabilities:
> +
> +		  - 0: default reset
> +		  - 1: deeper reset
> +
> +		Writing any value other than 1 defaults to level 0.

[Severity: High]
Will redefining 1 to mean a deeper reset break existing userspace scripts? 

Historically, the reset sysfs attribute triggered the default reset when
arbitrary data was written, and in the Linux ecosystem, userspace typically
writes 1 to trigger boolean actions.

If existing scripts use echo 1 > reset, wouldn't they unknowingly trigger a
much more disruptive reset than before? Could we map 1 to the default reset
and use a distinct value (e.g., 2) for the deeper reset to maintain backward
compatibility?

[ ... ]

> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index 9e39327dc1fe2..d3a03cf964216 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
[ ... ]
> @@ -2729,7 +2735,7 @@ static void btintel_pcie_hw_error(struct hci_dev *hdev, u8 code)
>  		return;
>  	}
>  	btintel_pcie_inc_recovery_count(pdev, &hdev->dev);
> -	btintel_pcie_reset(hdev);
> +	btintel_pcie_reset(hdev, (code == 0x13) ? 1 : 0);
>  }

[Severity: High]
This is a pre-existing issue, but does the unguarded write to
dev_data->reset_type earlier in btintel_pcie_hw_error() cause a data race?

While this patch updates btintel_pcie_reset() to safely assign the reset
type under the BTINTEL_PCIE_RECOVERY_IN_PROGRESS bitlock,
btintel_pcie_hw_error() still directly modifies dev_data->reset_type
without holding any structural locks or exclusion bits.

If a sysfs reset (FLR) is concurrently triggered and acquires the bitlock,
could this unguarded write in btintel_pcie_hw_error() silently overwrite the
pending reset type to PLDR, causing the asynchronously scheduled reset work
to read the corrupted type and perform the wrong level of reset?

[ ... ]

> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 8957ce7c21b76..a4fe329158cf7 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -97,8 +97,14 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>  {
>  	struct hci_dev *hdev = to_hci_dev(dev);
>  
> -	if (hdev->reset)
> -		hdev->reset(hdev);
> +	if (hdev->reset) {
> +		int val;
> +
> +		if (kstrtoint(buf, 10, &val) || val != 1)
> +			hdev->reset(hdev, 0);
> +		else
> +			hdev->reset(hdev, 1);
> +	}
>  
>  	return count;
>  }

[Severity: High]
This is a pre-existing issue, but does checking hdev->reset and then calling
it after kstrtoint() introduce a TOCTOU race condition?

Because kstrtoint() is an external function call, it acts as a compiler
barrier, forcing the compiler to reload hdev->reset from memory before
invoking it as a function pointer.

If a concurrent teardown path sets hdev->reset = NULL between the check and
the execution, could reset_store() dereference a NULL pointer and crash the
kernel? While a smaller race window existed before, this patch significantly
widens it by inserting the kstrtoint() call. Should the function pointer be
cached in a local variable before the NULL check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618085016.9173-1-chandrashekar.devegowda@intel.com?part=1