diff mbox series

[2/2] Revert "Bluetooth: RFCOMM: Replace use of memcpy_from_msg with bt_skb_sendmmsg"

Message ID 20220208221911.57058-2-pmenzel@molgen.mpg.de
State New
Headers show
Series [1/2] Revert "Bluetooth: Fix passing NULL to PTR_ERR" | expand

Commit Message

Paul Menzel Feb. 8, 2022, 10:19 p.m. UTC 
This reverts commit 81be03e026dc0c16dc1c64e088b2a53b73caa895.

Since the commit, transferring files greater than some bytes to the
Nokia N9 (MeeGo) or Jolla (Sailfish OS) is not possible anymore.

    # obexctl
    [NEW] Client /org/bluez/obex
    [obex]# connect 40:98:4E:5B:CE:XX
    Attempting to connect to 40:98:4E:5B:CE:XX
    [NEW] Session /org/bluez/obex/client/session0 [default]
    [NEW] ObjectPush /org/bluez/obex/client/session0
    Connection successful
    [40:98:4E:5B:CE:XX]# send /lib/systemd/systemd
    Attempting to send /lib/systemd/systemd to /org/bluez/obex/client/session0
    [NEW] Transfer /org/bluez/obex/client/session0/transfer0
    Transfer /org/bluez/obex/client/session0/transfer0
        Status: queued
        Name: systemd
        Size: 1841712
        Filename: /lib/systemd/systemd
        Session: /org/bluez/obex/client/session0
    [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Status: active
    [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Transferred: 32737 (@32KB/s 00:55)
    [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Status: error
    [DEL] Transfer /org/bluez/obex/client/session0/transfer0

Reverting it, fixes the regression.

Link: https://lore.kernel.org/linux-bluetooth/aa3ee7ac-6c52-3861-1798-3cc1a37f6ebf@molgen.mpg.de/T/#m1f9673e4ab0d55a7dccf87905337ab2e67d689f1
Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 net/bluetooth/rfcomm/core.c | 50 ++++++-------------------------------
 net/bluetooth/rfcomm/sock.c | 46 ++++++++++++++++++++++++++--------
 2 files changed, 43 insertions(+), 53 deletions(-)

Comments

Luiz Augusto von Dentz Feb. 8, 2022, 11:39 p.m. UTC | #1 
Hi Paul,

On Tue, Feb 8, 2022 at 2:20 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> This reverts commit 81be03e026dc0c16dc1c64e088b2a53b73caa895.
>
> Since the commit, transferring files greater than some bytes to the
> Nokia N9 (MeeGo) or Jolla (Sailfish OS) is not possible anymore.
>
>     # obexctl
>     [NEW] Client /org/bluez/obex
>     [obex]# connect 40:98:4E:5B:CE:XX
>     Attempting to connect to 40:98:4E:5B:CE:XX
>     [NEW] Session /org/bluez/obex/client/session0 [default]
>     [NEW] ObjectPush /org/bluez/obex/client/session0
>     Connection successful
>     [40:98:4E:5B:CE:XX]# send /lib/systemd/systemd
>     Attempting to send /lib/systemd/systemd to /org/bluez/obex/client/session0
>     [NEW] Transfer /org/bluez/obex/client/session0/transfer0
>     Transfer /org/bluez/obex/client/session0/transfer0
>         Status: queued
>         Name: systemd
>         Size: 1841712
>         Filename: /lib/systemd/systemd
>         Session: /org/bluez/obex/client/session0
>     [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Status: active
>     [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Transferred: 32737 (@32KB/s 00:55)
>     [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Status: error
>     [DEL] Transfer /org/bluez/obex/client/session0/transfer0
>
> Reverting it, fixes the regression.
>
> Link: https://lore.kernel.org/linux-bluetooth/aa3ee7ac-6c52-3861-1798-3cc1a37f6ebf@molgen.mpg.de/T/#m1f9673e4ab0d55a7dccf87905337ab2e67d689f1
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>

We would be much better off with the explanation on why it is causing
a regression on these, is there an error? On top of that we can avoid
such regressions by introducing a test to rfcomm-tester to transfer
big PDUs.

> ---
>  net/bluetooth/rfcomm/core.c | 50 ++++++-------------------------------
>  net/bluetooth/rfcomm/sock.c | 46 ++++++++++++++++++++++++++--------
>  2 files changed, 43 insertions(+), 53 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 7324764384b6..f2bacb464ccf 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -549,58 +549,22 @@ struct rfcomm_dlc *rfcomm_dlc_exists(bdaddr_t *src, bdaddr_t *dst, u8 channel)
>         return dlc;
>  }
>
> -static int rfcomm_dlc_send_frag(struct rfcomm_dlc *d, struct sk_buff *frag)
> -{
> -       int len = frag->len;
> -
> -       BT_DBG("dlc %p mtu %d len %d", d, d->mtu, len);
> -
> -       if (len > d->mtu)
> -               return -EINVAL;
> -
> -       rfcomm_make_uih(frag, d->addr);
> -       __skb_queue_tail(&d->tx_queue, frag);
> -
> -       return len;
> -}
> -
>  int rfcomm_dlc_send(struct rfcomm_dlc *d, struct sk_buff *skb)
>  {
> -       unsigned long flags;
> -       struct sk_buff *frag, *next;
> -       int len;
> +       int len = skb->len;
>
>         if (d->state != BT_CONNECTED)
>                 return -ENOTCONN;
>
> -       frag = skb_shinfo(skb)->frag_list;
> -       skb_shinfo(skb)->frag_list = NULL;
> -
> -       /* Queue all fragments atomically. */
> -       spin_lock_irqsave(&d->tx_queue.lock, flags);
> -
> -       len = rfcomm_dlc_send_frag(d, skb);
> -       if (len < 0 || !frag)
> -               goto unlock;
> -
> -       for (; frag; frag = next) {
> -               int ret;
> -
> -               next = frag->next;
> -
> -               ret = rfcomm_dlc_send_frag(d, frag);
> -               if (ret < 0) {
> -                       kfree_skb(frag);
> -                       goto unlock;
> -               }
> +       BT_DBG("dlc %p mtu %d len %d", d, d->mtu, len);
>
> -               len += ret;
> -       }
> +       if (len > d->mtu)
> +               return -EINVAL;
>
> -unlock:
> -       spin_unlock_irqrestore(&d->tx_queue.lock, flags);
> +       rfcomm_make_uih(skb, d->addr);
> +       skb_queue_tail(&d->tx_queue, skb);
>
> -       if (len > 0 && !test_bit(RFCOMM_TX_THROTTLED, &d->flags))
> +       if (!test_bit(RFCOMM_TX_THROTTLED, &d->flags))
>                 rfcomm_schedule();
>         return len;
>  }
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 5938af3e9936..2c95bb58f901 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -575,20 +575,46 @@ static int rfcomm_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>         lock_sock(sk);
>
>         sent = bt_sock_wait_ready(sk, msg->msg_flags);
> +       if (sent)
> +               goto done;
>
> -       release_sock(sk);
> +       while (len) {
> +               size_t size = min_t(size_t, len, d->mtu);
> +               int err;
>
> -       if (sent)
> -               return sent;
> +               skb = sock_alloc_send_skb(sk, size + RFCOMM_SKB_RESERVE,
> +                               msg->msg_flags & MSG_DONTWAIT, &err);
> +               if (!skb) {
> +                       if (sent == 0)
> +                               sent = err;
> +                       break;
> +               }
> +               skb_reserve(skb, RFCOMM_SKB_HEAD_RESERVE);
> +
> +               err = memcpy_from_msg(skb_put(skb, size), msg, size);
> +               if (err) {
> +                       kfree_skb(skb);
> +                       if (sent == 0)
> +                               sent = err;
> +                       break;
> +               }
> +
> +               skb->priority = sk->sk_priority;
> +
> +               err = rfcomm_dlc_send(d, skb);
> +               if (err < 0) {
> +                       kfree_skb(skb);
> +                       if (sent == 0)
> +                               sent = err;
> +                       break;
> +               }
>
> -       skb = bt_skb_sendmmsg(sk, msg, len, d->mtu, RFCOMM_SKB_HEAD_RESERVE,
> -                             RFCOMM_SKB_TAIL_RESERVE);
> -       if (IS_ERR_OR_NULL(skb))
> -               return PTR_ERR(skb);
> +               sent += size;
> +               len  -= size;
> +       }
>
> -       sent = rfcomm_dlc_send(d, skb);
> -       if (sent < 0)
> -               kfree_skb(skb);
> +done:
> +       release_sock(sk);
>
>         return sent;
>  }
> --
> 2.34.1
>
Luiz Augusto von Dentz Feb. 9, 2022, 1:06 a.m. UTC | #2 
Hi Paul,

On Tue, Feb 8, 2022 at 2:20 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> This reverts commit 81be03e026dc0c16dc1c64e088b2a53b73caa895.
>
> Since the commit, transferring files greater than some bytes to the
> Nokia N9 (MeeGo) or Jolla (Sailfish OS) is not possible anymore.
>
>     # obexctl
>     [NEW] Client /org/bluez/obex
>     [obex]# connect 40:98:4E:5B:CE:XX
>     Attempting to connect to 40:98:4E:5B:CE:XX
>     [NEW] Session /org/bluez/obex/client/session0 [default]
>     [NEW] ObjectPush /org/bluez/obex/client/session0
>     Connection successful
>     [40:98:4E:5B:CE:XX]# send /lib/systemd/systemd
>     Attempting to send /lib/systemd/systemd to /org/bluez/obex/client/session0
>     [NEW] Transfer /org/bluez/obex/client/session0/transfer0
>     Transfer /org/bluez/obex/client/session0/transfer0
>         Status: queued
>         Name: systemd
>         Size: 1841712
>         Filename: /lib/systemd/systemd
>         Session: /org/bluez/obex/client/session0
>     [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Status: active
>     [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Transferred: 32737 (@32KB/s 00:55)
>     [CHG] Transfer /org/bluez/obex/client/session0/transfer0 Status: error
>     [DEL] Transfer /org/bluez/obex/client/session0/transfer0

Would you please create a github issue
(https://github.com/bluez/bluez/issues/) and attach the btmon trace so
we can check what is the error, you might as well attach the obexd
logs.

> Reverting it, fixes the regression.
>
> Link: https://lore.kernel.org/linux-bluetooth/aa3ee7ac-6c52-3861-1798-3cc1a37f6ebf@molgen.mpg.de/T/#m1f9673e4ab0d55a7dccf87905337ab2e67d689f1
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  net/bluetooth/rfcomm/core.c | 50 ++++++-------------------------------
>  net/bluetooth/rfcomm/sock.c | 46 ++++++++++++++++++++++++++--------
>  2 files changed, 43 insertions(+), 53 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 7324764384b6..f2bacb464ccf 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -549,58 +549,22 @@ struct rfcomm_dlc *rfcomm_dlc_exists(bdaddr_t *src, bdaddr_t *dst, u8 channel)
>         return dlc;
>  }
>
> -static int rfcomm_dlc_send_frag(struct rfcomm_dlc *d, struct sk_buff *frag)
> -{
> -       int len = frag->len;
> -
> -       BT_DBG("dlc %p mtu %d len %d", d, d->mtu, len);
> -
> -       if (len > d->mtu)
> -               return -EINVAL;
> -
> -       rfcomm_make_uih(frag, d->addr);
> -       __skb_queue_tail(&d->tx_queue, frag);
> -
> -       return len;
> -}
> -
>  int rfcomm_dlc_send(struct rfcomm_dlc *d, struct sk_buff *skb)
>  {
> -       unsigned long flags;
> -       struct sk_buff *frag, *next;
> -       int len;
> +       int len = skb->len;
>
>         if (d->state != BT_CONNECTED)
>                 return -ENOTCONN;
>
> -       frag = skb_shinfo(skb)->frag_list;
> -       skb_shinfo(skb)->frag_list = NULL;
> -
> -       /* Queue all fragments atomically. */
> -       spin_lock_irqsave(&d->tx_queue.lock, flags);
> -
> -       len = rfcomm_dlc_send_frag(d, skb);
> -       if (len < 0 || !frag)
> -               goto unlock;
> -
> -       for (; frag; frag = next) {
> -               int ret;
> -
> -               next = frag->next;
> -
> -               ret = rfcomm_dlc_send_frag(d, frag);
> -               if (ret < 0) {
> -                       kfree_skb(frag);
> -                       goto unlock;
> -               }
> +       BT_DBG("dlc %p mtu %d len %d", d, d->mtu, len);
>
> -               len += ret;
> -       }
> +       if (len > d->mtu)
> +               return -EINVAL;
>
> -unlock:
> -       spin_unlock_irqrestore(&d->tx_queue.lock, flags);
> +       rfcomm_make_uih(skb, d->addr);
> +       skb_queue_tail(&d->tx_queue, skb);
>
> -       if (len > 0 && !test_bit(RFCOMM_TX_THROTTLED, &d->flags))
> +       if (!test_bit(RFCOMM_TX_THROTTLED, &d->flags))
>                 rfcomm_schedule();
>         return len;
>  }
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 5938af3e9936..2c95bb58f901 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -575,20 +575,46 @@ static int rfcomm_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>         lock_sock(sk);
>
>         sent = bt_sock_wait_ready(sk, msg->msg_flags);
> +       if (sent)
> +               goto done;
>
> -       release_sock(sk);
> +       while (len) {
> +               size_t size = min_t(size_t, len, d->mtu);
> +               int err;
>
> -       if (sent)
> -               return sent;
> +               skb = sock_alloc_send_skb(sk, size + RFCOMM_SKB_RESERVE,
> +                               msg->msg_flags & MSG_DONTWAIT, &err);
> +               if (!skb) {
> +                       if (sent == 0)
> +                               sent = err;
> +                       break;
> +               }
> +               skb_reserve(skb, RFCOMM_SKB_HEAD_RESERVE);
> +
> +               err = memcpy_from_msg(skb_put(skb, size), msg, size);
> +               if (err) {
> +                       kfree_skb(skb);
> +                       if (sent == 0)
> +                               sent = err;
> +                       break;
> +               }
> +
> +               skb->priority = sk->sk_priority;
> +
> +               err = rfcomm_dlc_send(d, skb);
> +               if (err < 0) {
> +                       kfree_skb(skb);
> +                       if (sent == 0)
> +                               sent = err;
> +                       break;
> +               }
>
> -       skb = bt_skb_sendmmsg(sk, msg, len, d->mtu, RFCOMM_SKB_HEAD_RESERVE,
> -                             RFCOMM_SKB_TAIL_RESERVE);
> -       if (IS_ERR_OR_NULL(skb))
> -               return PTR_ERR(skb);
> +               sent += size;
> +               len  -= size;
> +       }
>
> -       sent = rfcomm_dlc_send(d, skb);
> -       if (sent < 0)
> -               kfree_skb(skb);
> +done:
> +       release_sock(sk);
>
>         return sent;
>  }
> --
> 2.34.1
>
diff mbox series

Patch

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 7324764384b6..f2bacb464ccf 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -549,58 +549,22 @@  struct rfcomm_dlc *rfcomm_dlc_exists(bdaddr_t *src, bdaddr_t *dst, u8 channel)
 	return dlc;
 }
 
-static int rfcomm_dlc_send_frag(struct rfcomm_dlc *d, struct sk_buff *frag)
-{
-	int len = frag->len;
-
-	BT_DBG("dlc %p mtu %d len %d", d, d->mtu, len);
-
-	if (len > d->mtu)
-		return -EINVAL;
-
-	rfcomm_make_uih(frag, d->addr);
-	__skb_queue_tail(&d->tx_queue, frag);
-
-	return len;
-}
-
 int rfcomm_dlc_send(struct rfcomm_dlc *d, struct sk_buff *skb)
 {
-	unsigned long flags;
-	struct sk_buff *frag, *next;
-	int len;
+	int len = skb->len;
 
 	if (d->state != BT_CONNECTED)
 		return -ENOTCONN;
 
-	frag = skb_shinfo(skb)->frag_list;
-	skb_shinfo(skb)->frag_list = NULL;
-
-	/* Queue all fragments atomically. */
-	spin_lock_irqsave(&d->tx_queue.lock, flags);
-
-	len = rfcomm_dlc_send_frag(d, skb);
-	if (len < 0 || !frag)
-		goto unlock;
-
-	for (; frag; frag = next) {
-		int ret;
-
-		next = frag->next;
-
-		ret = rfcomm_dlc_send_frag(d, frag);
-		if (ret < 0) {
-			kfree_skb(frag);
-			goto unlock;
-		}
+	BT_DBG("dlc %p mtu %d len %d", d, d->mtu, len);
 
-		len += ret;
-	}
+	if (len > d->mtu)
+		return -EINVAL;
 
-unlock:
-	spin_unlock_irqrestore(&d->tx_queue.lock, flags);
+	rfcomm_make_uih(skb, d->addr);
+	skb_queue_tail(&d->tx_queue, skb);
 
-	if (len > 0 && !test_bit(RFCOMM_TX_THROTTLED, &d->flags))
+	if (!test_bit(RFCOMM_TX_THROTTLED, &d->flags))
 		rfcomm_schedule();
 	return len;
 }
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 5938af3e9936..2c95bb58f901 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -575,20 +575,46 @@  static int rfcomm_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	lock_sock(sk);
 
 	sent = bt_sock_wait_ready(sk, msg->msg_flags);
+	if (sent)
+		goto done;
 
-	release_sock(sk);
+	while (len) {
+		size_t size = min_t(size_t, len, d->mtu);
+		int err;
 
-	if (sent)
-		return sent;
+		skb = sock_alloc_send_skb(sk, size + RFCOMM_SKB_RESERVE,
+				msg->msg_flags & MSG_DONTWAIT, &err);
+		if (!skb) {
+			if (sent == 0)
+				sent = err;
+			break;
+		}
+		skb_reserve(skb, RFCOMM_SKB_HEAD_RESERVE);
+
+		err = memcpy_from_msg(skb_put(skb, size), msg, size);
+		if (err) {
+			kfree_skb(skb);
+			if (sent == 0)
+				sent = err;
+			break;
+		}
+
+		skb->priority = sk->sk_priority;
+
+		err = rfcomm_dlc_send(d, skb);
+		if (err < 0) {
+			kfree_skb(skb);
+			if (sent == 0)
+				sent = err;
+			break;
+		}
 
-	skb = bt_skb_sendmmsg(sk, msg, len, d->mtu, RFCOMM_SKB_HEAD_RESERVE,
-			      RFCOMM_SKB_TAIL_RESERVE);
-	if (IS_ERR_OR_NULL(skb))
-		return PTR_ERR(skb);
+		sent += size;
+		len  -= size;
+	}
 
-	sent = rfcomm_dlc_send(d, skb);
-	if (sent < 0)
-		kfree_skb(skb);
+done:
+	release_sock(sk);
 
 	return sent;
 }