2021-07-22 07:44:13

by Tetsuo Handa

Subject: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()

Since userfaultfd mechanism allows sleeping with kernel lock held,
avoiding page fault with kernel lock held where possible will make
the module more robust. This patch just brings memcpy_from_msg() calls
to out of sock lock.

Signed-off-by: Tetsuo Handa <[email protected]>
---
net/bluetooth/hci_sock.c | 50 +++++++++++++++++-----------------------
1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index ef7fc3e9d471..7fac44fb771f 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -1535,10 +1535,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,
- struct msghdr *msg, size_t msglen)
+static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, void *buf, size_t msglen)
{
- void *buf;
u8 *cp;
struct mgmt_hdr *hdr;
u16 opcode, index, len;
@@ -1552,15 +1550,6 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
if (msglen < sizeof(*hdr))
return -EINVAL;

- buf = kmalloc(msglen, GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
-
- if (memcpy_from_msg(buf, msg, msglen)) {
- err = -EFAULT;
- goto done;
- }
-
hdr = buf;
opcode = __le16_to_cpu(hdr->opcode);
index = __le16_to_cpu(hdr->index);
@@ -1657,11 +1646,10 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
if (hdev)
hci_dev_put(hdev);

- kfree(buf);
return err;
}

-static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len)
+static int hci_logging_frame(struct sock *sk, void *buf, int len, unsigned int flags)
{
struct hci_mon_hdr *hdr;
struct sk_buff *skb;
@@ -1676,14 +1664,11 @@ static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len)
if (len < sizeof(*hdr) + 3)
return -EINVAL;

- skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
+ skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
if (!skb)
return err;

- if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
- err = -EFAULT;
- goto drop;
- }
+ memcpy(skb_put(skb, len), buf, len);

hdr = (void *)skb->data;

@@ -1753,19 +1738,28 @@ 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);

- if (msg->msg_flags & MSG_OOB)
+ if (flags & MSG_OOB)
return -EOPNOTSUPP;

- if (msg->msg_flags & ~(MSG_DONTWAIT|MSG_NOSIGNAL|MSG_ERRQUEUE|
- MSG_CMSG_COMPAT))
+ if (flags & ~(MSG_DONTWAIT | MSG_NOSIGNAL | MSG_ERRQUEUE | MSG_CMSG_COMPAT))
return -EINVAL;

if (len < 4 || len > HCI_MAX_FRAME_SIZE)
return -EINVAL;

+ buf = kmalloc(len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+ if (memcpy_from_msg(buf, msg, len)) {
+ kfree(buf);
+ return -EFAULT;
+ }
+
lock_sock(sk);

switch (hci_pi(sk)->channel) {
@@ -1776,13 +1770,13 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
err = -EOPNOTSUPP;
goto done;
case HCI_CHANNEL_LOGGING:
- err = hci_logging_frame(sk, msg, len);
+ err = hci_logging_frame(sk, buf, len, flags);
goto done;
default:
mutex_lock(&mgmt_chan_list_lock);
chan = __hci_mgmt_chan_find(hci_pi(sk)->channel);
if (chan)
- err = hci_mgmt_cmd(chan, sk, msg, len);
+ err = hci_mgmt_cmd(chan, sk, buf, len);
else
err = -EINVAL;

@@ -1801,14 +1795,11 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
goto done;
}

- skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
+ skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
if (!skb)
goto done;

- if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
- err = -EFAULT;
- goto drop;
- }
+ memcpy(skb_put(skb, len), buf, len);

hci_skb_pkt_type(skb) = skb->data[0];
skb_pull(skb, 1);
@@ -1880,6 +1871,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,

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

drop:
--
2.18.4

2021-07-23 21:46:14

by Luiz Augusto von Dentz

Subject: Re: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()

Hi Tetsuo,

On Thu, Jul 22, 2021 at 12:42 AM Tetsuo Handa
<[email protected]> wrote:
>
> Since userfaultfd mechanism allows sleeping with kernel lock held,
> avoiding page fault with kernel lock held where possible will make
> the module more robust. This patch just brings memcpy_from_msg() calls
> to out of sock lock.
>
> Signed-off-by: Tetsuo Handa <[email protected]>

Reviewed-by: Luiz Augusto von Dentz <[email protected]>

