diff mbox series

[v2] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

Message ID 39b677ce-dcbf-6393-6279-88ed3a9e570e@i-love.sakura.ne.jp (mailing list archive)
State Superseded
Headers show
Series [v2] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl() | expand

Commit Message

Tetsuo Handa July 20, 2021, 3:02 p.m. UTC
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 copy_{from,to}_user()
calls to out of hdev lock and sock lock.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v2:
  Rename get_link_mode() to hci_get_link_mode() to avoid
  symbol name collision.

 include/net/bluetooth/hci_core.h |   3 +-
 net/bluetooth/hci_conn.c         |  52 +---------
 net/bluetooth/hci_sock.c         | 165 ++++++++++++++++++++-----------
 3 files changed, 109 insertions(+), 111 deletions(-)

Comments

bluez.test.bot@gmail.com July 20, 2021, 4:09 p.m. UTC | #1
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=518371

---Test result---

Test Summary:
CheckPatch                    FAIL      0.82 seconds
GitLint                       PASS      0.10 seconds
BuildKernel                   PASS      509.45 seconds
TestRunner: Setup             PASS      336.66 seconds
TestRunner: l2cap-tester      PASS      2.55 seconds
TestRunner: bnep-tester       PASS      1.88 seconds
TestRunner: mgmt-tester       PASS      30.35 seconds
TestRunner: rfcomm-tester     PASS      2.05 seconds
TestRunner: sco-tester        PASS      1.99 seconds
TestRunner: smp-tester        FAIL      2.03 seconds
TestRunner: userchan-tester   PASS      1.92 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.82 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()
WARNING: From:/Signed-off-by: email address mismatch: 'From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>' != 'Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>'

total: 0 errors, 1 warnings, 0 checks, 295 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


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


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


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


##############################
Test: TestRunner: l2cap-tester - PASS - 2.55 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 - 30.35 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

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

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

##############################
Test: TestRunner: smp-tester - FAIL - 2.03 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.022 seconds

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



---
Regards,
Linux Bluetooth
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a53e94459ecd..654677f59887 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1261,8 +1261,7 @@  int hci_dev_cmd(unsigned int cmd, void __user *arg);
 int hci_get_dev_list(void __user *arg);
 int hci_get_dev_info(void __user *arg);
 int hci_get_conn_list(void __user *arg);
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg);
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg);
+u32 hci_get_link_mode(struct hci_conn *conn);
 int hci_inquiry(void __user *arg);
 
 struct bdaddr_list *hci_bdaddr_list_lookup(struct list_head *list,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 2b5059a56cda..ea2b538537aa 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1626,7 +1626,7 @@  void hci_conn_check_pending(struct hci_dev *hdev)
 	hci_dev_unlock(hdev);
 }
 
-static u32 get_link_mode(struct hci_conn *conn)
+u32 hci_get_link_mode(struct hci_conn *conn)
 {
 	u32 link_mode = 0;
 
@@ -1683,7 +1683,7 @@  int hci_get_conn_list(void __user *arg)
 		(ci + n)->type  = c->type;
 		(ci + n)->out   = c->out;
 		(ci + n)->state = c->state;
-		(ci + n)->link_mode = get_link_mode(c);
+		(ci + n)->link_mode = hci_get_link_mode(c);
 		if (++n >= req.conn_num)
 			break;
 	}
