linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()
@ 2021-07-22  7:42 Tetsuo Handa
  2021-07-22  9:09 ` bluez.test.bot
  2021-07-23 21:44 ` [PATCH] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 5+ messages in thread
From: Tetsuo Handa @ 2021-07-22  7:42 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz
  Cc: linux-bluetooth, LinMa, Tetsuo Handa

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 <penguin-kernel@I-love.SAKURA.ne.jp>
---
 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: Bluetooth: reorganize functions from hci_sock_sendmsg()
  2021-07-22  7:42 [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg() Tetsuo Handa
@ 2021-07-22  9:09 ` bluez.test.bot
  2021-07-23 21:44 ` [PATCH] " Luiz Augusto von Dentz
  1 sibling, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2021-07-22  9:09 UTC (permalink / raw)
  To: linux-bluetooth, penguin-kernel

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=519543

---Test result---

Test Summary:
CheckPatch                    PASS      0.48 seconds
GitLint                       PASS      0.10 seconds
BuildKernel                   PASS      531.77 seconds
TestRunner: Setup             PASS      350.46 seconds
TestRunner: l2cap-tester      PASS      2.52 seconds
TestRunner: bnep-tester       PASS      1.88 seconds
TestRunner: mgmt-tester       PASS      31.41 seconds
TestRunner: rfcomm-tester     PASS      2.11 seconds
TestRunner: sco-tester        PASS      2.03 seconds
TestRunner: smp-tester        FAIL      2.10 seconds
TestRunner: userchan-tester   PASS      1.94 seconds

Details
##############################
Test: CheckPatch - PASS - 0.48 seconds
Run checkpatch.pl script with rule in .checkpatch.conf


##############################
Test: GitLint - PASS - 0.10 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 531.77 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 350.46 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.52 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.88 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 31.41 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.11 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.03 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.10 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.023 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 1.94 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44350 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3557 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 616827 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11677 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9912 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11705 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5454 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()
  2021-07-22  7:42 [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg() Tetsuo Handa
  2021-07-22  9:09 ` bluez.test.bot
@ 2021-07-23 21:44 ` Luiz Augusto von Dentz
  2021-08-22 15:42   ` Tetsuo Handa
  1 sibling, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-23 21:44 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: Marcel Holtmann, Johan Hedberg, linux-bluetooth, LinMa

Hi Tetsuo,

On Thu, Jul 22, 2021 at 12:42 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp>

Reviewed-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

> ---
>  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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()
  2021-07-23 21:44 ` [PATCH] " Luiz Augusto von Dentz
@ 2021-08-22 15:42   ` Tetsuo Handa
  2021-08-23 21:03     ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 5+ messages in thread
From: Tetsuo Handa @ 2021-08-22 15:42 UTC (permalink / raw)
  To: Luiz Augusto von Dentz, Marcel Holtmann, Johan Hedberg
  Cc: linux-bluetooth, LinMa

On 2021/07/24 6:44, Luiz Augusto von Dentz wrote:
> Hi Tetsuo,
> 
> On Thu, Jul 22, 2021 at 12:42 AM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> Reviewed-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 

Can this patch go to v5.15 ?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()
  2021-08-22 15:42   ` Tetsuo Handa
@ 2021-08-23 21:03     ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2021-08-23 21:03 UTC (permalink / raw)
  To: Tetsuo Handa; +Cc: Marcel Holtmann, Johan Hedberg, linux-bluetooth, LinMa

Hi Tetsuo,

On Sun, Aug 22, 2021 at 8:42 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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
> > <penguin-kernel@i-love.sakura.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp>
> >
> > Reviewed-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
>
> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-23 21:04 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22  7:42 [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg() Tetsuo Handa
2021-07-22  9:09 ` bluez.test.bot
2021-07-23 21:44 ` [PATCH] " Luiz Augusto von Dentz
2021-08-22 15:42   ` Tetsuo Handa
2021-08-23 21:03     ` Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).