> ---
> net/bluetooth/hci_sock.c | 50 +++++++++++++++++-----------------------
> 1 file changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index ef7fc3e9d471..7fac44fb771f 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -1535,10 +1535,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,
> - struct msghdr *msg, size_t msglen)
> +static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, void *buf, size_t msglen)
> {
> - void *buf;
> u8 *cp;
> struct mgmt_hdr *hdr;
> u16 opcode, index, len;
> @@ -1552,15 +1550,6 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
> if (msglen < sizeof(*hdr))
> return -EINVAL;
>
> - buf = kmalloc(msglen, GFP_KERNEL);
> - if (!buf)
> - return -ENOMEM;
> -
> - if (memcpy_from_msg(buf, msg, msglen)) {
> - err = -EFAULT;
> - goto done;
> - }
> -
> hdr = buf;
> opcode = __le16_to_cpu(hdr->opcode);
> index = __le16_to_cpu(hdr->index);
> @@ -1657,11 +1646,10 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
> if (hdev)
> hci_dev_put(hdev);
>
> - kfree(buf);
> return err;
> }
>
> -static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len)
> +static int hci_logging_frame(struct sock *sk, void *buf, int len, unsigned int flags)
> {
> struct hci_mon_hdr *hdr;
> struct sk_buff *skb;
> @@ -1676,14 +1664,11 @@ static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len)
> if (len < sizeof(*hdr) + 3)
> return -EINVAL;
>
> - skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
> + skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
> if (!skb)
> return err;
>
> - if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> - err = -EFAULT;
> - goto drop;
> - }
> + memcpy(skb_put(skb, len), buf, len);
>
> hdr = (void *)skb->data;
>
> @@ -1753,19 +1738,28 @@ 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);
>
> - if (msg->msg_flags & MSG_OOB)
> + if (flags & MSG_OOB)
> return -EOPNOTSUPP;
>
> - if (msg->msg_flags & ~(MSG_DONTWAIT|MSG_NOSIGNAL|MSG_ERRQUEUE|
> - MSG_CMSG_COMPAT))
> + if (flags & ~(MSG_DONTWAIT | MSG_NOSIGNAL | MSG_ERRQUEUE | MSG_CMSG_COMPAT))
> return -EINVAL;
>
> if (len < 4 || len > HCI_MAX_FRAME_SIZE)
> return -EINVAL;
>
> + buf = kmalloc(len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> + if (memcpy_from_msg(buf, msg, len)) {
> + kfree(buf);
> + return -EFAULT;
> + }
> +
> lock_sock(sk);
>
> switch (hci_pi(sk)->channel) {
> @@ -1776,13 +1770,13 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> err = -EOPNOTSUPP;
> goto done;
> case HCI_CHANNEL_LOGGING:
> - err = hci_logging_frame(sk, msg, len);
> + err = hci_logging_frame(sk, buf, len, flags);
> goto done;
> default:
> mutex_lock(&mgmt_chan_list_lock);
> chan = __hci_mgmt_chan_find(hci_pi(sk)->channel);
> if (chan)
> - err = hci_mgmt_cmd(chan, sk, msg, len);
> + err = hci_mgmt_cmd(chan, sk, buf, len);
> else
> err = -EINVAL;
>
> @@ -1801,14 +1795,11 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> goto done;
> }
>
> - skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
> + skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
> if (!skb)
> goto done;
>
> - if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> - err = -EFAULT;
> - goto drop;
> - }
> + memcpy(skb_put(skb, len), buf, len);
>
> hci_skb_pkt_type(skb) = skb->data[0];
> skb_pull(skb, 1);
> @@ -1880,6 +1871,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
> done:
> release_sock(sk);
> + kfree(buf);
> return err;
>
> drop:
> --
> 2.18.4
>


--
Luiz Augusto von Dentz

2021-08-22 15:45:02

by Tetsuo Handa

Subject: Re: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()

On 2021/07/24 6:44, Luiz Augusto von Dentz wrote:
> Hi Tetsuo,
>
> On Thu, Jul 22, 2021 at 12:42 AM Tetsuo Handa
> <[email protected]> wrote:
>>
>> Since userfaultfd mechanism allows sleeping with kernel lock held,
>> avoiding page fault with kernel lock held where possible will make
>> the module more robust. This patch just brings memcpy_from_msg() calls
>> to out of sock lock.
>>
>> Signed-off-by: Tetsuo Handa <[email protected]>
>
> Reviewed-by: Luiz Augusto von Dentz <[email protected]>
>

Can this patch go to v5.15 ?

2021-08-23 21:05:07

by Luiz Augusto von Dentz

Subject: Re: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()

Hi Tetsuo,

On Sun, Aug 22, 2021 at 8:42 AM Tetsuo Handa
<[email protected]> wrote:
>
> On 2021/07/24 6:44, Luiz Augusto von Dentz wrote:
> > Hi Tetsuo,
> >
> > On Thu, Jul 22, 2021 at 12:42 AM Tetsuo Handa
> > <[email protected]> wrote:
> >>
> >> Since userfaultfd mechanism allows sleeping with kernel lock held,
> >> avoiding page fault with kernel lock held where possible will make
> >> the module more robust. This patch just brings memcpy_from_msg() calls
> >> to out of sock lock.
> >>
> >> Signed-off-by: Tetsuo Handa <[email protected]>
> >
> > Reviewed-by: Luiz Augusto von Dentz <[email protected]>
> >
>
> Can this patch go to v5.15 ?

Applied, thanks. I will be sending another pull-request by the end of
this week which shall include it.

--
Luiz Augusto von Dentz