netdev - Re: [PATCH v4] Bluetooth: ISO: Support SCM

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: <CABBYNZLFnbfdXjRV0taeTNF5bsey-WFf4TFsf_ox0FNuJbEutw@mail.gmail.com>
Date: Tue, 15 Jul 2025 09:37:28 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Pauli Virtanen <pav@....fi>
Cc: yang.li@...ogic.com, Marcel Holtmann <marcel@...tmann.org>, 
	Johan Hedberg <johan.hedberg@...il.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] Bluetooth: ISO: Support SCM_TIMESTAMPING for ISO TS

Hi Pauli,

On Tue, Jul 15, 2025 at 9:30 AM Pauli Virtanen <pav@....fi> wrote:
>
> Hi Yang,
>
> ma, 2025-07-07 kello 10:38 +0800, Yang Li via B4 Relay kirjoitti:
> > From: Yang Li <yang.li@...ogic.com>
> >
> > User-space applications (e.g. PipeWire) depend on
> > ISO-formatted timestamps for precise audio sync.
> >
> > The ISO ts is based on the controller’s clock domain,
> > so hardware timestamping (hwtimestamp) must be used.
> >
> > Ref: Documentation/networking/timestamping.rst,
> > section 3.1 Hardware Timestamping.
> >
> > Signed-off-by: Yang Li <yang.li@...ogic.com>
> > ---
> > Changes in v4:
> > - Optimizing the code
> > - Link to v3: https://lore.kernel.org/r/20250704-iso_ts-v3-1-2328bc602961@amlogic.com
> >
> > Changes in v3:
> > - Change to use hwtimestamp
> > - Link to v2: https://lore.kernel.org/r/20250702-iso_ts-v2-1-723d199c8068@amlogic.com
> >
> > Changes in v2:
> > - Support SOCK_RCVTSTAMPNS via CMSG for ISO sockets
> > - Link to v1: https://lore.kernel.org/r/20250429-iso_ts-v1-1-e586f30de6cb@amlogic.com
> > ---
> >  net/bluetooth/iso.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index fc22782cbeeb..677144bb6b94 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -2278,6 +2278,7 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
> >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >  {
> >       struct iso_conn *conn = hcon->iso_data;
> > +     struct skb_shared_hwtstamps *hwts;
> >       __u16 pb, ts, len;
> >
> >       if (!conn)
> > @@ -2301,13 +2302,16 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >               if (ts) {
> >                       struct hci_iso_ts_data_hdr *hdr;
> >
> > -                     /* TODO: add timestamp to the packet? */
> >                       hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> >                       if (!hdr) {
> >                               BT_ERR("Frame is too short (len %d)", skb->len);
> >                               goto drop;
> >                       }
> >
> > +                     /*  Record the timestamp to skb*/
> > +                     hwts = skb_hwtstamps(skb);
> > +                     hwts->hwtstamp = us_to_ktime(le32_to_cpu(hdr->ts));
>
> Several lines below there is
>
>         conn->rx_skb = bt_skb_alloc(len, GFP_KERNEL);
>         skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb-
> >len),
>                                                   skb->len);
>
> so timestamp should be copied explicitly also into conn->rx_skb,
> otherwise it gets lost when you have ACL-fragmented ISO packets.

Yep, it is not that the code is completely wrong but it is operating
on the original skb not in the rx_skb as you said, that said is only
the first fragment that contains the ts header so we only have to do
it once in case that was not clear.

> It could also be useful to write a simple test case that extracts the
> timestamp from CMSG, see for example how it was done for BT_PKT_SEQNUM:
> https://lore.kernel.org/linux-bluetooth/b98b7691e4ba06550bb8f275cad0635bc9e4e8d2.1752511478.git.pav@iki.fi/
> bthost_send_iso() can take ts=true and some timestamp value.
>
> > +
> >                       len = __le16_to_cpu(hdr->slen);
> >               } else {
> >                       struct hci_iso_data_hdr *hdr;
> >
> > ---
> > base-commit: b8db3a9d4daeb7ff6a56c605ad6eca24e4da78ed
> > change-id: 20250421-iso_ts-c82a300ae784
> >
> > Best regards,
>
> --
> Pauli Virtanen



-- 
Luiz Augusto von Dentz