Commit de1bf25b authored by Mike Tipton's avatar Mike Tipton Committed by Georgi Djakov
Browse files

interconnect: Don't access req_list while it's being manipulated 



The icc_lock mutex was split into separate icc_lock and icc_bw_lock
mutexes in [1] to avoid lockdep splats. However, this didn't adequately
protect access to icc_node::req_list.

The icc_set_bw() function will eventually iterate over req_list while
only holding icc_bw_lock, but req_list can be modified while only
holding icc_lock. This causes races between icc_set_bw(), of_icc_get(),
and icc_put().

Example A:

  CPU0                               CPU1
  ----                               ----
  icc_set_bw(path_a)
    mutex_lock(&icc_bw_lock);
                                     icc_put(path_b)
                                       mutex_lock(&icc_lock);
    aggregate_requests()
      hlist_for_each_entry(r, ...
                                       hlist_del(...
        <r = invalid pointer>

Example B:

  CPU0                               CPU1
  ----                               ----
  icc_set_bw(path_a)
    mutex_lock(&icc_bw_lock);
                                     path_b = of_icc_get()
                                       of_icc_get_by_index()
                                         mutex_lock(&icc_lock);
                                         path_find()
                                           path_init()
    aggregate_requests()
      hlist_for_each_entry(r, ...
                                             hlist_add_head(...
        <r = invalid pointer>

Fix this by ensuring icc_bw_lock is always held before manipulating
icc_node::req_list. The additional places icc_bw_lock is held don't
perform any memory allocations, so we should still be safe from the
original lockdep splats that motivated the separate locks.

[1] commit af42269c ("interconnect: Fix locking for runpm vs reclaim")

Signed-off-by: default avatarMike Tipton <quic_mdtipton@quicinc.com>
Fixes: af42269c ("interconnect: Fix locking for runpm vs reclaim")
Reviewed-by: default avatarRob Clark <robdclark@chromium.org>
Link: https://lore.kernel.org/r/20240305225652.22872-1-quic_mdtipton@quicinc.com


Signed-off-by: default avatarGeorgi Djakov <djakov@kernel.org>
parent 59097a2a

drivers/interconnect/core.c

+8 −0
Original line number Diff line number Diff line
@@ -176,6 +176,8 @@ static struct icc_path *path_init(struct device *dev, struct icc_node *dst, 


	path->num_nodes = num_nodes;



	mutex_lock(&icc_bw_lock);



	for (i = num_nodes - 1; i >= 0; i--) {

		node->provider->users++;

		hlist_add_head(&path->reqs[i].req_node, &node->req_list);
@@ -186,6 +188,8 @@ static struct icc_path *path_init(struct device *dev, struct icc_node *dst, 
		node = node->reverse;

	}



	mutex_unlock(&icc_bw_lock);



	return path;

}
@@ -792,12 +796,16 @@ void icc_put(struct icc_path *path) 
		pr_err("%s: error (%d)\n", __func__, ret);



	mutex_lock(&icc_lock);

	mutex_lock(&icc_bw_lock);



	for (i = 0; i < path->num_nodes; i++) {

		node = path->reqs[i].node;

		hlist_del(&path->reqs[i].req_node);

		if (!WARN_ON(!node->provider->users))

			node->provider->users--;

	}



	mutex_unlock(&icc_bw_lock);

	mutex_unlock(&icc_lock);



	kfree_const(path->name);