diff mbox series

[v6,1/4] Bluetooth: hci_sock: Add support for BT_{SND,RCV}BUF

Message ID 20210916201049.114392-1-luiz.dentz@gmail.com
State New
Headers show
Series [v6,1/4] Bluetooth: hci_sock: Add support for BT_{SND,RCV}BUF | expand

Commit Message

Luiz Augusto von Dentz Sept. 16, 2021, 8:10 p.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds support for BT_{SND,RCV}BUF so userspace can set MTU based on
the channel usage.

Fixes: https://github.com/bluez/bluez/issues/201

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_sock.c | 102 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 91 insertions(+), 11 deletions(-)

Comments

Marcel Holtmann Sept. 21, 2021, 8:46 a.m. UTC | #1 
Hi Luiz,

> This adds support for BT_{SND,RCV}BUF so userspace can set MTU based on

> the channel usage.

> 

> Fixes: https://github.com/bluez/bluez/issues/201

> 

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

> ---

> net/bluetooth/hci_sock.c | 102 ++++++++++++++++++++++++++++++++++-----

> 1 file changed, 91 insertions(+), 11 deletions(-)


so I applied patches 1, 3 and 4 to bluetooth-next tree.

The patch 2 needs a more details review when I have time since I remember there were cases where the SKB copy was really needed.

Regards

Marcel
Luiz Augusto von Dentz Sept. 21, 2021, 6:03 p.m. UTC | #2 
Hi Marcel,

On Tue, Sep 21, 2021 at 1:46 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>

> Hi Luiz,

>

> > This adds support for BT_{SND,RCV}BUF so userspace can set MTU based on

> > the channel usage.

> >

> > Fixes: https://github.com/bluez/bluez/issues/201

> >

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

> > ---

> > net/bluetooth/hci_sock.c | 102 ++++++++++++++++++++++++++++++++++-----

> > 1 file changed, 91 insertions(+), 11 deletions(-)

>

> so I applied patches 1, 3 and 4 to bluetooth-next tree.

>

> The patch 2 needs a more details review when I have time since I remember there were cases where the SKB copy was really needed.


Is that something not covered by CI testing? Note we still have a copy
done internally with create_monitor_ctrl_command.

> Regards

>

> Marcel

>



-- 
Luiz Augusto von Dentz
Marcel Holtmann Sept. 22, 2021, 2:19 p.m. UTC | #3 
Hi Luiz,

>>> This adds support for BT_{SND,RCV}BUF so userspace can set MTU based on

>>> the channel usage.

>>> 

>>> Fixes: https://github.com/bluez/bluez/issues/201

>>> 

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

>>> ---

>>> net/bluetooth/hci_sock.c | 102 ++++++++++++++++++++++++++++++++++-----

>>> 1 file changed, 91 insertions(+), 11 deletions(-)

>> 

>> so I applied patches 1, 3 and 4 to bluetooth-next tree.

>> 

>> The patch 2 needs a more details review when I have time since I remember there were cases where the SKB copy was really needed.

> 

> Is that something not covered by CI testing? Note we still have a copy

> done internally with create_monitor_ctrl_command.


there are cases where the SKB handed down is in hdev->send() and that one is owned by the driver to make whatever modifications to headroom it pleases. So if the stack needs to send it out via other sockets, it needs a copy.

We can optimize here, but need to be careful.

Regards

Marcel
Luiz Augusto von Dentz Sept. 23, 2021, 9:04 p.m. UTC | #4 
Hi Marcel,

On Wed, Sep 22, 2021 at 7:19 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>

> Hi Luiz,

>

> >>> This adds support for BT_{SND,RCV}BUF so userspace can set MTU based on

> >>> the channel usage.

> >>>

> >>> Fixes: https://github.com/bluez/bluez/issues/201

> >>>

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

> >>> ---

> >>> net/bluetooth/hci_sock.c | 102 ++++++++++++++++++++++++++++++++++-----

> >>> 1 file changed, 91 insertions(+), 11 deletions(-)

> >>

> >> so I applied patches 1, 3 and 4 to bluetooth-next tree.

> >>

> >> The patch 2 needs a more details review when I have time since I remember there were cases where the SKB copy was really needed.

> >

> > Is that something not covered by CI testing? Note we still have a copy

> > done internally with create_monitor_ctrl_command.

>

> there are cases where the SKB handed down is in hdev->send() and that one is owned by the driver to make whatever modifications to headroom it pleases. So if the stack needs to send it out via other sockets, it needs a copy.

>

> We can optimize here, but need to be careful.


I guess you are referring to instances of hci_send_to_channel, which
appears there only one instance that is changed in the diff bellow:

        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);
                }
        }

Ive only changed the variable name since the original code use skb
which is now used as the original data instead of buf (extra copy), so
nothing has changed in this regard, also running this with KSAN, etc,
doesn't seem to produce any traces nor there seems to be anything
wrong in btmon either.

Note with this we are eliminating the extra copy on buf:

msg -> skb (new) vs msg-> buf -> skb (old)

> Regards

>

> Marcel

>



-- 
Luiz Augusto von Dentz
diff mbox series