@@ -1701,54 +1701,6 @@  int hci_get_conn_list(void __user *arg)
 	return err ? -EFAULT : 0;
 }
 
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
-{
-	struct hci_conn_info_req req;
-	struct hci_conn_info ci;
-	struct hci_conn *conn;
-	char __user *ptr = arg + sizeof(req);
-
-	if (copy_from_user(&req, arg, sizeof(req)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
-	conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
-	if (conn) {
-		bacpy(&ci.bdaddr, &conn->dst);
-		ci.handle = conn->handle;
-		ci.type  = conn->type;
-		ci.out   = conn->out;
-		ci.state = conn->state;
-		ci.link_mode = get_link_mode(conn);
-	}
-	hci_dev_unlock(hdev);
-
-	if (!conn)
-		return -ENOENT;
-
-	return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
-}
-
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
-{
-	struct hci_auth_info_req req;
-	struct hci_conn *conn;
-
-	if (copy_from_user(&req, arg, sizeof(req)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
-	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
-	if (conn)
-		req.type = conn->auth_type;
-	hci_dev_unlock(hdev);
-
-	if (!conn)
-		return -ENOENT;
-
-	return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
-}
-
 struct hci_chan *hci_chan_create(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..ef7fc3e9d471 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -892,82 +892,136 @@  static int hci_sock_release(struct socket *sock)
 	return 0;
 }
 
-static int hci_sock_reject_list_add(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_add(struct hci_dev *hdev, bdaddr_t *bdaddr)
 {
-	bdaddr_t bdaddr;
-	int err;
-
-	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
-
-	err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
-
-	hci_dev_unlock(hdev);
-
-	return err;
+	return hci_bdaddr_list_add(&hdev->reject_list, bdaddr, BDADDR_BREDR);
 }
 
-static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_del(struct hci_dev *hdev, bdaddr_t *bdaddr)
 {
-	bdaddr_t bdaddr;
-	int err;
-
-	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
+	return hci_bdaddr_list_del(&hdev->reject_list, bdaddr, BDADDR_BREDR);
+}
 
-	err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+static int hci_get_conn_info(struct hci_dev *hdev, struct hci_conn_info_req *req,
+			     struct hci_conn_info *ci)
+{
+	struct hci_conn *conn;
+
+	conn = hci_conn_hash_lookup_ba(hdev, req->type, &req->bdaddr);
+	if (!conn)
+		return -ENOENT;
+	bacpy(&ci->bdaddr, &conn->dst);
+	ci->handle = conn->handle;
+	ci->type  = conn->type;
+	ci->out   = conn->out;
+	ci->state = conn->state;
+	ci->link_mode = hci_get_link_mode(conn);
+	return 0;
+}
 
-	hci_dev_unlock(hdev);
+static int hci_get_auth_info(struct hci_dev *hdev, struct hci_auth_info_req *req)
+{
+	struct hci_conn *conn;
 
-	return err;
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req->bdaddr);
+	if (!conn)
+		return -ENOENT;
+	req->type = conn->auth_type;
+	return 0;
 }
 
 /* Ioctls that require bound socket */
-static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
-				unsigned long arg)
+static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 {
-	struct hci_dev *hdev = hci_pi(sk)->hdev;
+	struct hci_dev *hdev;
+	union {
+		bdaddr_t bdaddr;
+		struct hci_conn_info_req conn_req;
+		struct hci_auth_info_req auth_req;
+	} u;
+	struct hci_conn_info ci;
+	int err = 0;
 
-	if (!hdev)
-		return -EBADFD;
+	if (cmd == HCIBLOCKADDR || cmd == HCIUNBLOCKADDR) {
+		if (copy_from_user(&u.bdaddr, arg, sizeof(u.bdaddr)))
+			err = -EFAULT;
+	} else if (cmd == HCIGETCONNINFO) {
+		if (copy_from_user(&u.conn_req, arg, sizeof(u.conn_req)))
+			err = -EFAULT;
+	} else if (cmd == HCIGETAUTHINFO) {
+		if (copy_from_user(&u.auth_req, arg, sizeof(u.auth_req)))
+			err = -EFAULT;
+	}
 
-	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
-		return -EBUSY;
+	lock_sock(sk);
 
-	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
-		return -EOPNOTSUPP;
+	hdev = hci_pi(sk)->hdev;
+	if (!hdev) {
+		err = -EBADFD;
+		goto out;
+	}
 
-	if (hdev->dev_type != HCI_PRIMARY)
-		return -EOPNOTSUPP;
+	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+		err = -EBUSY;
+		goto out;
+	}
 
+	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (hdev->dev_type != HCI_PRIMARY) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	hci_dev_lock(hdev);
 	switch (cmd) {
 	case HCISETRAW:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return -EOPNOTSUPP;
-
+			err = -EPERM;
+		else
+			err = -EOPNOTSUPP;
+		break;
 	case HCIGETCONNINFO:
-		return hci_get_conn_info(hdev, (void __user *)arg);
-
+		if (!err)
+			err = hci_get_conn_info(hdev, &u.conn_req, &ci);
+		break;
 	case HCIGETAUTHINFO:
-		return hci_get_auth_info(hdev, (void __user *)arg);
-
+		if (!err)
+			err = hci_get_auth_info(hdev, &u.auth_req);
+		break;
 	case HCIBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_reject_list_add(hdev, (void __user *)arg);
-
+			err = -EPERM;
+		else if (!err)
+			err = hci_sock_reject_list_add(hdev, &u.bdaddr);
+		break;
 	case HCIUNBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_reject_list_del(hdev, (void __user *)arg);
+			err = -EPERM;
+		else if (!err)
+			err = hci_sock_reject_list_del(hdev, &u.bdaddr);
+		break;
+	default:
+		err = -ENOIOCTLCMD;
 	}
+	hci_dev_unlock(hdev);
+
+ out:
+	release_sock(sk);
 
-	return -ENOIOCTLCMD;
+	if (!err) {
+		if (cmd == HCIGETCONNINFO) {
+			if (copy_to_user(arg + sizeof(u.conn_req), &ci, sizeof(ci)))
+				err = -EFAULT;
+		} else if (cmd == HCIGETAUTHINFO) {
+			if (copy_to_user(arg, &u.auth_req, sizeof(u.auth_req)))
+				err = -EFAULT;
+		}
+	}
+	return err;
 }
 
 static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
@@ -975,15 +1029,14 @@  static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
 {
 	void __user *argp = (void __user *)arg;
 	struct sock *sk = sock->sk;
-	int err;
 
 	BT_DBG("cmd %x arg %lx", cmd, arg);
 
 	lock_sock(sk);
 
 	if (hci_pi(sk)->channel != HCI_CHANNEL_RAW) {
-		err = -EBADFD;
-		goto done;
+		release_sock(sk);
+		return -EBADFD;
 	}
 
 	/* When calling an ioctl on an unbound raw socket, then ensure
@@ -1055,13 +1108,7 @@  static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
 		return hci_inquiry(argp);
 	}
 
-	lock_sock(sk);
-
-	err = hci_sock_bound_ioctl(sk, cmd, arg);
-
-done:
-	release_sock(sk);
-	return err;
+	return hci_sock_bound_ioctl(sk, cmd, (void __user *)arg);
 }
 
 #ifdef CONFIG_COMPAT