netdev - Re: pull request: bluetooth-next 2024-05-10

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

Message-ID: <CABBYNZJqwGo6VmTsSHFveRLy7pYQ0PJ+4X_aaL9r5if0dyqf6A@mail.gmail.com>
Date: Mon, 13 May 2024 21:34:36 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, davem@...emloft.net, 
	linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org, 
	Pauli Virtanen <pav@....fi>
Subject: Re: pull request: bluetooth-next 2024-05-10

Hi Jakub,

On Mon, May 13, 2024 at 6:43 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 13 May 2024 18:09:31 -0400 Luiz Augusto von Dentz wrote:
> > > There is one more warning in the Intel driver:
> > >
> > > drivers/bluetooth/btintel_pcie.c:673:33: warning: symbol 'causes_list'
> > > was not declared. Should it be static?
> >
> > We have a fix for that but I was hoping to have it in before the merge
> > window and then have the fix merged later.
> >
> > > It'd also be great to get an ACK from someone familiar with the socket
> > > time stamping (Willem?) I'm not sure there's sufficient detail in the
> > > commit message to explain the choices to:
> > >  - change the definition of SCHED / SEND to mean queued / completed,
> > >    while for Ethernet they mean queued to qdisc, queued to HW.
> >
> > hmm I thought this was hardware specific, it obviously won't work
> > exactly as Ethernet since it is a completely different protocol stack,
> > or are you suggesting we need other definitions for things like TX
> > completed?
>
> I don't know anything about queuing in BT, in terms of timestamping
> the SEND - SCHED difference is supposed to indicate the level of
> host delay or host congestion. If the queuing in BT happens mostly in
> the device HW queue then it may make sense to generate SCHED when
> handing over to the driver. OTOH if the devices can coalesce or delay
> completions the completion timeout may be less accurate than stamping
> before submitting to HW... I'm looking for the analysis that the choices
> were well thought thru.

I guess you want to know if is SCHED is done at enqueing (before
submitting to the driver) or dequeing (after it has been submitted),
right now it is the former, the said the driver should normally just
submit the packets immediately since we do have events from HCI to
informing when a buffer has been freed, so that tells HW queue
situation, so the driver doesn't have any queueing.

> > >    How does it compare to stamping in the driver in terms of accuracy?
> >
> > @Pauli any input here?
> >
> > >  - the "experimental" BT_POLL_ERRQUEUE, how does the user space look?
> >
> > There are test cases in BlueZ:
> >
> > https://github.com/bluez/bluez/commit/141f66411ca488e26bdd64e6f858ffa190395d23
> >
> > >    What is the "upper layer"? What does it mean for kernel uAPI to be
> > >    "experimental"? When does the "upper layer" get to run and how does
> > >    it know that there are time stamps on the error queue?
> >
> > The socketopt only gets enabled with use of MGMT Set Experimental
> > Feature Command:
> >
> > https://github.com/bluez/bluez/blob/master/doc/mgmt-api.txt#L3205
> >
> > Anyway you can see on the tests how we are using it.
>
> Either I didn't grok the test or it doesn't answer my question.
> What is the lower layer that we want to "protect" from POLLERR?

This is more or less explained on the cover letter:

https://lore.kernel.org/linux-bluetooth/713b1d0333eb2f12e63bc8a7b8f423e1240abae0.camel@iki.fi/T/

The problem with POLLERR is that there are 2 processes (bluetoothd and
pipewire) monitoring the same file descriptor, so it will keep waking
up both processes to process the errqueue while only one is really
reading those events (pipewire), bluetoothd is only really monitoring
the sockets to know if there is a disconnection but it can't really
process the errqueue otherwise pipewire would compete reading from the
same queue. Anyway I wasn't sure  this was the best approach thus we
decided to go with something experimental until we have a better
understanding how all of this should work.

-- 
Luiz Augusto von Dentz