Thread (15 messages) 15 messages, 2 authors, 2025-02-19

Re: [PATCH v5] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

From: Hsin-chen Chuang <hidden>
Date: 2025-02-14 11:24:51
Also in: linux-bluetooth, lkml

On Fri, Feb 14, 2025 at 7:16 PM Hsin-chen Chuang [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Hsin-chen Chuang <redacted>

Expose the isoc_alt attr with device group to avoid the racing.

Now we create a dev node for btusb. The isoc_alt attr belongs to it and
it also becomes the parent device of hci dev.

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

Changes in v5:
- Merge the ABI doc into this patch
- Manage the driver data with device

Changes in v4:
- Create a dev node for btusb. It's now hci dev's parent and the
  isoc_alt now belongs to it.
- Since the changes is almost limitted in btusb, no need to add the
  callbacks in hdev anymore.

Changes in v3:
- Make the attribute exported only when the isoc_alt is available.
- In btusb_probe, determine data->isoc before calling hci_alloc_dev_priv
  (which calls hci_init_sysfs).
- Since hci_init_sysfs is called before btusb could modify the hdev,
  add new argument add_isoc_alt_attr for btusb to inform hci_init_sysfs.

Changes in v2:
- The patch has been removed from series

 .../ABI/stable/sysfs-class-bluetooth          |  13 ++
 drivers/bluetooth/btusb.c                     | 111 ++++++++++++++----
 include/net/bluetooth/hci_core.h              |   1 +
 net/bluetooth/hci_sysfs.c                     |   3 +-
 4 files changed, 102 insertions(+), 26 deletions(-)
diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
index 36be02471174..c1024c7c4634 100644
--- a/Documentation/ABI/stable/sysfs-class-bluetooth
+++ b/Documentation/ABI/stable/sysfs-class-bluetooth
@@ -7,3 +7,16 @@ 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/btusb<usb-intf>/isoc_alt
+Date:          13-Feb-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 is not yet init-ed, the write fails with -ENODEV.
+               If the data is not a valid number, the write fails with -EINVAL.
+               The other failures are vendor specific.
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1caf7a071a73..e2fb3d08a5ed 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -920,6 +920,8 @@ struct btusb_data {
        int oob_wake_irq;   /* irq for out-of-band wake-on-bt */

        struct qca_dump_info qca_dump;
+
+       struct device dev;
 };

 static void btusb_reset(struct hci_dev *hdev)
@@ -3693,6 +3695,9 @@ static ssize_t isoc_alt_store(struct device *dev,
        int alt;
        int ret;

+       if (!data->hdev)
+               return -ENODEV;
+
        if (kstrtoint(buf, 10, &alt))
                return -EINVAL;
@@ -3702,6 +3707,36 @@ static ssize_t isoc_alt_store(struct device *dev,

 static DEVICE_ATTR_RW(isoc_alt);

+static struct attribute *btusb_sysfs_attrs[] = {
+       NULL,
+};
+ATTRIBUTE_GROUPS(btusb_sysfs);
+
+static void btusb_sysfs_release(struct device *dev)
+{
+       struct btusb_data *data = dev_get_drvdata(dev);
+
+       kfree(data);
+}
+
+static const struct device_type btusb_sysfs = {
+       .name    = "btusb",
+       .release = btusb_sysfs_release,
+       .groups  = btusb_sysfs_groups,
+};
+
+static struct attribute *btusb_sysfs_isoc_alt_attrs[] = {
+       &dev_attr_isoc_alt.attr,
+       NULL,
+};
+ATTRIBUTE_GROUPS(btusb_sysfs_isoc_alt);
+
+static const struct device_type btusb_sysfs_isoc_alt = {
+       .name    = "btusb",
+       .release = btusb_sysfs_release,
+       .groups  = btusb_sysfs_isoc_alt_groups,
+};
+
 static int btusb_probe(struct usb_interface *intf,
                       const struct usb_device_id *id)
 {
@@ -3743,7 +3778,7 @@ static int btusb_probe(struct usb_interface *intf,
                        return -ENODEV;
        }

-       data = devm_kzalloc(&intf->dev, sizeof(*data), GFP_KERNEL);
+       data = kzalloc(sizeof(*data), GFP_KERNEL);
        if (!data)
                return -ENOMEM;
@@ -3766,8 +3801,10 @@ static int btusb_probe(struct usb_interface *intf,
                }
        }

-       if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep)
-               return -ENODEV;
+       if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep) {
+               err = -ENODEV;
+               goto out_free_data;
+       }

        if (id->driver_info & BTUSB_AMP) {
                data->cmdreq_type = USB_TYPE_CLASS | 0x01;
@@ -3821,16 +3858,47 @@ static int btusb_probe(struct usb_interface *intf,

        data->recv_acl = hci_recv_frame;

+       if (id->driver_info & BTUSB_AMP) {
+               /* AMP controllers do not support SCO packets */
+               data->isoc = NULL;
+       } else {
+               /* Interface orders are hardcoded in the specification */
+               data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
+               data->isoc_ifnum = ifnum_base + 1;
+       }
+
+       if (id->driver_info & BTUSB_BROKEN_ISOC)
+               data->isoc = NULL;
+
+       /* Init a dev for btusb. The attr depends on the support of isoc. */
+       if (data->isoc)
+               data->dev.type = &btusb_sysfs_isoc_alt;
+       else
+               data->dev.type = &btusb_sysfs;
+       data->dev.class = &bt_class;
+       data->dev.parent = &intf->dev;
+
+       err = dev_set_name(&data->dev, "btusb%s", dev_name(&intf->dev));
+       if (err)
+               goto out_free_data;
+
+       dev_set_drvdata(&data->dev, data);
+       err = device_register(&data->dev);
+       if (err < 0)
+               goto out_put_sysfs;
+
        hdev = hci_alloc_dev_priv(priv_size);
-       if (!hdev)
-               return -ENOMEM;
+       if (!hdev) {
+               err = -ENOMEM;
+               goto out_free_sysfs;
+       }

        hdev->bus = HCI_USB;
        hci_set_drvdata(hdev, data);

        data->hdev = hdev;

-       SET_HCIDEV_DEV(hdev, &intf->dev);
+       SET_HCIDEV_DEV(hdev, &data->dev);

        reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
                                        GPIOD_OUT_LOW);
@@ -3969,15 +4037,6 @@ static int btusb_probe(struct usb_interface *intf,
                hci_set_msft_opcode(hdev, 0xFD70);
        }

-       if (id->driver_info & BTUSB_AMP) {
-               /* AMP controllers do not support SCO packets */
-               data->isoc = NULL;
-       } else {
-               /* Interface orders are hardcoded in the specification */
-               data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
-               data->isoc_ifnum = ifnum_base + 1;
-       }
-
        if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) &&
            (id->driver_info & BTUSB_REALTEK)) {
                btrtl_set_driver_name(hdev, btusb_driver.name);
@@ -4010,9 +4069,6 @@ static int btusb_probe(struct usb_interface *intf,
                        set_bit(HCI_QUIRK_FIXUP_BUFFER_SIZE, &hdev->quirks);
        }

-       if (id->driver_info & BTUSB_BROKEN_ISOC)
-               data->isoc = NULL;
-
        if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
                set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
@@ -4065,10 +4121,6 @@ static int btusb_probe(struct usb_interface *intf,
                                                 data->isoc, data);
                if (err < 0)
                        goto out_free_dev;
-
-               err = device_create_file(&intf->dev, &dev_attr_isoc_alt);
-               if (err)
-                       goto out_free_dev;
        }

        if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
