thunderbolt: Couple of fixes for reported issues

Hi all,

This series contains fixes for issues we have noticed during internal validation.

Mika Westerberg (3):
thunderbolt: Fix KASAN reported use-after-free when request is canceled
thunderbolt: Use separate lock class for each ring
Revert "thunderbolt: xdomain: Notify peers after enumeration"

 drivers/thunderbolt/ctl.c     | 63 +++++++++++++++++------------------
 drivers/thunderbolt/nhi.c     |  5 +++
 drivers/thunderbolt/xdomain.c |  1 -
 include/linux/thunderbolt.h   |  3 ++
 4 files changed, 39 insertions(+), 33 deletions(-)

2.50.1

Alan reported that when doing stress testing sometimes KASAN notices use-after-free during control channel operation (stripped down keeping the relevant parts):

 BUG: KASAN: slab-use-after-free in tb_cfg_request_sync+0x240/0x250 [thunderbolt]
 Read of size 24 at addr ffff88811067f290 by task kworker/u40:2/1760
  <TASK>
  tb_cfg_request_sync+0x240/0x250 [thunderbolt]
  tb_cfg_read_raw+0x367/0x510 [thunderbolt]
  tb_cfg_read+0xec/0x240 [thunderbolt]
  tb_port_get_link_generation+0x258/0x420 [thunderbolt]
  tb_usb3_consumed_bandwidth+0x1c1/0x2c0 [thunderbolt]
  tb_tunnel_consumed_bandwidth+0xfd/0x910 [thunderbolt]
  tb_available_bandwidth+0x5f2/0xeb0 [thunderbolt]
  tb_recalc_estimated_bandwidth+0x2a0/0x1bc0 [thunderbolt]
  tb_handle_dp_bandwidth_request+0x1897/0x5e20 [thunderbolt]
  process_one_work+0x675/0x1230
  worker_thread+0x5e6/0xf70
  kthread+0x365/0x470
  ret_from_fork+0x54d/0x710
  ret_from_fork_asm+0x1a/0x30
  </TASK>

 Allocated by task 1760:
  __kmalloc_cache_noprof+0x1ee/0x550
  tb_cfg_read_raw+0x1d3/0x510 [thunderbolt]
  tb_cfg_read+0xec/0x240 [thunderbolt]
  tb_port_get_link_generation+0x258/0x420 [thunderbolt]
  tb_usb3_consumed_bandwidth+0x1c1/0x2c0 [thunderbolt]
  tb_tunnel_consumed_bandwidth+0xfd/0x910 [thunderbolt]
  tb_available_bandwidth+0x5f2/0xeb0 [thunderbolt]
  tb_recalc_estimated_bandwidth+0x2a0/0x1bc0 [thunderbolt]
  tb_handle_dp_bandwidth_request+0x1897/0x5e20 [thunderbolt]
  process_one_work+0x675/0x1230
  worker_thread+0x5e6/0xf70
  kthread+0x365/0x470
  ret_from_fork+0x54d/0x710
  ret_from_fork_asm+0x1a/0x30

 Freed by task 926:
  kfree+0x18f/0x4a0
  tb_cfg_request_put+0xb7/0xe0 [thunderbolt]
  tb_cfg_request_work+0x82/0x120 [thunderbolt]
  process_one_work+0x675/0x1230
  worker_thread+0x5e6/0xf70
  kthread+0x365/0x470
  ret_from_fork+0x54d/0x710
  ret_from_fork_asm+0x1a/0x30

 Second to last potentially related work creation:
  __queue_work+0x575/0xd00
  queue_work_on+0x77/0x80
  tb_cfg_request_cancel+0xc7/0x260 [thunderbolt]
  tb_cfg_request_sync+0x1f6/0x250 [thunderbolt]
  tb_cfg_read_raw+0x367/0x510 [thunderbolt]
  tb_cfg_read+0xec/0x240 [thunderbolt]
  tb_port_get_link_generation+0x258/0x420 [thunderbolt]
  tb_usb3_consumed_bandwidth+0x1c1/0x2c0 [thunderbolt]
  tb_tunnel_consumed_bandwidth+0xfd/0x910 [thunderbolt]
  tb_available_bandwidth+0x5f2/0xeb0 [thunderbolt]
  tb_recalc_estimated_bandwidth+0x2a0/0x1bc0 [thunderbolt]
  tb_handle_dp_bandwidth_request+0x1897/0x5e20 [thunderbolt]
  process_one_work+0x675/0x1230
  worker_thread+0x5e6/0xf70
  kthread+0x365/0x470
  ret_from_fork+0x54d/0x710
  ret_from_fork_asm+0x1a/0x30

