[PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Hsin-chen Chuang]

From: Hsin-chen Chuang <chharry@chromium.org>

Use device group to avoid the racing. To reuse the group defined in
hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
read_usb_alt_setting which are only registered in btusb.

Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
---

(no changes since v1)

 drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 75a0f15819c4..bf210275e5b7 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
 	return 0;
 }
 
+static int btusb_read_alt_setting(struct hci_dev *hdev)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+
+	return data->isoc_altsetting;
+}
+
 static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
 							int alt)
 {
@@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
 	.llseek		= default_llseek,
 };
 
-static ssize_t isoc_alt_show(struct device *dev,
-			     struct device_attribute *attr,
-			     char *buf)
-{
-	struct btusb_data *data = dev_get_drvdata(dev);
-
-	return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
-}
-
-static ssize_t isoc_alt_store(struct device *dev,
-			      struct device_attribute *attr,
-			      const char *buf, size_t count)
-{
-	struct btusb_data *data = dev_get_drvdata(dev);
-	int alt;
-	int ret;
-
-	if (kstrtoint(buf, 10, &alt))
-		return -EINVAL;
-
-	ret = btusb_switch_alt_setting(data->hdev, alt);
-	return ret < 0 ? ret : count;
-}
-
-static DEVICE_ATTR_RW(isoc_alt);
-
 static int btusb_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
@@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
 		if (err < 0)
 			goto out_free_dev;
 
-		err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
-		if (err)
-			goto out_free_dev;
+		hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
+		hdev->read_usb_alt_setting = btusb_read_alt_setting;
 	}
 
 	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
@@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
 	hdev = data->hdev;
 	usb_set_intfdata(data->intf, NULL);
 
-	if (data->isoc) {
-		device_remove_file(&intf->dev, &dev_attr_isoc_alt);
+	if (data->isoc)
 		usb_set_intfdata(data->isoc, NULL);
-	}
 
 	if (data->diag)
 		usb_set_intfdata(data->diag, NULL);
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f756fac95488..5d3ec5ff5adb 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -641,6 +641,8 @@ struct hci_dev {
 				     struct bt_codec *codec, __u8 *vnd_len,
 				     __u8 **vnd_data);
 	u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
+	int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
+	int (*read_usb_alt_setting)(struct hci_dev *hdev);
 };
 
 #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 041ce9adc378..887aa1935b1e 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(reset);
 
+static ssize_t isoc_alt_show(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct hci_dev *hdev = to_hci_dev(dev);
+
+	if (hdev->read_usb_alt_setting)
+		return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
+
+	return -ENODEV;
+}
+
+static ssize_t isoc_alt_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct hci_dev *hdev = to_hci_dev(dev);
+	int alt;
+	int ret;
+
+	if (kstrtoint(buf, 10, &alt))
+		return -EINVAL;
+
+	if (hdev->switch_usb_alt_setting) {
+		ret = hdev->switch_usb_alt_setting(hdev, alt);
+		return ret < 0 ? ret : count;
+	}
+
+	return -ENODEV;
+}
+static DEVICE_ATTR_RW(isoc_alt);
+
 static struct attribute *bt_host_attrs[] = {
 	&dev_attr_reset.attr,
+	&dev_attr_isoc_alt.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(bt_host);
-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v2 2/3] Bluetooth: Always allow SCO packets for user channel

[Hsin-chen Chuang]

From: Hsin-chen Chuang <chharry@chromium.org>

The SCO packets from Bluetooth raw socket are now rejected because
hci_conn_num is left 0. This patch allows such the usecase to enable
the userspace SCO support.

Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
---

Changes in v2:
- Check HCI_USER_CHANNEL rather than just remove the hci_conn_num check

 drivers/bluetooth/btusb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index bf210275e5b7..acfa83228b25 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2104,7 +2104,8 @@ static int btusb_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 		return submit_or_queue_tx_urb(hdev, urb);
 
 	case HCI_SCODATA_PKT:
-		if (hci_conn_num(hdev, SCO_LINK) < 1)
+		if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
+		    hci_conn_num(hdev, SCO_LINK) < 1)
 			return -ENODEV;
 
 		urb = alloc_isoc_urb(hdev, skb);
@@ -2585,7 +2586,8 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
 		return submit_or_queue_tx_urb(hdev, urb);
 
 	case HCI_SCODATA_PKT:
-		if (hci_conn_num(hdev, SCO_LINK) < 1)
+		if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
+		    hci_conn_num(hdev, SCO_LINK) < 1)
 			return -ENODEV;
 
 		urb = alloc_isoc_urb(hdev, skb);
-- 
2.48.1.262.g85cc9f2d1e-goog

[PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt

[Hsin-chen Chuang]

From: Hsin-chen Chuang <chharry@chromium.org>

The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix
possible race with userspace of sysfs isoc_alt")

Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt")
Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
---

(no changes since v1)

 Documentation/ABI/stable/sysfs-class-bluetooth | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
index 36be02471174..8cc5f3cfe133 100644
--- a/Documentation/ABI/stable/sysfs-class-bluetooth
+++ b/Documentation/ABI/stable/sysfs-class-bluetooth
@@ -7,3 +7,17 @@ Description: 	This write-only attribute allows users to trigger the vendor reset
 		The reset may or may not be done through the device transport
 		(e.g., UART/USB), and can also be done through an out-of-band
 		approach such as GPIO.
+
+What: 		/sys/class/bluetooth/hci<index>/isoc_alt
+Date:		22-Jan-2025
+KernelVersion:	6.13
+Contact:	linux-bluetooth@vger.kernel.org
+Description: 	This attribute allows users to configure the USB Alternate setting
+		for the specific HCI device. Reading this attribute returns the
+		current setting, and writing any supported numbers would change
+		the setting. See the USB Alternate setting definition in Bluetooth
+		core spec 5, vol 4, part B, table 2.1.
+		If the HCI device doesn't support USB Alternate setting
+		configuration, the read/write fails with -ENODEV.
+		If the data is not a valid number, the write fails with -EINVAL.
+		The other failures are vendor specific.
-- 
2.48.1.262.g85cc9f2d1e-goog

Re: [PATCH v2 3/3] Bluetooth: Add ABI doc for sysfs isoc_alt

[Hsin-chen Chuang]

Hi Luiz,

On Wed, Jan 22, 2025 at 1:21 PM Hsin-chen Chuang <chharry@google.com> wrote:
>
> From: Hsin-chen Chuang <chharry@chromium.org>
>
> The functionality was completed in commit 5e5c3898ef49 ("Bluetooth: Fix
> possible race with userspace of sysfs isoc_alt")

5e5c3898ef49 is the first patch in this series, I assume the hash would
change after it is applied? Shall I instead squash this patch to that or
shall I wait for 5e5c3898ef49 to be applied first?

>
> Fixes: 5e5c3898ef49 ("Bluetooth: Fix possible race with userspace of sysfs isoc_alt")
> Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> ---
>
> (no changes since v1)
>
>  Documentation/ABI/stable/sysfs-class-bluetooth | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> index 36be02471174..8cc5f3cfe133 100644
> --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> @@ -7,3 +7,17 @@ Description:   This write-only attribute allows users to trigger the vendor reset
>                 The reset may or may not be done through the device transport
>                 (e.g., UART/USB), and can also be done through an out-of-band
>                 approach such as GPIO.
> +
> +What:          /sys/class/bluetooth/hci<index>/isoc_alt
> +Date:          22-Jan-2025
> +KernelVersion: 6.13
> +Contact:       linux-bluetooth@vger.kernel.org
> +Description:   This attribute allows users to configure the USB Alternate setting
> +               for the specific HCI device. Reading this attribute returns the
> +               current setting, and writing any supported numbers would change
> +               the setting. See the USB Alternate setting definition in Bluetooth
> +               core spec 5, vol 4, part B, table 2.1.
> +               If the HCI device doesn't support USB Alternate setting
> +               configuration, the read/write fails with -ENODEV.
> +               If the data is not a valid number, the write fails with -EINVAL.
> +               The other failures are vendor specific.
> --
> 2.48.1.262.g85cc9f2d1e-goog
>

-- 
Best Regards,
Hsin-chen

RE: [v2,1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=927394

---Test result---

Test Summary:
CheckPatch                    PENDING   0.48 seconds
GitLint                       PENDING   0.29 seconds
SubjectPrefix                 PASS      0.19 seconds
BuildKernel                   PASS      24.01 seconds
CheckAllWarning               PASS      26.31 seconds
CheckSparse                   PASS      29.89 seconds
BuildKernel32                 PASS      23.61 seconds
TestRunnerSetup               PASS      424.15 seconds
TestRunner_l2cap-tester       PASS      20.28 seconds
TestRunner_iso-tester         PASS      29.46 seconds
TestRunner_bnep-tester        PASS      4.76 seconds
TestRunner_mgmt-tester        FAIL      122.54 seconds
TestRunner_rfcomm-tester      PASS      7.51 seconds
TestRunner_sco-tester         PASS      9.28 seconds
TestRunner_ioctl-tester       PASS      8.02 seconds
TestRunner_mesh-tester        PASS      5.87 seconds
TestRunner_smp-tester         PASS      6.86 seconds
TestRunner_userchan-tester    PASS      7.03 seconds
IncrementalBuild              PENDING   0.46 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
LL Privacy - Set Flags 2 (Enable RL)                 Failed       0.138 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Luiz Augusto von Dentz]

Hi Hsin-chen,

On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote:
>
> From: Hsin-chen Chuang <chharry@chromium.org>
>
> Use device group to avoid the racing. To reuse the group defined in
> hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
> read_usb_alt_setting which are only registered in btusb.
>
> Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> ---
>
> (no changes since v1)
>
>  drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
>  include/net/bluetooth/hci_core.h |  2 ++
>  net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
>  3 files changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 75a0f15819c4..bf210275e5b7 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
>         return 0;
>  }
>
> +static int btusb_read_alt_setting(struct hci_dev *hdev)
> +{
> +       struct btusb_data *data = hci_get_drvdata(hdev);
> +
> +       return data->isoc_altsetting;
> +}
> +
>  static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
>                                                         int alt)
>  {
> @@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
>         .llseek         = default_llseek,
>  };
>
> -static ssize_t isoc_alt_show(struct device *dev,
> -                            struct device_attribute *attr,
> -                            char *buf)
> -{
> -       struct btusb_data *data = dev_get_drvdata(dev);
> -
> -       return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
> -}
> -
> -static ssize_t isoc_alt_store(struct device *dev,
> -                             struct device_attribute *attr,
> -                             const char *buf, size_t count)
> -{
> -       struct btusb_data *data = dev_get_drvdata(dev);
> -       int alt;
> -       int ret;
> -
> -       if (kstrtoint(buf, 10, &alt))
> -               return -EINVAL;
> -
> -       ret = btusb_switch_alt_setting(data->hdev, alt);
> -       return ret < 0 ? ret : count;
> -}
> -
> -static DEVICE_ATTR_RW(isoc_alt);
> -
>  static int btusb_probe(struct usb_interface *intf,
>                        const struct usb_device_id *id)
>  {
> @@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
>                 if (err < 0)
>                         goto out_free_dev;
>
> -               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> -               if (err)
> -                       goto out_free_dev;
> +               hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
> +               hdev->read_usb_alt_setting = btusb_read_alt_setting;
>         }
>
>         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> @@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
>         hdev = data->hdev;
>         usb_set_intfdata(data->intf, NULL);
>
> -       if (data->isoc) {
> -               device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> +       if (data->isoc)
>                 usb_set_intfdata(data->isoc, NULL);
> -       }
>
>         if (data->diag)
>                 usb_set_intfdata(data->diag, NULL);
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f756fac95488..5d3ec5ff5adb 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -641,6 +641,8 @@ struct hci_dev {
>                                      struct bt_codec *codec, __u8 *vnd_len,
>                                      __u8 **vnd_data);
>         u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
> +       int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
> +       int (*read_usb_alt_setting)(struct hci_dev *hdev);
>  };
>
>  #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 041ce9adc378..887aa1935b1e 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(reset);
>
> +static ssize_t isoc_alt_show(struct device *dev,
> +                            struct device_attribute *attr,
> +                            char *buf)
> +{
> +       struct hci_dev *hdev = to_hci_dev(dev);
> +
> +       if (hdev->read_usb_alt_setting)
> +               return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
> +
> +       return -ENODEV;
> +}
> +
> +static ssize_t isoc_alt_store(struct device *dev,
> +                             struct device_attribute *attr,
> +                             const char *buf, size_t count)
> +{
> +       struct hci_dev *hdev = to_hci_dev(dev);
> +       int alt;
> +       int ret;
> +
> +       if (kstrtoint(buf, 10, &alt))
> +               return -EINVAL;
> +
> +       if (hdev->switch_usb_alt_setting) {
> +               ret = hdev->switch_usb_alt_setting(hdev, alt);
> +               return ret < 0 ? ret : count;
> +       }
> +
> +       return -ENODEV;
> +}
> +static DEVICE_ATTR_RW(isoc_alt);
> +
>  static struct attribute *bt_host_attrs[] = {
>         &dev_attr_reset.attr,
> +       &dev_attr_isoc_alt.attr,
>         NULL,
>  };
>  ATTRIBUTE_GROUPS(bt_host);

