Message-ID: <CADg1FFcQzRbVhx+hcWpjzkTCzbE2wY+KLMYz0O-NUUOfv=XyDg@mail.gmail.com>
Date: Wed, 19 Feb 2025 09:54:09 +0800
From: Hsin-chen Chuang <chharry@...gle.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: linux-bluetooth@...r.kernel.org, luiz.dentz@...il.com, 
	chromeos-bluetooth-upstreaming@...omium.org, 
	Hsin-chen Chuang <chharry@...omium.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Johan Hedberg <johan.hedberg@...il.com>, Marcel Holtmann <marcel@...tmann.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Ying Hsu <yinghsu@...omium.org>, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v5] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

Hi Greg,

On Tue, Feb 18, 2025 at 8:24 PM Hsin-chen Chuang <chharry@...gle.com> wrote:
>
> Hi Greg,
>
> On Tue, Feb 18, 2025 at 6:56 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> >
> > On Tue, Feb 18, 2025 at 06:01:42PM +0800, Hsin-chen Chuang wrote:
> > > Hi Greg,
> > >
> > > On Tue, Feb 18, 2025 at 5:21 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > >
> > > > On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Tue, Feb 18, 2025 at 4:23 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > > > > > > From: Hsin-chen Chuang <chharry@...omium.org>
> > > > > > > > > > >
> > > > > > > > > > > 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")
> > > > > > > > > >
> > > > > > > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > > > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > > > > > > the bluetooth stack at all?
> > > > > > > > >
> > > > > > > > > In short: We want to configure the alternate settings without
> > > > > > > > > detaching the btusb driver, while detaching seems necessary for
> > > > > > > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > > > > > > wrong!)
> > > > > > > >
> > > > > > > > I think changing the alternate setting should work using usbfs as you
> > > > > > > > would send that command to the device, not the interface, so the driver
> > > > > > > > bound to the existing interface would not need to be removed.
> > > > > > >
> > > > > > > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > > > > > > but it seems not working in this case.
> > > > > > > The command itself attempts to claim the interface, but the interface
> > > > > > > is already claimed by btusb so it failed with Device or resource busy
> > > > > > >
> > > > > > > drivers/usb/core/devio.c:
> > > > > > >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
> > > > > >
> > > > > > Ah, ok, thanks for checking.  So as you control this device, why not
> > > > > > just disconnect it, change the setting, and then reconnect it?
> > > > >
> > > > > After dis/reconnecting, a Bluetooth chipset would lose all its state:
> > > > > Existing connections/scanners/advertisers are all dropped.
> > > >
> > > > If you are changing the alternate USB configuration, all state should be
> > > > dropped, right?  If not, huh how does the device know to keep that
> > > > state?
> > >
> > > No, the Bluetooth chip doesn't drop any info when the alt is changed.
> > > It only affects the data transfer bandwidth on that interface.
> > >
> > > >
> > > > > This is as bad as (just an analogy) "Whenever you access a http web
> > > > > page, you need to bring your ethernet interface down and up, and after
> > > > > the page is downloaded, do that again".
> > > >
> > > > Your ethernet interface does not contain state like this, we handle
> > > > chainging IP addresses and devices all the time, so perhaps wrong
> > > > analogy :)
> > > >
> > > > > > Also, see my other review comment, how does BlueZ do this today?
> > > > >
> > > > > BlueZ handles that in their MGMT command, that is, through Control
> > > > > channel -> BlueZ kernel space code -> driver callbacks.
> > > > > Once a Bluetooth chipset is opened with the User channel, it can't be
> > > > > used with the Control channel simultaneously, and vice versa.
> > > >
> > > > So why not use that same control channel in your code?  Why are you
> > >
> > > Because we're using the User channel, and they can't be used at the same time.
> >
> > This doesn't make sense.  Either BlueZ has this same problem, or it
> > doesn't.  As you say it does not, then again, why can't you use the
> > exact same user/kernel api to achieve this?
> >
> > The user/kernel api is "fixed" right now, if you wish to replace the
> > userspace side of the BlueZ code with your own, then you should/must use
> > that same user/kernel api.  Don't go adding duplicate interfaces please.
>
> I would say the kernel provides 2 sets of the API, Control and User,
> and now the User channel is missing something.
> I think it makes sense to add support for it.
>
> >
> > > > reinventing a new control channel for something that is obviously there
> > > > already?
> > >
> > > Not quite the same as "reinventing". The Control channel command does
> > > much more than just setting the alt; It just doesn't work with the
> > > User channel.
> > >
> > > >
> > > > So in short, what's preventing you from using the same exact driver
> > > > callbacks, OR the same exact kernel api.  Surely you all are not
> > >
> > > The answer is the same as the above. This feature is missing in the
> > > User channel, and I'm completing it with this patch.
> >
> > Again, that seems to be your userspace's issue, not the kernel's.  Just
> > use the same api that bluez uses here.
> >
> > > > replacing all of the in-kernel BlueZ code with an external kernel driver
> > > > just for this, right?  If so, that's not ok at all.
> > >
> > > Sorry I don't quite get it. What do you mean by the external kernel driver?
> >
> > You said you are not using the bluez kernel code, right?  So you must
> > have some kernel code to implement this instead for the same
> > functionality.  Otherwise you are using the bluez kernel api here.
>
> No, we don't have kernel code for Bluetooth. We have everything in user space.
>
> >
> > Again, just use the same api please, don't go adding new one-off apis
> > through sysfs for this when it is not needed.
> >
> > I'll also step back further and say, why not use bluez?  What is so
> > wrong with that that you all need a totally different bluetooth stack?
> > Why not just fix the bluez code for anything that is currently missing
> > or lacking there that required you to write a new one.
>
> I think the main purpose is moving the stack to the user space.
> When the user hits a Bluetooth issue, it's much easier to reset the stack.
> Also, a simple Bluetooth bug just won't crash your kernel and we could
> even crash MORE to detect an incorrect chipset behavior earlier.
> Of course BlueZ has its own advantages, but it's just all trade-offs.
>
> >
> > And yes, I know the inclination of Android to constantly rewrite the
> > bluetooth stack, it's on the what, third or fourth iteration already?
> > What's to guarantee that this really will be the last one?  :)
>
> That's incorrect. Android is still using and maintaining the same
> stack since it left BlueZ.
>
> --
> Best Regards,
> Hsin-chen

I'm moving forward and sending out v6 for review as there's no more
comment related to the struct device usage.
We could continue the discussion on v6, thanks.

-- 
Best Regards,
Hsin-chen

Powered by blists - more mailing lists