| 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: <21eded54-3460-4000-baba-815522012e02@molgen.mpg.de>
Date: Mon, 14 Jul 2025 16:58:58 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Pauli Virtanen <pav@....fi>
Cc: linux-bluetooth@...r.kernel.org, marcel@...tmann.org,
johan.hedberg@...il.com, luiz.dentz@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum
via CMSG
Dear Pauli,
Thank you for your prompt reply.
Am 14.07.25 um 16:53 schrieb Pauli Virtanen:
> ma, 2025-07-14 kello 16:15 +0200, Paul Menzel kirjoitti:
>> Am 14.07.25 um 16:02 schrieb Pauli Virtanen:
>>> User applications need a way to track which ISO interval a given SDU
>>> belongs to, to properly detect packet loss. All controllers do not set
>>> timestamps, and it's not guaranteed user application receives all packet
>>> reports (small socket buffer, or controller doesn't send all reports
>>> like Intel AX210 is doing).
>>>
>>> Add socket option BT_PKT_SEQNUM that enables reporting of received
>>> packet ISO sequence number in BT_SCM_PKT_SEQNUM CMSG.
>>
>> Are there user applications already supporting this, so it can be tested?
>
> I sent the associated tests to linux-bluetooth list
>
> https://lore.kernel.org/linux-bluetooth/c9a75585e3640d8a1efca0bf96158eec1ca25fdc.1752501450.git.pav@iki.fi/
Awesome. Can this be referenced in the commit message?
>>> Signed-off-by: Pauli Virtanen <pav@....fi>
>>> ---
>>>
>>> Notes:
>>> Intel AX210 is not sending all reports:
>>>
>>> $ btmon -r dump.btsnoop -I -C90|grep -A1 'ISO Data RX: Handle 2304'
>>> ...
>>> > ISO Data RX: Handle 2304 flags 0x02 dlen 64 #1713 [hci0] 22.567744
>>> dd 01 3c 00 6d 08 e9 14 1e 3b 85 7b 35 c2 25 0b ..<.m....;.{5.%.
>>> --
>>> > ISO Data RX: Handle 2304 flags 0x02 dlen 64 #1718 [hci0] 22.573745
>>> de 01 3c 00 41 65 22 4f 99 9b 0b b6 ff cb 06 00 ..<.Ae"O........
>>> --
>>> > ISO Data RX: Handle 2304 flags 0x02 dlen 64 #1727 [hci0] 22.587933
>>> e0 01 3c 00 8b 6e 33 44 65 51 ee d7 e0 ee 49 d8 ..<..n3DeQ....I.
>>> --
>>> > ISO Data RX: Handle 2304 flags 0x02 dlen 64 #1732 [hci0] 22.596742
>>> e1 01 3c 00 a7 48 54 a7 c1 9f dc 37 66 fe 04 ab ..<..HT....7f...
>>> ...
>>>
>>> Here, report for packet with sequence number 0x01df is missing.
>>
>> Sorry, but where are the sequence number in the trace?
>
> It's the first two bytes, see Core specification Vol 4E Sec 5.4.5 "HCI
> ISO Data packets".
Now I see it. Thank you!
>>>
>>> This may be spec violation by the controller, see Core v6.1 pp. 3702
>>>
>>> All SDUs shall be sent to the upper layer including the indication
>>> of validity of data. A report shall be sent to the upper layer if
>>> the SDU is completely missing.
>>>
>>> Regardless, it will be easier for user applications to see the HW
>>> sequence numbers directly, so they don't have to count packets and it's
>>> in any case more reliable if packets get dropped due to socket buffer
>>> size.
>>
>> I wouldn’t mind to have the note in the commit message.
>
> I'm not sure it's a spec violation --- the text in the specification is
> not fully clear what "All SDUs" means in the context here --- so I
> don't really want to say so in the commit message.
>
> The limited socket buffer and that AX210 drops some reports is mentioned
> in the commit message.
True.
>>> include/net/bluetooth/bluetooth.h | 9 ++++++++-
>>> net/bluetooth/af_bluetooth.c | 7 +++++++
>>> net/bluetooth/iso.c | 21 ++++++++++++++++++---
>>> 3 files changed, 33 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>>> index 114299bd8b98..0e31779a3341 100644
>>> --- a/include/net/bluetooth/bluetooth.h
>>> +++ b/include/net/bluetooth/bluetooth.h
>>> @@ -244,6 +244,10 @@ struct bt_codecs {
>>>
>>> #define BT_ISO_BASE 20
>>>
>>> +#define BT_PKT_SEQNUM 21
>>> +
>>> +#define BT_SCM_PKT_SEQNUM 0x05
>>> +
>>> __printf(1, 2)
>>> void bt_info(const char *fmt, ...);
>>> __printf(1, 2)
>>> @@ -391,7 +395,8 @@ struct bt_sock {
>>> enum {
>>> BT_SK_DEFER_SETUP,
>>> BT_SK_SUSPEND,
>>> - BT_SK_PKT_STATUS
>>> + BT_SK_PKT_STATUS,
>>> + BT_SK_PKT_SEQNUM,
>>> };
>>>
>>> struct bt_sock_list {
>>> @@ -475,6 +480,7 @@ struct bt_skb_cb {
>>> u8 pkt_type;
>>> u8 force_active;
>>> u16 expect;
>>> + u16 pkt_seqnum;
>>
>> Excuse my ignorance, just want to make sure, the type is big enough.
>
> The hardware sequence number is also 16 bits.
Understood.
>>> u8 incoming:1;
>>> u8 pkt_status:2;
>>> union {
>>> @@ -488,6 +494,7 @@ struct bt_skb_cb {
>>>
>>> #define hci_skb_pkt_type(skb) bt_cb((skb))->pkt_type
>>> #define hci_skb_pkt_status(skb) bt_cb((skb))->pkt_status
>>> +#define hci_skb_pkt_seqnum(skb) bt_cb((skb))->pkt_seqnum
>>> #define hci_skb_expect(skb) bt_cb((skb))->expect
>>> #define hci_skb_opcode(skb) bt_cb((skb))->hci.opcode
>>> #define hci_skb_event(skb) bt_cb((skb))->hci.req_event
>>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>>> index 6ad2f72f53f4..44b7acb20a67 100644
>>> --- a/net/bluetooth/af_bluetooth.c
>>> +++ b/net/bluetooth/af_bluetooth.c
>>> @@ -364,6 +364,13 @@ int bt_sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>>> put_cmsg(msg, SOL_BLUETOOTH, BT_SCM_PKT_STATUS,
>>> sizeof(pkt_status), &pkt_status);
>>> }
>>> +
>>> + if (test_bit(BT_SK_PKT_SEQNUM, &bt_sk(sk)->flags)) {
>>> + u16 pkt_seqnum = hci_skb_pkt_seqnum(skb);
>>> +
>>> + put_cmsg(msg, SOL_BLUETOOTH, BT_SCM_PKT_SEQNUM,
>>> + sizeof(pkt_seqnum), &pkt_seqnum);
>>> + }
>>> }
>>>
>>> skb_free_datagram(sk, skb);
>>> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
>>> index fc22782cbeeb..469450bb6b6c 100644
>>> --- a/net/bluetooth/iso.c
>>> +++ b/net/bluetooth/iso.c
>>> @@ -1687,6 +1687,17 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>>> clear_bit(BT_SK_PKT_STATUS, &bt_sk(sk)->flags);
>>> break;
>>>
>>> + case BT_PKT_SEQNUM:
>>> + err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
>>> + if (err)
>>> + break;
>>> +
>>> + if (opt)
>>> + set_bit(BT_SK_PKT_SEQNUM, &bt_sk(sk)->flags);
>>> + else
>>> + clear_bit(BT_SK_PKT_SEQNUM, &bt_sk(sk)->flags);
>>> + break;
>>> +
>>> case BT_ISO_QOS:
>>> if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND &&
>>> sk->sk_state != BT_CONNECT2 &&
>>> @@ -2278,7 +2289,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;
>>> - __u16 pb, ts, len;
>>> + __u16 pb, ts, len, sn;
>>
>> Use `seqnum` for consistency with the parts above.
>>
>>>
>>> if (!conn)
>>> goto drop;
>>> @@ -2308,6 +2319,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>>> goto drop;
>>> }
>>>
>>> + sn = hdr->sn;
>>> len = __le16_to_cpu(hdr->slen);
>>> } else {
>>> struct hci_iso_data_hdr *hdr;
>>> @@ -2318,18 +2330,20 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>>> goto drop;
>>> }
>>>
>>> + sn = hdr->sn;
>>> len = __le16_to_cpu(hdr->slen);
>>> }
>>>
>>> flags = hci_iso_data_flags(len);
>>> len = hci_iso_data_len(len);
>>>
>>> - BT_DBG("Start: total len %d, frag len %d flags 0x%4.4x", len,
>>> - skb->len, flags);
>>> + BT_DBG("Start: total len %d, frag len %d flags 0x%4.4x sn %d",
>>> + len, skb->len, flags, sn);
>>>
>>> if (len == skb->len) {
>>> /* Complete frame received */
>>> hci_skb_pkt_status(skb) = flags & 0x03;
>>> + hci_skb_pkt_seqnum(skb) = sn;
>>> iso_recv_frame(conn, skb);
>>> return;
>>> }
>>> @@ -2352,6 +2366,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>>> goto drop;
>>>
>>> hci_skb_pkt_status(conn->rx_skb) = flags & 0x03;
>>> + hci_skb_pkt_seqnum(conn->rx_skb) = sn;
>>> skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
>>> skb->len);
>>> conn->rx_len = len - skb->len;
Kind regards,
Paul
Powered by blists - more mailing lists