While this fixes the race it also forces the inclusion of an attribute
that is driver specific, so I wonder if we should introduce some
internal interface to register driver specific entries like this.

> --
> 2.48.1.262.g85cc9f2d1e-goog
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Hsin-chen Chuang]

Hi Luiz,

On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote:
> >
> > From: Hsin-chen Chuang <chharry@chromium.org>
> >
> > Use device group to avoid the racing. To reuse the group defined in
> > hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
> > read_usb_alt_setting which are only registered in btusb.
> >
> > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
> >  include/net/bluetooth/hci_core.h |  2 ++
> >  net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
> >  3 files changed, 45 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 75a0f15819c4..bf210275e5b7 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> >         return 0;
> >  }
> >
> > +static int btusb_read_alt_setting(struct hci_dev *hdev)
> > +{
> > +       struct btusb_data *data = hci_get_drvdata(hdev);
> > +
> > +       return data->isoc_altsetting;
> > +}
> > +
> >  static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> >                                                         int alt)
> >  {
> > @@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
> >         .llseek         = default_llseek,
> >  };
> >
> > -static ssize_t isoc_alt_show(struct device *dev,
> > -                            struct device_attribute *attr,
> > -                            char *buf)
> > -{
> > -       struct btusb_data *data = dev_get_drvdata(dev);
> > -
> > -       return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
> > -}
> > -
> > -static ssize_t isoc_alt_store(struct device *dev,
> > -                             struct device_attribute *attr,
> > -                             const char *buf, size_t count)
> > -{
> > -       struct btusb_data *data = dev_get_drvdata(dev);
> > -       int alt;
> > -       int ret;
> > -
> > -       if (kstrtoint(buf, 10, &alt))
> > -               return -EINVAL;
> > -
> > -       ret = btusb_switch_alt_setting(data->hdev, alt);
> > -       return ret < 0 ? ret : count;
> > -}
> > -
> > -static DEVICE_ATTR_RW(isoc_alt);
> > -
> >  static int btusb_probe(struct usb_interface *intf,
> >                        const struct usb_device_id *id)
> >  {
> > @@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
> >                 if (err < 0)
> >                         goto out_free_dev;
> >
> > -               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> > -               if (err)
> > -                       goto out_free_dev;
> > +               hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
> > +               hdev->read_usb_alt_setting = btusb_read_alt_setting;
> >         }
> >
> >         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> > @@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> >         hdev = data->hdev;
> >         usb_set_intfdata(data->intf, NULL);
> >
> > -       if (data->isoc) {
> > -               device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > +       if (data->isoc)
> >                 usb_set_intfdata(data->isoc, NULL);
> > -       }
> >
> >         if (data->diag)
> >                 usb_set_intfdata(data->diag, NULL);
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index f756fac95488..5d3ec5ff5adb 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -641,6 +641,8 @@ struct hci_dev {
> >                                      struct bt_codec *codec, __u8 *vnd_len,
> >                                      __u8 **vnd_data);
> >         u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
> > +       int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
> > +       int (*read_usb_alt_setting)(struct hci_dev *hdev);
> >  };
> >
> >  #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > index 041ce9adc378..887aa1935b1e 100644
> > --- a/net/bluetooth/hci_sysfs.c
> > +++ b/net/bluetooth/hci_sysfs.c
> > @@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_WO(reset);
> >
> > +static ssize_t isoc_alt_show(struct device *dev,
> > +                            struct device_attribute *attr,
> > +                            char *buf)
> > +{
> > +       struct hci_dev *hdev = to_hci_dev(dev);
> > +
> > +       if (hdev->read_usb_alt_setting)
> > +               return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
> > +
> > +       return -ENODEV;
> > +}
> > +
> > +static ssize_t isoc_alt_store(struct device *dev,
> > +                             struct device_attribute *attr,
> > +                             const char *buf, size_t count)
> > +{
> > +       struct hci_dev *hdev = to_hci_dev(dev);
> > +       int alt;
> > +       int ret;
> > +
> > +       if (kstrtoint(buf, 10, &alt))
> > +               return -EINVAL;
> > +
> > +       if (hdev->switch_usb_alt_setting) {
> > +               ret = hdev->switch_usb_alt_setting(hdev, alt);
> > +               return ret < 0 ? ret : count;
> > +       }
> > +
> > +       return -ENODEV;
> > +}
> > +static DEVICE_ATTR_RW(isoc_alt);
> > +
> >  static struct attribute *bt_host_attrs[] = {
> >         &dev_attr_reset.attr,
> > +       &dev_attr_isoc_alt.attr,
> >         NULL,
> >  };
> >  ATTRIBUTE_GROUPS(bt_host);
>
> While this fixes the race it also forces the inclusion of an attribute
> that is driver specific, so I wonder if we should introduce some
> internal interface to register driver specific entries like this.

