[PATCH 1/2] Revert "Bluetooth: Fix passing NULL to PTR_ERR"

Subsystems: bluetooth subsystem, the rest

STALE1670d

4 messages, 2 authors, 2022-02-09 · open the first message on its own page

[PATCH 1/2] Revert "Bluetooth: Fix passing NULL to PTR_ERR"

From: Paul Menzel <hidden>
Date: 2022-02-08 22:24:57

This reverts commit 266191aa8d14b84958aaeb5e96ee4e97839e3d87, so that
commit 81be03e026dc ("Bluetooth: RFCOMM: Replace use of memcpy_from_msg
with bt_skb_sendmmsg"), introducing a regression, can be reverted.

Signed-off-by: Paul Menzel <redacted>
---
 include/net/bluetooth/bluetooth.h | 2 +-
 net/bluetooth/rfcomm/sock.c       | 2 +-
 net/bluetooth/sco.c               | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 4b3d0b16c185..318df161e6d3 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h

@@ -505,7 +505,7 @@ static inline struct sk_buff *bt_skb_sendmmsg(struct sock *sk,
 		struct sk_buff *tmp;
 
 		tmp = bt_skb_sendmsg(sk, msg, len, mtu, headroom, tailroom);
-		if (IS_ERR(tmp)) {
+		if (IS_ERR_OR_NULL(tmp)) {
 			kfree_skb(skb);
 			return tmp;
 		}

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 4bf4ea6cbb5e..5938af3e9936 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c

@@ -583,7 +583,7 @@ static int rfcomm_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	skb = bt_skb_sendmmsg(sk, msg, len, d->mtu, RFCOMM_SKB_HEAD_RESERVE,
 			      RFCOMM_SKB_TAIL_RESERVE);
-	if (IS_ERR(skb))
+	if (IS_ERR_OR_NULL(skb))
 		return PTR_ERR(skb);
 
 	sent = rfcomm_dlc_send(d, skb);

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 8eabf41b2993..7ebc9d294f0c 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c

@@ -734,7 +734,7 @@ static int sco_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		return -EOPNOTSUPP;
 
 	skb = bt_skb_sendmsg(sk, msg, len, len, 0, 0);
-	if (IS_ERR(skb))
+	if (IS_ERR_OR_NULL(skb))
 		return PTR_ERR(skb);
 
 	lock_sock(sk);

-- 
2.34.1

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

From: Paul Menzel <hidden>
Date: 2022-02-08 22:34:18

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

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

From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: 2022-02-08 23:39:41

Hi Paul,

On Tue, Feb 8, 2022 at 2:20 PM Paul Menzel [off-list ref] 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 <redacted>

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.

quoted hunk

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

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

From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: 2022-02-09 02:45:35

Hi Paul,

On Tue, Feb 8, 2022 at 2:20 PM Paul Menzel [off-list ref] 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.

quoted hunk

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help