Patch

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 55b0d177375b..99de17922bda 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -57,6 +57,7 @@  struct hci_pinfo {
 	unsigned long     flags;
 	__u32             cookie;
 	char              comm[TASK_COMM_LEN];
+	__u16             mtu;
 };
 
 static struct hci_dev *hci_hdev_from_sock(struct sock *sk)
@@ -1374,6 +1375,10 @@  static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 		break;
 	}
 
+	/* Default MTU to HCI_MAX_FRAME_SIZE if not set */
+	if (!hci_pi(sk)->mtu)
+		hci_pi(sk)->mtu = HCI_MAX_FRAME_SIZE;
+
 	sk->sk_state = BT_BOUND;
 
 done:
@@ -1719,7 +1724,7 @@  static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (flags & ~(MSG_DONTWAIT | MSG_NOSIGNAL | MSG_ERRQUEUE | MSG_CMSG_COMPAT))
 		return -EINVAL;
 
-	if (len < 4 || len > HCI_MAX_FRAME_SIZE)
+	if (len < 4 || len > hci_pi(sk)->mtu)
 		return -EINVAL;
 
 	buf = kmalloc(len, GFP_KERNEL);
@@ -1849,8 +1854,8 @@  static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	goto done;
 }
 
-static int hci_sock_setsockopt(struct socket *sock, int level, int optname,
-			       sockptr_t optval, unsigned int len)
+static int hci_sock_setsockopt_old(struct socket *sock, int level, int optname,
+				   sockptr_t optval, unsigned int len)
 {
 	struct hci_ufilter uf = { .opcode = 0 };
 	struct sock *sk = sock->sk;
@@ -1858,9 +1863,6 @@  static int hci_sock_setsockopt(struct socket *sock, int level, int optname,
 
 	BT_DBG("sk %p, opt %d", sk, optname);
 
-	if (level != SOL_HCI)
-		return -ENOPROTOOPT;
-
 	lock_sock(sk);
 
 	if (hci_pi(sk)->channel != HCI_CHANNEL_RAW) {
@@ -1935,18 +1937,63 @@  static int hci_sock_setsockopt(struct socket *sock, int level, int optname,
 	return err;
 }
 
-static int hci_sock_getsockopt(struct socket *sock, int level, int optname,
-			       char __user *optval, int __user *optlen)
+static int hci_sock_setsockopt(struct socket *sock, int level, int optname,
+			       sockptr_t optval, unsigned int len)
 {
-	struct hci_ufilter uf;
 	struct sock *sk = sock->sk;
-	int len, opt, err = 0;
+	int err = 0, opt = 0;
 
 	BT_DBG("sk %p, opt %d", sk, optname);
 
-	if (level != SOL_HCI)
+	if (level == SOL_HCI)
+		return hci_sock_setsockopt_old(sock, level, optname, optval,
+					       len);
+
+	if (level != SOL_BLUETOOTH)
 		return -ENOPROTOOPT;
 
+	lock_sock(sk);
+
+	switch (optname) {
+	case BT_SNDMTU:
+	case BT_RCVMTU:
+		switch (hci_pi(sk)->channel) {
+		/* Don't allow changing MTU for channels that are meant for HCI
+		 * traffic only.
+		 */
+		case HCI_CHANNEL_RAW:
+		case HCI_CHANNEL_USER:
+			err = -ENOPROTOOPT;
+			goto done;
+		}
+
+		if (copy_from_sockptr(&opt, optval, sizeof(u16))) {
+			err = -EFAULT;
+			break;
+		}
+
+		hci_pi(sk)->mtu = opt;
+		break;
+
+	default:
+		err = -ENOPROTOOPT;
+		break;
+	}
+
+done:
+	release_sock(sk);
+	return err;
+}
+
+static int hci_sock_getsockopt_old(struct socket *sock, int level, int optname,
+				   char __user *optval, int __user *optlen)
+{
+	struct hci_ufilter uf;
+	struct sock *sk = sock->sk;
+	int len, opt, err = 0;
+
+	BT_DBG("sk %p, opt %d", sk, optname);
+
 	if (get_user(len, optlen))
 		return -EFAULT;
 
@@ -2004,6 +2051,39 @@  static int hci_sock_getsockopt(struct socket *sock, int level, int optname,
 	return err;
 }
 
+static int hci_sock_getsockopt(struct socket *sock, int level, int optname,
+			       char __user *optval, int __user *optlen)
+{
+	struct sock *sk = sock->sk;
+	int err = 0;
+
+	BT_DBG("sk %p, opt %d", sk, optname);
+
+	if (level == SOL_HCI)
+		return hci_sock_getsockopt_old(sock, level, optname, optval,
+					       optlen);
+
+	if (level != SOL_BLUETOOTH)
+		return -ENOPROTOOPT;
+
+	lock_sock(sk);
+
+	switch (optname) {
+	case BT_SNDMTU:
+	case BT_RCVMTU:
+		if (put_user(hci_pi(sk)->mtu, (u16 __user *)optval))
+			err = -EFAULT;
+		break;
+
+	default:
+		err = -ENOPROTOOPT;
+		break;
+	}
+
+	release_sock(sk);
+	return err;
+}
+
 static const struct proto_ops hci_sock_ops = {
 	.family		= PF_BLUETOOTH,
 	.owner		= THIS_MODULE,