Do you mean you prefer the original interface that only exports the
attribute when isoc_altsetting is supported?
Agree it makes more sense but I hit the obstacle: hci_init_sysfs is
called earlier than data->isoc is determined. I need some time to
verify whether changing the order won't break anything.

>
> > --
> > 2.48.1.262.g85cc9f2d1e-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Luiz Augusto von Dentz]

Hi Hsin-chen,

On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote:
>
> Hi Luiz,
>
> On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote:
> > >
> > > From: Hsin-chen Chuang <chharry@chromium.org>
> > >
> > > Use device group to avoid the racing. To reuse the group defined in
> > > hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
> > > read_usb_alt_setting which are only registered in btusb.
> > >
> > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
> > >  include/net/bluetooth/hci_core.h |  2 ++
> > >  net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
> > >  3 files changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 75a0f15819c4..bf210275e5b7 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> > >         return 0;
> > >  }
> > >
> > > +static int btusb_read_alt_setting(struct hci_dev *hdev)
> > > +{
> > > +       struct btusb_data *data = hci_get_drvdata(hdev);
> > > +
> > > +       return data->isoc_altsetting;
> > > +}
> > > +
> > >  static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> > >                                                         int alt)
> > >  {
> > > @@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
> > >         .llseek         = default_llseek,
> > >  };
> > >
> > > -static ssize_t isoc_alt_show(struct device *dev,
> > > -                            struct device_attribute *attr,
> > > -                            char *buf)
> > > -{
> > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > -
> > > -       return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
> > > -}
> > > -
> > > -static ssize_t isoc_alt_store(struct device *dev,
> > > -                             struct device_attribute *attr,
> > > -                             const char *buf, size_t count)
> > > -{
> > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > -       int alt;
> > > -       int ret;
> > > -
> > > -       if (kstrtoint(buf, 10, &alt))
> > > -               return -EINVAL;
> > > -
> > > -       ret = btusb_switch_alt_setting(data->hdev, alt);
> > > -       return ret < 0 ? ret : count;
> > > -}
> > > -
> > > -static DEVICE_ATTR_RW(isoc_alt);
> > > -
> > >  static int btusb_probe(struct usb_interface *intf,
> > >                        const struct usb_device_id *id)
> > >  {
> > > @@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
> > >                 if (err < 0)
> > >                         goto out_free_dev;
> > >
> > > -               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> > > -               if (err)
> > > -                       goto out_free_dev;
> > > +               hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
> > > +               hdev->read_usb_alt_setting = btusb_read_alt_setting;
> > >         }
> > >
> > >         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> > > @@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> > >         hdev = data->hdev;
> > >         usb_set_intfdata(data->intf, NULL);
> > >
> > > -       if (data->isoc) {
> > > -               device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > > +       if (data->isoc)
> > >                 usb_set_intfdata(data->isoc, NULL);
> > > -       }
> > >
> > >         if (data->diag)
> > >                 usb_set_intfdata(data->diag, NULL);
> > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > index f756fac95488..5d3ec5ff5adb 100644
> > > --- a/include/net/bluetooth/hci_core.h
> > > +++ b/include/net/bluetooth/hci_core.h
> > > @@ -641,6 +641,8 @@ struct hci_dev {
> > >                                      struct bt_codec *codec, __u8 *vnd_len,
> > >                                      __u8 **vnd_data);
> > >         u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
> > > +       int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
> > > +       int (*read_usb_alt_setting)(struct hci_dev *hdev);
> > >  };
> > >
> > >  #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > > index 041ce9adc378..887aa1935b1e 100644
> > > --- a/net/bluetooth/hci_sysfs.c
> > > +++ b/net/bluetooth/hci_sysfs.c
> > > @@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > >  }
> > >  static DEVICE_ATTR_WO(reset);
> > >
> > > +static ssize_t isoc_alt_show(struct device *dev,
> > > +                            struct device_attribute *attr,
> > > +                            char *buf)
> > > +{
> > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > +
> > > +       if (hdev->read_usb_alt_setting)
> > > +               return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
> > > +
> > > +       return -ENODEV;
> > > +}
> > > +
> > > +static ssize_t isoc_alt_store(struct device *dev,
> > > +                             struct device_attribute *attr,
> > > +                             const char *buf, size_t count)
> > > +{
> > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > +       int alt;
> > > +       int ret;
> > > +
> > > +       if (kstrtoint(buf, 10, &alt))
> > > +               return -EINVAL;
> > > +
> > > +       if (hdev->switch_usb_alt_setting) {
> > > +               ret = hdev->switch_usb_alt_setting(hdev, alt);
> > > +               return ret < 0 ? ret : count;
> > > +       }
> > > +
> > > +       return -ENODEV;
> > > +}
> > > +static DEVICE_ATTR_RW(isoc_alt);
> > > +
> > >  static struct attribute *bt_host_attrs[] = {
> > >         &dev_attr_reset.attr,
> > > +       &dev_attr_isoc_alt.attr,
> > >         NULL,
> > >  };
> > >  ATTRIBUTE_GROUPS(bt_host);
> >
> > While this fixes the race it also forces the inclusion of an attribute
> > that is driver specific, so I wonder if we should introduce some
> > internal interface to register driver specific entries like this.
>
> Do you mean you prefer the original interface that only exports the
> attribute when isoc_altsetting is supported?
> Agree it makes more sense but I hit the obstacle: hci_init_sysfs is
> called earlier than data->isoc is determined. I need some time to
> verify whether changing the order won't break anything.

