Received-SPF: pass (google.com: domain of linux-kernel+bounces-93083-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240305225652.22872-1-quic_mdtipton@quicinc.com>
In-Reply-To: <20240305225652.22872-1-quic_mdtipton@quicinc.com>
From: Rob Clark <robdclark@chromium.org>
Date: Tue, 5 Mar 2024 15:13:46 -0800
Message-ID: <CAJs_Fx4DKiBjzh0-F6aBFdsqGDiE9uppXAE84yrO3hxMSzwgjA@mail.gmail.com>
Subject: Re: [PATCH] interconnect: Don't access req_list while it's being manipulated
To: Mike Tipton <quic_mdtipton@quicinc.com>
Cc: djakov@kernel.org, quic_rlaggysh@quicinc.com, quic_okukatla@quicinc.com, 
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 5, 2024 at 2:57=E2=80=AFPM Mike Tipton <quic_mdtipton@quicinc.c=
om> wrote:
>
> 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 =3D invalid pointer>
>
> Example B:
>
>   CPU0                               CPU1
>   ----                               ----
>   icc_set_bw(path_a)
>     mutex_lock(&icc_bw_lock);
>                                      path_b =3D 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 =3D 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 af42269c3523 ("interconnect: Fix locking for runpm vs reclaim"=
)
>
> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> Fixes: af42269c3523 ("interconnect: Fix locking for runpm vs reclaim")

Looks good from a memory/lockdep standpoint,

Reviewed-by: Rob Clark <robdclark@chromium.org>

> ---
>  drivers/interconnect/core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 5d1010cafed8..7e9b996b47c8 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -176,6 +176,8 @@ static struct icc_path *path_init(struct device *dev,=
 struct icc_node *dst,
>
>         path->num_nodes =3D num_nodes;
>
> +       mutex_lock(&icc_bw_lock);
> +
>         for (i =3D num_nodes - 1; i >=3D 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 =3D 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 =3D 0; i < path->num_nodes; i++) {
>                 node =3D 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);
> --
> 2.17.1
>