The last stack trace is helpful because it shows that we are cancelling a request and looking at tb_cfg_request_cancel() what might happen is that tb_cfg_request_work() completes right before tb_cfg_request_cancel() starts and because of this it will call schedule_work() queueing the same work to run again. However, it is already removed from the request_queue and reference count is dropped so when tb_cfg_request_work() triggers again it will access memory that is already released.

Fix this so that we first make sure a cancelled request is not handed away from tb_cfg_request_find() or scheduled to run. Then instead of relying on the worker to clean up the request we will do it in tb_cfg_request_cancel() after the work is canceled from running. Make tb_cfg_request_dequeue() release the request only if it was actually removed from the queue.

---
 drivers/thunderbolt/ctl.c | 63 +++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 32 deletions(-)

index cd47b627f97b..965988b18608 100644
@@ -73,7 +73,6 @@ struct tb_ctl {
 #define tb_ctl_dbg_once(ctl, format, arg...) \
 	dev_dbg_once((ctl)->nhi->dev, format, ## arg)
 
-static DECLARE_WAIT_QUEUE_HEAD(tb_cfg_request_cancel_queue);
 /* Serializes access to request kref_get/put */
 static DEFINE_MUTEX(tb_cfg_request_lock);
 

@@ -133,41 +132,42 @@ void tb_cfg_request_put(struct tb_cfg_request *req)
 static int tb_cfg_request_enqueue(struct tb_ctl *ctl,
 				  struct tb_cfg_request *req)
 {
+	tb_cfg_request_get(req);
+
 	WARN_ON(test_bit(TB_CFG_REQUEST_ACTIVE, &req->flags));
 	WARN_ON(req->ctl);
 
-	mutex_lock(&ctl->request_queue_lock);
+	guard(mutex)(&ctl->request_queue_lock);
 	if (!ctl->running) {
-		mutex_unlock(&ctl->request_queue_lock);
+		tb_cfg_request_put(req);
 		return -ENOTCONN;
 	}
 	req->ctl = ctl;
 	list_add_tail(&req->list, &ctl->request_queue);
 	set_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
-	mutex_unlock(&ctl->request_queue_lock);
 	return 0;
 }
 
-static void tb_cfg_request_dequeue(struct tb_cfg_request *req)
+static bool tb_cfg_request_is_active(struct tb_cfg_request *req)
 {
-	struct tb_ctl *ctl = req->ctl;
-
-	mutex_lock(&ctl->request_queue_lock);
-	if (!test_bit(TB_CFG_REQUEST_ACTIVE, &req->flags)) {
-		mutex_unlock(&ctl->request_queue_lock);
-		return;
-	}
+	return test_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
+}
 
-	list_del(&req->list);
-	clear_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
-	if (test_bit(TB_CFG_REQUEST_CANCELED, &req->flags))
-		wake_up(&tb_cfg_request_cancel_queue);
-	mutex_unlock(&ctl->request_queue_lock);
+static bool tb_cfg_request_is_canceled(struct tb_cfg_request *req)
+{
+	return test_bit(TB_CFG_REQUEST_CANCELED, &req->flags);
 }
 
-static bool tb_cfg_request_is_active(struct tb_cfg_request *req)
+static void tb_cfg_request_dequeue(struct tb_cfg_request *req)
 {
-	return test_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
+	struct tb_ctl *ctl = req->ctl;
+
+	guard(mutex)(&ctl->request_queue_lock);
+	if (tb_cfg_request_is_active(req)) {
+		list_del(&req->list);
+		clear_bit(TB_CFG_REQUEST_ACTIVE, &req->flags);
+		tb_cfg_request_put(req);
+	}
 }
 
 static struct tb_cfg_request *

@@ -178,7 +178,7 @@ tb_cfg_request_find(struct tb_ctl *ctl, struct ctl_pkg *pkg)
 	mutex_lock(&pkg->ctl->request_queue_lock);
 	list_for_each_entry(iter, &pkg->ctl->request_queue, list) {
 		tb_cfg_request_get(iter);
-		if (iter->match(iter, pkg)) {
+		if (!tb_cfg_request_is_canceled(iter) && iter->match(iter, pkg)) {
 			req = iter;
 			break;
 		}

@@ -512,8 +512,11 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 	trace_tb_rx(pkg->ctl->index, frame->eof, pkg->buffer, frame->size, !req);
 
 	if (req) {
-		if (req->copy(req, pkg))
-			schedule_work(&req->work);
+		scoped_guard(mutex, &pkg->ctl->request_queue_lock) {
+			if (!tb_cfg_request_is_canceled(req) &&
+			    req->copy(req, pkg))
+				schedule_work(&req->work);
+		}
 		tb_cfg_request_put(req);
 	}
 

@@ -525,11 +528,10 @@ static void tb_cfg_request_work(struct work_struct *work)
 {
 	struct tb_cfg_request *req = container_of(work, typeof(*req), work);
 
-	if (!test_bit(TB_CFG_REQUEST_CANCELED, &req->flags))
+	if (!tb_cfg_request_is_canceled(req))
 		req->callback(req->callback_data);
 
 	tb_cfg_request_dequeue(req);
-	tb_cfg_request_put(req);
 }
 
 /**

@@ -555,10 +557,9 @@ int tb_cfg_request(struct tb_ctl *ctl, struct tb_cfg_request *req,
 	INIT_WORK(&req->work, tb_cfg_request_work);
 	INIT_LIST_HEAD(&req->list);
 
-	tb_cfg_request_get(req);
 	ret = tb_cfg_request_enqueue(ctl, req);
 	if (ret)
-		goto err_put;
+		return ret;
 
 	ret = tb_ctl_tx(ctl, req->request, req->request_size,
 			req->request_type);

@@ -572,9 +573,6 @@ int tb_cfg_request(struct tb_ctl *ctl, struct tb_cfg_request *req,
 
 err_dequeue:
 	tb_cfg_request_dequeue(req);
-err_put:
-	tb_cfg_request_put(req);
-
 	return ret;
 }
 

@@ -588,9 +586,10 @@ int tb_cfg_request(struct tb_ctl *ctl, struct tb_cfg_request *req,
  */
 void tb_cfg_request_cancel(struct tb_cfg_request *req, int err)
 {
-	set_bit(TB_CFG_REQUEST_CANCELED, &req->flags);
-	schedule_work(&req->work);
-	wait_event(tb_cfg_request_cancel_queue, !tb_cfg_request_is_active(req));
+	scoped_guard(mutex, &req->ctl->request_queue_lock)
+		set_bit(TB_CFG_REQUEST_CANCELED, &req->flags);
+	cancel_work_sync(&req->work);
+	tb_cfg_request_dequeue(req);
 	req->result.err = err;
 }
 

2.50.1

When connected to another host and then unplugging cable lockdep triggers following:

======================================================
WARNING: possible circular locking dependency detected
7.1.0-rc2+ #1775 Tainted: G U
------------------------------------------------------
kworker/u16:6/312 is trying to acquire lock:
ffff8881179c70a8 ((work_completion)(&ring->work)){+.+.}-{0:0}, at: __flush_work+0x3cf/0xd10

but task is already holding lock:
ffff8881a8b810b0 (&net->connection_lock){+.+.}-{4:4}, at: tbnet_tear_down+0x110/0x720 [thunderbolt_net]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&net->connection_lock){+.+.}-{4:4}:

        __mutex_lock+0x19a/0x2490
        mutex_lock_nested+0x1b/0x30
        tbnet_handle_packet+0x74c/0xd70 [thunderbolt_net]
        tb_xdomain_handle_request+0x37c/0x4b0 [thunderbolt]
        tb_domain_event_cb+0xc9/0x140 [thunderbolt]
        tb_ctl_handle_event+0xd6/0x2c0 [thunderbolt]
        tb_ctl_rx_callback+0x22c/0xa10 [thunderbolt]
        ring_work+0x715/0xcb0 [thunderbolt]
        process_one_work+0x902/0x1790
        worker_thread+0x5cd/0xfe0
        kthread+0x339/0x420
        ret_from_fork+0x79a/0x9d0
        ret_from_fork_asm+0x1a/0x30

-> #0 ((work_completion)(&ring->work)){+.+.}-{0:0}:

        __lock_acquire+0x1592/0x2640
        lock_acquire+0x1a3/0x300
        __flush_work+0x3e9/0xd10
        flush_work+0x21/0x30
        tb_ring_stop+0x240/0x840 [thunderbolt]
        tbnet_tear_down+0x2ff/0x720 [thunderbolt_net]
        tbnet_stop+0x47/0x1a0 [thunderbolt_net]
        __dev_close_many+0x19e/0x4e0
        netif_close_many+0x1e8/0x640
        unregister_netdevice_many_notify+0x6d3/0x22d0
        unregister_netdevice_queue+0x2b9/0x3a0
        unregister_netdev+0x1c/0x70
        tbnet_remove+0x52/0xb0 [thunderbolt_net]
        tb_service_remove+0x8a/0xe0 [thunderbolt]
        device_remove+0xc5/0x190
        device_release_driver_internal+0x3db/0x590
        device_release_driver+0x12/0x20
        bus_remove_device+0x2c1/0x580
        device_del+0x3d9/0x9f0
        device_unregister+0x17/0xc0
        unregister_service+0x46/0x60 [thunderbolt]
        device_for_each_child_reverse+0xfa/0x180
        tb_xdomain_unregister+0x57/0xe0 [thunderbolt]
        unregister_unplugged_xdomain+0x101/0x1a0 [thunderbolt]
        bus_for_each_dev+0x111/0x1a0
        tb_domain_unregister_unplugged_xdomains+0x98/0xe0 [thunderbolt]
        tb_handle_hotplug+0xc3/0x2bb0 [thunderbolt]
        process_one_work+0x902/0x1790
        worker_thread+0x5cd/0xfe0
        kthread+0x339/0x420
        ret_from_fork+0x79a/0x9d0
        ret_from_fork_asm+0x1a/0x30

other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(&net->connection_lock);
                                lock((work_completion)(&ring->work));
                                lock(&net->connection_lock);

lock((work_completion)(&ring->work));

This in fact is false positive because they involve unrelated rings (and unrelated work structures). In the first one it is ring 0 which is used for control traffic and in the second it is dealing with another ring used for the high-speed traffic.

Fix this by using separate lock class for each ring worker.

---
 drivers/thunderbolt/nhi.c   | 5 +++++
 include/linux/thunderbolt.h | 3 +++
 2 files changed, 8 insertions(+)

index 5809809f64d4..d4d1efa2afa0 100644
@@ -15,6 +15,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/iommu.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/property.h>

@@ -560,6 +561,8 @@ static struct tb_ring *tb_ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
 	INIT_LIST_HEAD(&ring->in_flight);
 	INIT_WORK(&ring->work, ring_work);
 	init_waitqueue_head(&ring->wait);
+	lockdep_register_key(&ring->lock_key);
+	lockdep_init_map(&ring->work.lockdep_map, "ring.work", &ring->lock_key, 0);
 
 	ring->nhi = nhi;
 	ring->hop = hop;

@@ -599,6 +602,7 @@ static struct tb_ring *tb_ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
 			  ring->size * sizeof(*ring->descriptors),
 			  ring->descriptors, ring->descriptors_dma);
 err_free_ring:
+	lockdep_unregister_key(&ring->lock_key);
 	kfree(ring);
 
 	return NULL;

@@ -848,6 +852,7 @@ void tb_ring_free(struct tb_ring *ring)
 	 * to finish before freeing the ring.
 	 */
 	flush_work(&ring->work);
+	lockdep_unregister_key(&ring->lock_key);
 	kfree(ring);
 }
 EXPORT_SYMBOL_GPL(tb_ring_free);

index d48623fda79b..b62dfa52b149 100644
@@ -22,6 +22,7 @@ struct device;
 #include <linux/device.h>
 #include <linux/idr.h>
 #include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/mutex.h>
 #include <linux/device-id/tb.h>
 #include <linux/pci.h>

@@ -565,6 +566,7 @@ struct tb_nhi {
  * @interval_nsec: Interval counter if interrupt throttling is to be
  *		   used with this ring (in ns)
  * @wait: Used to signal that the ring may be empty now
+ * @lock_key: Lock validator class key per-ring
  */
 struct tb_ring {
 	spinlock_t lock;

@@ -590,6 +592,7 @@ struct tb_ring {
 	void *poll_data;
 	unsigned int interval_nsec;
 	wait_queue_head_t wait;
+	struct lock_class_key lock_key;
 };
 
 /* Leave ring interrupt enabled on suspend */

2.50.1

This reverts commit e027dba038f0008df9bc9575f5c3e803e90636c6.

Alan noticed that this causes the peers send change properties to each other continuously.

---
 drivers/thunderbolt/xdomain.c | 1 -
 1 file changed, 1 deletion(-)

index c179bd751fe4..05442df0e99c 100644
@@ -1814,7 +1814,6 @@ static void tb_xdomain_state_work(struct work_struct *work)
 			tb_xdomain_failed(xd);
 		} else {
 			xd->state = XDOMAIN_STATE_ENUMERATED;
-			tb_xdomain_queue_properties_changed(xd);
 		}
 		break;
 

2.50.1

Applied to thunderbolt.git/fixes.