We might have to do something like the following within hci_init_sysfs:

if (hdev->isoc_alt)
    dev->type = bt_host_isoc_alt
else
    dev->type = bt_host

Now perhaps instead of adding the callbacks to hdev we add the
attribute itself, btw did you check if there isn't a sysfs entry to
interact with USB alternate settings? Because contrary to reset this
actually operates directly on the driver bus so it sort of made more
sense to me that this would be handled by USB rather than Bluetooth.

> >
> > > --
> > > 2.48.1.262.g85cc9f2d1e-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Luiz Augusto von Dentz]

Hi Hsin-chen,

On Fri, Jan 24, 2025 at 10:54 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote:
> >
> > Hi Luiz,
> >
> > On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Hsin-chen,
> > >
> > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote:
> > > >
> > > > From: Hsin-chen Chuang <chharry@chromium.org>
> > > >
> > > > Use device group to avoid the racing. To reuse the group defined in
> > > > hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
> > > > read_usb_alt_setting which are only registered in btusb.
> > > >
> > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
> > > >  include/net/bluetooth/hci_core.h |  2 ++
> > > >  net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
> > > >  3 files changed, 45 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > index 75a0f15819c4..bf210275e5b7 100644
> > > > --- a/drivers/bluetooth/btusb.c
> > > > +++ b/drivers/bluetooth/btusb.c
> > > > @@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> > > >         return 0;
> > > >  }
> > > >
> > > > +static int btusb_read_alt_setting(struct hci_dev *hdev)
> > > > +{
> > > > +       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > +
> > > > +       return data->isoc_altsetting;
> > > > +}
> > > > +
> > > >  static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> > > >                                                         int alt)
> > > >  {
> > > > @@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
> > > >         .llseek         = default_llseek,
> > > >  };
> > > >
> > > > -static ssize_t isoc_alt_show(struct device *dev,
> > > > -                            struct device_attribute *attr,
> > > > -                            char *buf)
> > > > -{
> > > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > > -
> > > > -       return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
> > > > -}
> > > > -
> > > > -static ssize_t isoc_alt_store(struct device *dev,
> > > > -                             struct device_attribute *attr,
> > > > -                             const char *buf, size_t count)
> > > > -{
> > > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > > -       int alt;
> > > > -       int ret;
> > > > -
> > > > -       if (kstrtoint(buf, 10, &alt))
> > > > -               return -EINVAL;
> > > > -
> > > > -       ret = btusb_switch_alt_setting(data->hdev, alt);
> > > > -       return ret < 0 ? ret : count;
> > > > -}
> > > > -
> > > > -static DEVICE_ATTR_RW(isoc_alt);
> > > > -
> > > >  static int btusb_probe(struct usb_interface *intf,
> > > >                        const struct usb_device_id *id)
> > > >  {
> > > > @@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
> > > >                 if (err < 0)
> > > >                         goto out_free_dev;
> > > >
> > > > -               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> > > > -               if (err)
> > > > -                       goto out_free_dev;
> > > > +               hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
> > > > +               hdev->read_usb_alt_setting = btusb_read_alt_setting;
> > > >         }
> > > >
> > > >         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> > > > @@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> > > >         hdev = data->hdev;
> > > >         usb_set_intfdata(data->intf, NULL);
> > > >
> > > > -       if (data->isoc) {
> > > > -               device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > > > +       if (data->isoc)
> > > >                 usb_set_intfdata(data->isoc, NULL);
> > > > -       }
> > > >
> > > >         if (data->diag)
> > > >                 usb_set_intfdata(data->diag, NULL);
> > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > > index f756fac95488..5d3ec5ff5adb 100644
> > > > --- a/include/net/bluetooth/hci_core.h
> > > > +++ b/include/net/bluetooth/hci_core.h
> > > > @@ -641,6 +641,8 @@ struct hci_dev {
> > > >                                      struct bt_codec *codec, __u8 *vnd_len,
> > > >                                      __u8 **vnd_data);
> > > >         u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > +       int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
> > > > +       int (*read_usb_alt_setting)(struct hci_dev *hdev);
> > > >  };
> > > >
> > > >  #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > > > index 041ce9adc378..887aa1935b1e 100644
> > > > --- a/net/bluetooth/hci_sysfs.c
> > > > +++ b/net/bluetooth/hci_sysfs.c
> > > > @@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > > >  }
> > > >  static DEVICE_ATTR_WO(reset);
> > > >
> > > > +static ssize_t isoc_alt_show(struct device *dev,
> > > > +                            struct device_attribute *attr,
> > > > +                            char *buf)
> > > > +{
> > > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > > +
> > > > +       if (hdev->read_usb_alt_setting)
> > > > +               return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
> > > > +
> > > > +       return -ENODEV;
> > > > +}
> > > > +
> > > > +static ssize_t isoc_alt_store(struct device *dev,
> > > > +                             struct device_attribute *attr,
> > > > +                             const char *buf, size_t count)
> > > > +{
> > > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > > +       int alt;
> > > > +       int ret;
> > > > +
> > > > +       if (kstrtoint(buf, 10, &alt))
> > > > +               return -EINVAL;
> > > > +
> > > > +       if (hdev->switch_usb_alt_setting) {
> > > > +               ret = hdev->switch_usb_alt_setting(hdev, alt);
> > > > +               return ret < 0 ? ret : count;
> > > > +       }
> > > > +
> > > > +       return -ENODEV;
> > > > +}
> > > > +static DEVICE_ATTR_RW(isoc_alt);
> > > > +
> > > >  static struct attribute *bt_host_attrs[] = {
> > > >         &dev_attr_reset.attr,
> > > > +       &dev_attr_isoc_alt.attr,
> > > >         NULL,
> > > >  };
> > > >  ATTRIBUTE_GROUPS(bt_host);
> > >
> > > While this fixes the race it also forces the inclusion of an attribute
> > > that is driver specific, so I wonder if we should introduce some
> > > internal interface to register driver specific entries like this.
> >
> > Do you mean you prefer the original interface that only exports the
> > attribute when isoc_altsetting is supported?
> > Agree it makes more sense but I hit the obstacle: hci_init_sysfs is
> > called earlier than data->isoc is determined. I need some time to
> > verify whether changing the order won't break anything.
>
> We might have to do something like the following within hci_init_sysfs:
>
> if (hdev->isoc_alt)
>     dev->type = bt_host_isoc_alt
> else
>     dev->type = bt_host
>
> Now perhaps instead of adding the callbacks to hdev we add the
> attribute itself, btw did you check if there isn't a sysfs entry to
> interact with USB alternate settings? Because contrary to reset this
> actually operates directly on the driver bus so it sort of made more
> sense to me that this would be handled by USB rather than Bluetooth.