@@ -4099,6 +4151,16 @@ static int btusb_probe(struct usb_interface *intf,
        if (data->reset_gpio)
                gpiod_put(data->reset_gpio);
        hci_free_dev(hdev);
+
+out_free_sysfs:
+       device_del(&data->dev);
+
+out_put_sysfs:
+       put_device(&data->dev);
+       return err;
+
+out_free_data:
+       kfree(data);
        return err;
 }
@@ -4115,10 +4177,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);
@@ -4150,6 +4210,7 @@ static void btusb_disconnect(struct usb_interface *intf)
                gpiod_put(data->reset_gpio);

        hci_free_dev(hdev);
+       device_unregister(&data->dev);
 }

 #ifdef CONFIG_PM
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 05919848ea95..776dd6183509 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1843,6 +1843,7 @@ int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);

 void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);

+extern const struct class bt_class;
 void hci_init_sysfs(struct hci_dev *hdev);
 void hci_conn_init_sysfs(struct hci_conn *conn);
 void hci_conn_add_sysfs(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 041ce9adc378..aab3ffaa264c 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -6,9 +6,10 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>

-static const struct class bt_class = {
+const struct class bt_class = {
        .name = "bluetooth",
 };
+EXPORT_SYMBOL(bt_class);

 static void bt_link_release(struct device *dev)
 {
--
2.48.1.601.g30ceb7b040-goog
Ouch, please kindly ignore this patch. I missed some discussion in V4

-- 
Best Regards,
Hsin-chen
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help