| 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: <CAL+tcoCHfAbxCLjF2M-K6Zqj9q1rCFuXcdgO8Eb_tBMDsSYAxw@mail.gmail.com>
Date: Mon, 10 Feb 2025 13:39:00 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Pauli Virtanen <pav@....fi>
Cc: linux-bluetooth@...r.kernel.org,
Luiz Augusto von Dentz <luiz.dentz@...il.com>, netdev@...r.kernel.org, davem@...emloft.net,
kuba@...nel.org, willemdebruijn.kernel@...il.com
Subject: Re: [PATCH v3 2/5] Bluetooth: add support for skb TX SND/COMPLETION timestamping
On Sun, Feb 9, 2025 at 6:40 PM Pauli Virtanen <pav@....fi> wrote:
>
> Support enabling TX timestamping for some skbs, and track them until
> packet completion. Generate software SCM_TSTAMP_COMPLETION when getting
> completion report from hardware.
>
> Generate software SCM_TSTAMP_SND before sending to driver. Sending from
> driver requires changes in the driver API, and drivers mostly are going
> to send the skb immediately.
>
> Make the default situation with no COMPLETION TX timestamping more
> efficient by only counting packets in the queue when there is nothing to
> track. When there is something to track, we need to make clones, since
> the driver may modify sent skbs.
>
> The tx_q queue length is bounded by the hdev flow control, which will
> not send new packets before it has got completion reports for old ones.
>
> Signed-off-by: Pauli Virtanen <pav@....fi>
> ---
> include/net/bluetooth/hci_core.h | 13 ++++
> net/bluetooth/hci_conn.c | 117 +++++++++++++++++++++++++++++++
> net/bluetooth/hci_core.c | 17 +++--
> net/bluetooth/hci_event.c | 4 ++
> 4 files changed, 146 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 05919848ea95..1f539a9881ad 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -261,6 +261,12 @@ struct adv_info {
> struct delayed_work rpa_expired_cb;
> };
>
> +struct tx_queue {
> + struct sk_buff_head queue;
> + unsigned int extra;
> + unsigned int tracked;
> +};
> +
> #define HCI_MAX_ADV_INSTANCES 5
> #define HCI_DEFAULT_ADV_DURATION 2
>
> @@ -733,6 +739,8 @@ struct hci_conn {
> struct sk_buff_head data_q;
> struct list_head chan_list;
>
> + struct tx_queue tx_q;
> +
> struct delayed_work disc_work;
> struct delayed_work auto_accept_work;
> struct delayed_work idle_work;
> @@ -1571,6 +1579,11 @@ void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
> void hci_conn_failed(struct hci_conn *conn, u8 status);
> u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle);
>
> +void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb);
> +void hci_conn_tx_dequeue(struct hci_conn *conn);
> +void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
> + const struct sockcm_cookie *sockc);
> +
> /*
> * hci_conn_get() and hci_conn_put() are used to control the life-time of an
> * "hci_conn" object. They do not guarantee that the hci_conn object is running,
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index d097e308a755..e437290d8b70 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -27,6 +27,7 @@
>
> #include <linux/export.h>
> #include <linux/debugfs.h>
> +#include <linux/errqueue.h>
>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -1002,6 +1003,7 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t
> }
>
> skb_queue_head_init(&conn->data_q);
> + skb_queue_head_init(&conn->tx_q.queue);
>
> INIT_LIST_HEAD(&conn->chan_list);
> INIT_LIST_HEAD(&conn->link_list);
> @@ -1155,6 +1157,7 @@ void hci_conn_del(struct hci_conn *conn)
> }
>
> skb_queue_purge(&conn->data_q);
> + skb_queue_purge(&conn->tx_q.queue);
>
> /* Remove the connection from the list and cleanup its remaining
> * state. This is a separate function since for some cases like
> @@ -3064,3 +3067,117 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
> */
> return hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
> }
> +
> +void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
> + const struct sockcm_cookie *sockc)
> +{
> + struct sock *sk = skb ? skb->sk : NULL;
> +
> + /* This shall be called on a single skb of those generated by user
> + * sendmsg(), and only when the sendmsg() does not return error to
> + * user. This is required for keeping the tskey that increments here in
> + * sync with possible sendmsg() counting by user.
> + *
> + * Stream sockets shall set key_offset to sendmsg() length in bytes
> + * and call with the last fragment, others to 1 and first fragment.
> + */
> +
> + if (!skb || !sockc || !sk || !key_offset)
> + return;
> +
> + sock_tx_timestamp(sk, sockc, &skb_shinfo(skb)->tx_flags);
> +
> + if (sockc->tsflags & SOF_TIMESTAMPING_OPT_ID &&
> + sockc->tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) {
> + if (sockc->tsflags & SOCKCM_FLAG_TS_OPT_ID) {
> + skb_shinfo(skb)->tskey = sockc->ts_opt_id;
Will applications take control of managing the tskey on their own
instead of kernel in the future?
> + } else {
> + int key = atomic_add_return(key_offset, &sk->sk_tskey);
> +
> + skb_shinfo(skb)->tskey = key - 1;
> + }
> + }
> +}
> +
> +void hci_conn_tx_queue(struct hci_conn *conn, struct sk_buff *skb)
> +{
> + struct tx_queue *comp = &conn->tx_q;
> + bool track = false;
> +
> + /* Emit SND now, ie. just before sending to driver */
> + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP))
Nit: I think it's not necessary to test if skb->sk is NULL here since
__skb_tstamp_tx() will do the check.
> + __skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SND);
> +
> + /* COMPLETION tstamp is emitted for tracked skb later in Number of
> + * Completed Packets event. Available only for flow controlled cases.
> + *
> + * TODO: SCO support (needs to be done in drivers)
> + */
> + switch (conn->type) {
> + case ISO_LINK:
> + case ACL_LINK:
> + case LE_LINK:
> + break;
> + default:
> + return;
> + }
> +
> + if (skb->sk && (skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP))
> + track = true;
> +
> + /* If nothing is tracked, just count extra skbs at the queue head */
> + if (!track && !comp->tracked) {
> + comp->extra++;
> + return;
> + }
> +
> + if (track) {
> + skb = skb_clone_sk(skb);
> + if (!skb)
> + goto count_only;
> +
> + comp->tracked++;
> + } else {
> + skb = skb_clone(skb, GFP_KERNEL);
> + if (!skb)
> + goto count_only;
> + }
> +
> + skb_queue_tail(&comp->queue, skb);
> + return;
> +
> +count_only:
> + /* Stop tracking skbs, and only count. This will not emit timestamps for
> + * the packets, but if we get here something is more seriously wrong.
> + */
> + comp->tracked = 0;
> + comp->extra += skb_queue_len(&comp->queue) + 1;
> + skb_queue_purge(&comp->queue);
> +}
> +
> +void hci_conn_tx_dequeue(struct hci_conn *conn)
> +{
> + struct tx_queue *comp = &conn->tx_q;
> + struct sk_buff *skb;
> +
> + /* If there are tracked skbs, the counted extra go before dequeuing real
> + * skbs, to keep ordering. When nothing is tracked, the ordering doesn't
> + * matter so dequeue real skbs first to get rid of them ASAP.
> + */
> + if (comp->extra && (comp->tracked || skb_queue_empty(&comp->queue))) {
> + comp->extra--;
> + return;
> + }
> +
> + skb = skb_dequeue(&comp->queue);
> + if (!skb)
> + return;
> +
> + if (skb->sk) {
> + comp->tracked--;
> + __skb_tstamp_tx(skb, NULL, NULL, skb->sk,
> + SCM_TSTAMP_COMPLETION);
> + }
> +
> + kfree_skb(skb);
> +}
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index e7ec12437c8b..e0845188f626 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3025,6 +3025,13 @@ static int hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> return 0;
> }
>
> +static int hci_send_conn_frame(struct hci_dev *hdev, struct hci_conn *conn,
> + struct sk_buff *skb)
> +{
> + hci_conn_tx_queue(conn, skb);
> + return hci_send_frame(hdev, skb);
> +}
> +
> /* Send HCI command */
> int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen,
> const void *param)
> @@ -3562,7 +3569,7 @@ static void hci_sched_sco(struct hci_dev *hdev)
> while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, "e))) {
> while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> BT_DBG("skb %p len %d", skb, skb->len);
> - hci_send_frame(hdev, skb);
> + hci_send_conn_frame(hdev, conn, skb);
>
> conn->sent++;
> if (conn->sent == ~0)
> @@ -3586,7 +3593,7 @@ static void hci_sched_esco(struct hci_dev *hdev)
> "e))) {
> while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> BT_DBG("skb %p len %d", skb, skb->len);
> - hci_send_frame(hdev, skb);
> + hci_send_conn_frame(hdev, conn, skb);
>
> conn->sent++;
> if (conn->sent == ~0)
> @@ -3620,7 +3627,7 @@ static void hci_sched_acl_pkt(struct hci_dev *hdev)
> hci_conn_enter_active_mode(chan->conn,
> bt_cb(skb)->force_active);
>
> - hci_send_frame(hdev, skb);
> + hci_send_conn_frame(hdev, chan->conn, skb);
> hdev->acl_last_tx = jiffies;
>
> hdev->acl_cnt--;
> @@ -3676,7 +3683,7 @@ static void hci_sched_le(struct hci_dev *hdev)
>
> skb = skb_dequeue(&chan->data_q);
>
> - hci_send_frame(hdev, skb);
> + hci_send_conn_frame(hdev, chan->conn, skb);
> hdev->le_last_tx = jiffies;
>
> (*cnt)--;
> @@ -3710,7 +3717,7 @@ static void hci_sched_iso(struct hci_dev *hdev)
> while (*cnt && (conn = hci_low_sent(hdev, ISO_LINK, "e))) {
> while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> BT_DBG("skb %p len %d", skb, skb->len);
> - hci_send_frame(hdev, skb);
> + hci_send_conn_frame(hdev, conn, skb);
>
> conn->sent++;
> if (conn->sent == ~0)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 2cc7a9306350..144b442180f7 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4405,6 +4405,7 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
> struct hci_comp_pkts_info *info = &ev->handles[i];
> struct hci_conn *conn;
> __u16 handle, count;
> + unsigned int i;
>
> handle = __le16_to_cpu(info->handle);
> count = __le16_to_cpu(info->count);
> @@ -4415,6 +4416,9 @@ static void hci_num_comp_pkts_evt(struct hci_dev *hdev, void *data,
>
> conn->sent -= count;
>
> + for (i = 0; i < count; ++i)
> + hci_conn_tx_dequeue(conn);
> +
> switch (conn->type) {
> case ACL_LINK:
> hdev->acl_cnt += count;
> --
> 2.48.1
>
>
Powered by blists - more mailing lists