A quick git grep shows that this exists:

Documentation/ABI/testing/sysfs-bus-usb:What:
/sys/bus/usb/devices/usbX/bAlternateSetting

> > >
> > > > --
> > > > 2.48.1.262.g85cc9f2d1e-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Hsin-chen Chuang]

Hi Luiz,

Good point. Although the sysfs-bus-usb API only supports reading
rather than writing the alt setting, I'll look for the opportunity to
configure it through libusb first.

Thanks

On Sat, Jan 25, 2025 at 12:06 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Fri, Jan 24, 2025 at 10:54 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Hsin-chen,
> > > >
> > > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote:
> > > > >
> > > > > From: Hsin-chen Chuang <chharry@chromium.org>
> > > > >
> > > > > Use device group to avoid the racing. To reuse the group defined in
> > > > > hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
> > > > > read_usb_alt_setting which are only registered in btusb.
> > > > >
> > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
> > > > >  include/net/bluetooth/hci_core.h |  2 ++
> > > > >  net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
> > > > >  3 files changed, 45 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > index 75a0f15819c4..bf210275e5b7 100644
> > > > > --- a/drivers/bluetooth/btusb.c
> > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > @@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static int btusb_read_alt_setting(struct hci_dev *hdev)
> > > > > +{
> > > > > +       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > > +
> > > > > +       return data->isoc_altsetting;
> > > > > +}
> > > > > +
> > > > >  static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> > > > >                                                         int alt)
> > > > >  {
> > > > > @@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
> > > > >         .llseek         = default_llseek,
> > > > >  };
> > > > >
> > > > > -static ssize_t isoc_alt_show(struct device *dev,
> > > > > -                            struct device_attribute *attr,
> > > > > -                            char *buf)
> > > > > -{
> > > > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > > > -
> > > > > -       return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
> > > > > -}
> > > > > -
> > > > > -static ssize_t isoc_alt_store(struct device *dev,
> > > > > -                             struct device_attribute *attr,
> > > > > -                             const char *buf, size_t count)
> > > > > -{
> > > > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > > > -       int alt;
> > > > > -       int ret;
> > > > > -
> > > > > -       if (kstrtoint(buf, 10, &alt))
> > > > > -               return -EINVAL;
> > > > > -
> > > > > -       ret = btusb_switch_alt_setting(data->hdev, alt);
> > > > > -       return ret < 0 ? ret : count;
> > > > > -}
> > > > > -
> > > > > -static DEVICE_ATTR_RW(isoc_alt);
> > > > > -
> > > > >  static int btusb_probe(struct usb_interface *intf,
> > > > >                        const struct usb_device_id *id)
> > > > >  {
> > > > > @@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
> > > > >                 if (err < 0)
> > > > >                         goto out_free_dev;
> > > > >
> > > > > -               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> > > > > -               if (err)
> > > > > -                       goto out_free_dev;
> > > > > +               hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
> > > > > +               hdev->read_usb_alt_setting = btusb_read_alt_setting;
> > > > >         }
> > > > >
> > > > >         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> > > > > @@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> > > > >         hdev = data->hdev;
> > > > >         usb_set_intfdata(data->intf, NULL);
> > > > >
> > > > > -       if (data->isoc) {
> > > > > -               device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > > > > +       if (data->isoc)
> > > > >                 usb_set_intfdata(data->isoc, NULL);
> > > > > -       }
> > > > >
> > > > >         if (data->diag)
> > > > >                 usb_set_intfdata(data->diag, NULL);
> > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > > > index f756fac95488..5d3ec5ff5adb 100644
> > > > > --- a/include/net/bluetooth/hci_core.h
> > > > > +++ b/include/net/bluetooth/hci_core.h
> > > > > @@ -641,6 +641,8 @@ struct hci_dev {
> > > > >                                      struct bt_codec *codec, __u8 *vnd_len,
> > > > >                                      __u8 **vnd_data);
> > > > >         u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > > +       int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
> > > > > +       int (*read_usb_alt_setting)(struct hci_dev *hdev);
> > > > >  };
> > > > >
> > > > >  #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > > > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > > > > index 041ce9adc378..887aa1935b1e 100644
> > > > > --- a/net/bluetooth/hci_sysfs.c
> > > > > +++ b/net/bluetooth/hci_sysfs.c
> > > > > @@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > > > >  }
> > > > >  static DEVICE_ATTR_WO(reset);
> > > > >
> > > > > +static ssize_t isoc_alt_show(struct device *dev,
> > > > > +                            struct device_attribute *attr,
> > > > > +                            char *buf)
> > > > > +{
> > > > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > > > +
> > > > > +       if (hdev->read_usb_alt_setting)
> > > > > +               return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
> > > > > +
> > > > > +       return -ENODEV;
> > > > > +}
> > > > > +
> > > > > +static ssize_t isoc_alt_store(struct device *dev,
> > > > > +                             struct device_attribute *attr,
> > > > > +                             const char *buf, size_t count)
> > > > > +{
> > > > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > > > +       int alt;
> > > > > +       int ret;
> > > > > +
> > > > > +       if (kstrtoint(buf, 10, &alt))
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       if (hdev->switch_usb_alt_setting) {
> > > > > +               ret = hdev->switch_usb_alt_setting(hdev, alt);
> > > > > +               return ret < 0 ? ret : count;
> > > > > +       }
> > > > > +
> > > > > +       return -ENODEV;
> > > > > +}
> > > > > +static DEVICE_ATTR_RW(isoc_alt);
> > > > > +
> > > > >  static struct attribute *bt_host_attrs[] = {
> > > > >         &dev_attr_reset.attr,
> > > > > +       &dev_attr_isoc_alt.attr,
> > > > >         NULL,
> > > > >  };
> > > > >  ATTRIBUTE_GROUPS(bt_host);
> > > >
> > > > While this fixes the race it also forces the inclusion of an attribute
> > > > that is driver specific, so I wonder if we should introduce some
> > > > internal interface to register driver specific entries like this.
> > >
> > > Do you mean you prefer the original interface that only exports the
> > > attribute when isoc_altsetting is supported?
> > > Agree it makes more sense but I hit the obstacle: hci_init_sysfs is
> > > called earlier than data->isoc is determined. I need some time to
> > > verify whether changing the order won't break anything.
> >
> > We might have to do something like the following within hci_init_sysfs:
> >
> > if (hdev->isoc_alt)
> >     dev->type = bt_host_isoc_alt
> > else
> >     dev->type = bt_host
> >
> > Now perhaps instead of adding the callbacks to hdev we add the
> > attribute itself, btw did you check if there isn't a sysfs entry to
> > interact with USB alternate settings? Because contrary to reset this
> > actually operates directly on the driver bus so it sort of made more
> > sense to me that this would be handled by USB rather than Bluetooth.
>
> A quick git grep shows that this exists:
>
> Documentation/ABI/testing/sysfs-bus-usb:What:
> /sys/bus/usb/devices/usbX/bAlternateSetting
>
> > > >
> > > > > --
> > > > > 2.48.1.262.g85cc9f2d1e-goog
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v2 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Hsin-chen Chuang]

