| 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
| ||
|
Message-ID: <CABBYNZKKJE05c3037Pab-GpJK0P2NoYNm=eYa9g93NpshEaHXg@mail.gmail.com>
Date: Tue, 29 Apr 2025 10:38:40 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Pauli Virtanen <pav@....fi>
Cc: yang.li@...ogic.com, linux-bluetooth@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH] iso: add BT_ISO_TS optional to enable ISO timestamp
Hi Pauli,
On Tue, Apr 29, 2025 at 10:35 AM Pauli Virtanen <pav@....fi> wrote:
>
> Hi,
>
> 29. huhtikuuta 2025 17.31.25 GMT+03:00 Luiz Augusto von Dentz <luiz.dentz@...il.com> kirjoitti:
> >Hi Pauli,
> >
> >On Tue, Apr 29, 2025 at 10:29 AM Pauli Virtanen <pav@....fi> wrote:
> >>
> >> ti, 2025-04-29 kello 17:26 +0300, Pauli Virtanen kirjoitti:
> >> > Hi,
> >> >
> >> > ti, 2025-04-29 kello 11:35 +0800, Yang Li via B4 Relay kirjoitti:
> >> > > From: Yang Li <yang.li@...ogic.com>
> >> > >
> >> > > Application layer programs (like pipewire) need to use
> >> > > iso timestamp information for audio synchronization.
> >> >
> >> > I think the timestamp should be put into CMSG, same ways as packet
> >> > status is. The packet body should then always contain only the payload.
> >>
> >> Or, this maybe should instead use the SOF_TIMESTAMPING_RX_HARDWARE
> >> mechanism, which would avoid adding a new API.
> >
> >Either that or we use BT_PKT_STATUS, does SOF_TIMESTAMPING_RX_HARDWARE
> >requires the use of errqueue?
>
> No, it just adds a CMSG, similar to the RX software tstamp.
Perfect, then there should be no problem going with that, we might
want to introduce some tests for it and perhaps have the emulator
adding timestamps headers so we can test this with the likes of
iso-tester.
> >
> >> > >
> >> > > Signed-off-by: Yang Li <yang.li@...ogic.com>
> >> > > ---
> >> > > include/net/bluetooth/bluetooth.h | 4 ++-
> >> > > net/bluetooth/iso.c | 58 +++++++++++++++++++++++++++++++++------
> >> > > 2 files changed, 52 insertions(+), 10 deletions(-)
> >> > >
> >> > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> >> > > index bbefde319f95..a102bd76647c 100644
> >> > > --- a/include/net/bluetooth/bluetooth.h
> >> > > +++ b/include/net/bluetooth/bluetooth.h
> >> > > @@ -242,6 +242,7 @@ struct bt_codecs {
> >> > > #define BT_CODEC_MSBC 0x05
> >> > >
> >> > > #define BT_ISO_BASE 20
> >> > > +#define BT_ISO_TS 21
> >> > >
> >> > > __printf(1, 2)
> >> > > void bt_info(const char *fmt, ...);
> >> > > @@ -390,7 +391,8 @@ struct bt_sock {
> >> > > enum {
> >> > > BT_SK_DEFER_SETUP,
> >> > > BT_SK_SUSPEND,
> >> > > - BT_SK_PKT_STATUS
> >> > > + BT_SK_PKT_STATUS,
> >> > > + BT_SK_ISO_TS
> >> > > };
> >> > >
> >> > > struct bt_sock_list {
> >> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> >> > > index 2f348f48e99d..2c1fdea4b8c1 100644
> >> > > --- a/net/bluetooth/iso.c
> >> > > +++ b/net/bluetooth/iso.c
> >> > > @@ -1718,7 +1718,21 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
> >> > > iso_pi(sk)->base_len = optlen;
> >> > >
> >> > > break;
> >> > > + case BT_ISO_TS:
> >> > > + if (optlen != sizeof(opt)) {
> >> > > + err = -EINVAL;
> >> > > + break;
> >> > > + }
> >> > >
> >> > > + err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> >> > > + if (err)
> >> > > + break;
> >> > > +
> >> > > + if (opt)
> >> > > + set_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> >> > > + else
> >> > > + clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> >> > > + break;
> >> > > default:
> >> > > err = -ENOPROTOOPT;
> >> > > break;
> >> > > @@ -1789,7 +1803,16 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
> >> > > err = -EFAULT;
> >> > >
> >> > > break;
> >> > > + case BT_ISO_TS:
> >> > > + if (len < sizeof(u32)) {
> >> > > + err = -EINVAL;
> >> > > + break;
> >> > > + }
> >> > >
> >> > > + if (put_user(test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags),
> >> > > + (u32 __user *)optval))
> >> > > + err = -EFAULT;
> >> > > + break;
> >> > > default:
> >> > > err = -ENOPROTOOPT;
> >> > > break;
> >> > > @@ -2271,13 +2294,21 @@ 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 sock *sk;
> >> > > __u16 pb, ts, len;
> >> > >
> >> > > if (!conn)
> >> > > goto drop;
> >> > >
> >> > > - pb = hci_iso_flags_pb(flags);
> >> > > - ts = hci_iso_flags_ts(flags);
> >> > > + iso_conn_lock(conn);
> >> > > + sk = conn->sk;
> >> > > + iso_conn_unlock(conn);
> >> > > +
> >> > > + if (!sk)
> >> > > + goto drop;
> >> > > +
> >> > > + pb = hci_iso_flags_pb(flags);
> >> > > + ts = hci_iso_flags_ts(flags);
> >> > >
> >> > > BT_DBG("conn %p len %d pb 0x%x ts 0x%x", conn, skb->len, pb, ts);
> >> > >
> >> > > @@ -2294,17 +2325,26 @@ 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;
> >> > > - }
> >> > > + if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> >> > > + hdr = (struct hci_iso_ts_data_hdr *)skb->data;
> >> > > + len = hdr->slen + HCI_ISO_TS_DATA_HDR_SIZE;
> >> > > + } else {
> >> > > + 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;
> >> > > + }
> >> > >
> >> > > - len = __le16_to_cpu(hdr->slen);
> >> > > + len = __le16_to_cpu(hdr->slen);
> >> > > + }
> >> > > } else {
> >> > > struct hci_iso_data_hdr *hdr;
> >> > >
> >> > > + if (test_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags)) {
> >> > > + BT_ERR("Invalid option BT_SK_ISO_TS");
> >> > > + clear_bit(BT_SK_ISO_TS, &bt_sk(sk)->flags);
> >> > > + }
> >> > > +
> >> > > hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
> >> > > if (!hdr) {
> >> > > BT_ERR("Frame is too short (len %d)", skb->len);
> >> > >
> >> > > ---
> >> > > base-commit: 16b4f97defefd93cfaea017a7c3e8849322f7dde
> >> > > change-id: 20250421-iso_ts-c82a300ae784
> >> > >
> >> > > Best regards,
> >
> >
> >
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists