2021-09-15 23:38:19

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [PATCH v4 2/4] Bluetooth: hci_sock: Replace use of memcpy_from_msg with bt_skb_sendmsg

From: Luiz Augusto von Dentz <[email protected]>

This makes use of bt_skb_sendmsg instead of allocating a different
buffer to be used with memcpy_from_msg which cause one extra copy.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
net/bluetooth/hci_sock.c | 100 +++++++++++++++------------------------
1 file changed, 37 insertions(+), 63 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index e481eee8e61e..39309ef6af4a 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -1510,7 +1510,8 @@ static int hci_sock_recvmsg(struct socket *sock, struct msghdr *msg,
return err ? : copied;
}

-static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, void *buf, size_t msglen)
+static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
+ struct sk_buff *skb)
{
u8 *cp;
struct mgmt_hdr *hdr;
@@ -1520,31 +1521,31 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, void *buf,
bool var_len, no_hdev;
int err;

- BT_DBG("got %zu bytes", msglen);
+ BT_DBG("got %d bytes", skb->len);

- if (msglen < sizeof(*hdr))
+ if (skb->len < sizeof(*hdr))
return -EINVAL;

- hdr = buf;
+ hdr = (void *)skb->data;
opcode = __le16_to_cpu(hdr->opcode);
index = __le16_to_cpu(hdr->index);
len = __le16_to_cpu(hdr->len);

- if (len != msglen - sizeof(*hdr)) {
+ if (len != skb->len - sizeof(*hdr)) {
err = -EINVAL;
goto done;
}

if (chan->channel == HCI_CHANNEL_CONTROL) {
- struct sk_buff *skb;
+ struct sk_buff *cmd;

/* Send event to monitor */
- skb = create_monitor_ctrl_command(sk, index, opcode, len,
- buf + sizeof(*hdr));
- if (skb) {
- hci_send_to_channel(HCI_CHANNEL_MONITOR, skb,
+ cmd = create_monitor_ctrl_command(sk, index, opcode, len,
+ skb->data + sizeof(*hdr));
+ if (cmd) {
+ hci_send_to_channel(HCI_CHANNEL_MONITOR, cmd,
HCI_SOCK_TRUSTED, NULL);
- kfree_skb(skb);
+ kfree_skb(cmd);
}
}

@@ -1609,13 +1610,13 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, void *buf,
if (hdev && chan->hdev_init)
chan->hdev_init(sk, hdev);

- cp = buf + sizeof(*hdr);
+ cp = skb->data + sizeof(*hdr);

err = handler->func(sk, hdev, cp, len);
if (err < 0)
goto done;

- err = msglen;
+ err = skb->len;

done:
if (hdev)
@@ -1624,10 +1625,10 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, void *buf,
return err;
}

-static int hci_logging_frame(struct sock *sk, void *buf, int len, unsigned int flags)
+static int hci_logging_frame(struct sock *sk, struct sk_buff *skb,
+ unsigned int flags)
{
struct hci_mon_hdr *hdr;
- struct sk_buff *skb;
struct hci_dev *hdev;
u16 index;
int err;
@@ -1636,21 +1637,13 @@ static int hci_logging_frame(struct sock *sk, void *buf, int len, unsigned int f
* the priority byte, the ident length byte and at least one string
* terminator NUL byte. Anything shorter are invalid packets.
*/
- if (len < sizeof(*hdr) + 3)
+ if (skb->len < sizeof(*hdr) + 3)
return -EINVAL;

- skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
- if (!skb)
- return err;
-
- memcpy(skb_put(skb, len), buf, len);
-
hdr = (void *)skb->data;

- if (__le16_to_cpu(hdr->len) != len - sizeof(*hdr)) {
- err = -EINVAL;
- goto drop;
- }
+ if (__le16_to_cpu(hdr->len) != skb->len - sizeof(*hdr))
+ return -EINVAL;

if (__le16_to_cpu(hdr->opcode) == 0x0000) {
__u8 priority = skb->data[sizeof(*hdr)];
@@ -1669,25 +1662,20 @@ static int hci_logging_frame(struct sock *sk, void *buf, int len, unsigned int f
* The message follows the ident string (if present) and
* must be NUL terminated. Otherwise it is not a valid packet.
*/
- if (priority > 7 || skb->data[len - 1] != 0x00 ||
- ident_len > len - sizeof(*hdr) - 3 ||
- skb->data[sizeof(*hdr) + ident_len + 1] != 0x00) {
- err = -EINVAL;
- goto drop;
- }
+ if (priority > 7 || skb->data[skb->len - 1] != 0x00 ||
+ ident_len > skb->len - sizeof(*hdr) - 3 ||
+ skb->data[sizeof(*hdr) + ident_len + 1] != 0x00)
+ return -EINVAL;
} else {
- err = -EINVAL;
- goto drop;
+ return -EINVAL;
}

index = __le16_to_cpu(hdr->index);

if (index != MGMT_INDEX_NONE) {
hdev = hci_dev_get(index);
- if (!hdev) {
- err = -ENODEV;
- goto drop;
- }
+ if (!hdev)
+ return -ENODEV;
} else {
hdev = NULL;
}
@@ -1695,13 +1683,11 @@ static int hci_logging_frame(struct sock *sk, void *buf, int len, unsigned int f
hdr->opcode = cpu_to_le16(HCI_MON_USER_LOGGING);

hci_send_to_channel(HCI_CHANNEL_MONITOR, skb, HCI_SOCK_TRUSTED, NULL);
- err = len;
+ err = skb->len;

if (hdev)
hci_dev_put(hdev);

-drop:
- kfree_skb(skb);
return err;
}

@@ -1713,7 +1699,6 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
struct hci_dev *hdev;
struct sk_buff *skb;
int err;
- void *buf;
const unsigned int flags = msg->msg_flags;

BT_DBG("sock %p sk %p", sock, sk);
@@ -1727,13 +1712,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
if (len < 4 || len > hci_pi(sk)->mtu)
return -EINVAL;

- buf = kmalloc(len, GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
- if (memcpy_from_msg(buf, msg, len)) {
- kfree(buf);
- return -EFAULT;
- }
+ skb = bt_skb_sendmsg(sk, msg, len, len, 0, 0);
+ if (IS_ERR(skb))
+ return PTR_ERR(skb);

lock_sock(sk);

@@ -1743,39 +1724,33 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
break;
case HCI_CHANNEL_MONITOR:
err = -EOPNOTSUPP;
- goto done;
+ goto drop;
case HCI_CHANNEL_LOGGING:
- err = hci_logging_frame(sk, buf, len, flags);
- goto done;
+ err = hci_logging_frame(sk, skb, flags);
+ goto drop;
default:
mutex_lock(&mgmt_chan_list_lock);
chan = __hci_mgmt_chan_find(hci_pi(sk)->channel);
if (chan)
- err = hci_mgmt_cmd(chan, sk, buf, len);
+ err = hci_mgmt_cmd(chan, sk, skb);
else
err = -EINVAL;

mutex_unlock(&mgmt_chan_list_lock);
- goto done;
+ goto drop;
}

hdev = hci_hdev_from_sock(sk);
if (IS_ERR(hdev)) {
err = PTR_ERR(hdev);
- goto done;
+ goto drop;
}

if (!test_bit(HCI_UP, &hdev->flags)) {
err = -ENETDOWN;
- goto done;
+ goto drop;
}

- skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
- if (!skb)
- goto done;
-
- memcpy(skb_put(skb, len), buf, len);
-
hci_skb_pkt_type(skb) = skb->data[0];
skb_pull(skb, 1);

@@ -1846,7 +1821,6 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,

done:
release_sock(sk);
- kfree(buf);
return err;

drop:
--
2.31.1


2021-09-16 00:07:54

by Tedd Ho-Jeong An

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] Bluetooth: hci_sock: Replace use of memcpy_from_msg with bt_skb_sendmsg

Hi Luiz

On Wed, 2021-09-15 at 16:35 -0700, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This makes use of bt_skb_sendmsg instead of allocating a different
> buffer to be used with memcpy_from_msg which cause one extra copy.
>
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> net/bluetooth/hci_sock.c | 100 +++++++++++++++------------------------
> 1 file changed, 37 insertions(+), 63 deletions(-)
>
Tested-by: Tedd Ho-Jeong An <[email protected]>