Hi Luiz,

On Fri, Jan 31, 2025 at 6:23 PM Hsin-chen Chuang <chharry@google.com> wrote:
>
> Hi Luiz,
>
> Good point. Although the sysfs-bus-usb API only supports reading
> rather than writing the alt setting, I'll look for the opportunity to
> configure it through libusb first.
>
> Thanks
>
> On Sat, Jan 25, 2025 at 12:06 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Fri, Jan 24, 2025 at 10:54 AM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Hsin-chen,
> > >
> > > On Wed, Jan 22, 2025 at 11:57 PM Hsin-chen Chuang <chharry@google.com> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Thu, Jan 23, 2025 at 3:35 AM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > Hi Hsin-chen,
> > > > >
> > > > > On Wed, Jan 22, 2025 at 12:20 AM Hsin-chen Chuang <chharry@google.com> wrote:
> > > > > >
> > > > > > From: Hsin-chen Chuang <chharry@chromium.org>
> > > > > >
> > > > > > Use device group to avoid the racing. To reuse the group defined in
> > > > > > hci_sysfs.c, defined 2 callbacks switch_usb_alt_setting and
> > > > > > read_usb_alt_setting which are only registered in btusb.
> > > > > >
> > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > >  drivers/bluetooth/btusb.c        | 42 ++++++++------------------------
> > > > > >  include/net/bluetooth/hci_core.h |  2 ++
> > > > > >  net/bluetooth/hci_sysfs.c        | 33 +++++++++++++++++++++++++
> > > > > >  3 files changed, 45 insertions(+), 32 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > > index 75a0f15819c4..bf210275e5b7 100644
> > > > > > --- a/drivers/bluetooth/btusb.c
> > > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > > @@ -2221,6 +2221,13 @@ static int btusb_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int btusb_read_alt_setting(struct hci_dev *hdev)
> > > > > > +{
> > > > > > +       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > > > +
> > > > > > +       return data->isoc_altsetting;
> > > > > > +}
> > > > > > +
> > > > > >  static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data,
> > > > > >                                                         int alt)
> > > > > >  {
> > > > > > @@ -3650,32 +3657,6 @@ static const struct file_operations force_poll_sync_fops = {
> > > > > >         .llseek         = default_llseek,
> > > > > >  };
> > > > > >
> > > > > > -static ssize_t isoc_alt_show(struct device *dev,
> > > > > > -                            struct device_attribute *attr,
> > > > > > -                            char *buf)
> > > > > > -{
> > > > > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > > > > -
> > > > > > -       return sysfs_emit(buf, "%d\n", data->isoc_altsetting);
> > > > > > -}
> > > > > > -
> > > > > > -static ssize_t isoc_alt_store(struct device *dev,
> > > > > > -                             struct device_attribute *attr,
> > > > > > -                             const char *buf, size_t count)
> > > > > > -{
> > > > > > -       struct btusb_data *data = dev_get_drvdata(dev);
> > > > > > -       int alt;
> > > > > > -       int ret;
> > > > > > -
> > > > > > -       if (kstrtoint(buf, 10, &alt))
> > > > > > -               return -EINVAL;
> > > > > > -
> > > > > > -       ret = btusb_switch_alt_setting(data->hdev, alt);
> > > > > > -       return ret < 0 ? ret : count;
> > > > > > -}
> > > > > > -
> > > > > > -static DEVICE_ATTR_RW(isoc_alt);
> > > > > > -
> > > > > >  static int btusb_probe(struct usb_interface *intf,
> > > > > >                        const struct usb_device_id *id)
> > > > > >  {
> > > > > > @@ -4040,9 +4021,8 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > >                 if (err < 0)
> > > > > >                         goto out_free_dev;
> > > > > >
> > > > > > -               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
> > > > > > -               if (err)
> > > > > > -                       goto out_free_dev;
> > > > > > +               hdev->switch_usb_alt_setting = btusb_switch_alt_setting;
> > > > > > +               hdev->read_usb_alt_setting = btusb_read_alt_setting;
> > > > > >         }
> > > > > >
> > > > > >         if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> > > > > > @@ -4089,10 +4069,8 @@ static void btusb_disconnect(struct usb_interface *intf)
> > > > > >         hdev = data->hdev;
> > > > > >         usb_set_intfdata(data->intf, NULL);
> > > > > >
> > > > > > -       if (data->isoc) {
> > > > > > -               device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> > > > > > +       if (data->isoc)
> > > > > >                 usb_set_intfdata(data->isoc, NULL);
> > > > > > -       }
> > > > > >
> > > > > >         if (data->diag)
> > > > > >                 usb_set_intfdata(data->diag, NULL);
> > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > > > > index f756fac95488..5d3ec5ff5adb 100644
> > > > > > --- a/include/net/bluetooth/hci_core.h
> > > > > > +++ b/include/net/bluetooth/hci_core.h
> > > > > > @@ -641,6 +641,8 @@ struct hci_dev {
> > > > > >                                      struct bt_codec *codec, __u8 *vnd_len,
> > > > > >                                      __u8 **vnd_data);
> > > > > >         u8 (*classify_pkt_type)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > > > +       int (*switch_usb_alt_setting)(struct hci_dev *hdev, int new_alts);
> > > > > > +       int (*read_usb_alt_setting)(struct hci_dev *hdev);
> > > > > >  };
> > > > > >
> > > > > >  #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > > > > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > > > > > index 041ce9adc378..887aa1935b1e 100644
> > > > > > --- a/net/bluetooth/hci_sysfs.c
> > > > > > +++ b/net/bluetooth/hci_sysfs.c
> > > > > > @@ -102,8 +102,41 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > > > > >  }
> > > > > >  static DEVICE_ATTR_WO(reset);
> > > > > >
> > > > > > +static ssize_t isoc_alt_show(struct device *dev,
> > > > > > +                            struct device_attribute *attr,
> > > > > > +                            char *buf)
> > > > > > +{
> > > > > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > > > > +
> > > > > > +       if (hdev->read_usb_alt_setting)
> > > > > > +               return sysfs_emit(buf, "%d\n", hdev->read_usb_alt_setting(hdev));
> > > > > > +
> > > > > > +       return -ENODEV;
> > > > > > +}
> > > > > > +
> > > > > > +static ssize_t isoc_alt_store(struct device *dev,
> > > > > > +                             struct device_attribute *attr,
> > > > > > +                             const char *buf, size_t count)
> > > > > > +{
> > > > > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > > > > +       int alt;
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       if (kstrtoint(buf, 10, &alt))
> > > > > > +               return -EINVAL;
> > > > > > +
> > > > > > +       if (hdev->switch_usb_alt_setting) {
> > > > > > +               ret = hdev->switch_usb_alt_setting(hdev, alt);
> > > > > > +               return ret < 0 ? ret : count;
> > > > > > +       }
> > > > > > +
> > > > > > +       return -ENODEV;
> > > > > > +}
> > > > > > +static DEVICE_ATTR_RW(isoc_alt);
> > > > > > +
> > > > > >  static struct attribute *bt_host_attrs[] = {
> > > > > >         &dev_attr_reset.attr,
> > > > > > +       &dev_attr_isoc_alt.attr,
> > > > > >         NULL,
> > > > > >  };
> > > > > >  ATTRIBUTE_GROUPS(bt_host);
> > > > >
> > > > > While this fixes the race it also forces the inclusion of an attribute
> > > > > that is driver specific, so I wonder if we should introduce some
> > > > > internal interface to register driver specific entries like this.
> > > >
> > > > Do you mean you prefer the original interface that only exports the
> > > > attribute when isoc_altsetting is supported?
> > > > Agree it makes more sense but I hit the obstacle: hci_init_sysfs is
> > > > called earlier than data->isoc is determined. I need some time to
> > > > verify whether changing the order won't break anything.
> > >
> > > We might have to do something like the following within hci_init_sysfs:
> > >
> > > if (hdev->isoc_alt)
> > >     dev->type = bt_host_isoc_alt
> > > else
> > >     dev->type = bt_host
> > >
> > > Now perhaps instead of adding the callbacks to hdev we add the
> > > attribute itself, btw did you check if there isn't a sysfs entry to
> > > interact with USB alternate settings? Because contrary to reset this
> > > actually operates directly on the driver bus so it sort of made more
> > > sense to me that this would be handled by USB rather than Bluetooth.

Unfortunately I tried the libusb API and it requires detaching the
kernel driver, which means the user channel would be broken. I'll send
out a new version which addresses the above comments, thanks.

> >
> > A quick git grep shows that this exists:
> >
> > Documentation/ABI/testing/sysfs-bus-usb:What:
> > /sys/bus/usb/devices/usbX/bAlternateSetting
> >
> > > > >
> > > > > > --
> > > > > > 2.48.1.262.g85cc9f2d1e-goog
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz

-- 
Best Regards,
Hsin-chen