[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a69a750a-884b-c60b-9d52-f4ae2cd1b2ea@quicinc.com>
Date: Fri, 14 Jul 2023 11:18:58 +0530
From: Vignesh Viswanathan <quic_viswanat@...cinc.com>
To: Simon Horman <simon.horman@...igine.com>
CC: <mani@...nel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
<kuba@...nel.org>, <pabeni@...hat.com>,
<linux-arm-msm@...r.kernel.org>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <quic_srichara@...cinc.com>,
<quic_clew@...cinc.com>
Subject: Re: [PATCH net-next 1/3] net: qrtr: ns: Change servers radix tree to
xarray
On 7/13/2023 8:24 PM, Simon Horman wrote:
> On Wed, Jul 12, 2023 at 04:56:29PM +0530, Vignesh Viswanathan wrote:
>> There is a use after free scenario while iterating through the servers
>> radix tree despite the ns being a single threaded process. This can
>> happen when the radix tree APIs are not synchronized with the
>> rcu_read_lock() APIs.
>>
>> Convert the radix tree for servers to xarray to take advantage of the
>> built in rcu lock usage provided by xarray.
>>
>> Signed-off-by: Chris Lew <quic_clew@...cinc.com>
>> Signed-off-by: Vignesh Viswanathan <quic_viswanat@...cinc.com>
>
> Reviewed-by: Simon Horman <simon.horman@...igine.com>
>
> A few very minor nits below.
Thanks Simon for the review. Will fix both the comments in V2.
Thanks,
Vignesh
>
> ...
>
>> @@ -256,14 +240,17 @@ static struct qrtr_server *server_add(unsigned int service,
>> goto err;
>>
>> /* Delete the old server on the same port */
>> - old = radix_tree_lookup(&node->servers, port);
>> + old = xa_store(&node->servers, port, srv, GFP_KERNEL);
>> if (old) {
>> - radix_tree_delete(&node->servers, port);
>> - kfree(old);
>> + if (xa_is_err(old)) {
>> + pr_err("failed to add server [0x%x:0x%x] ret:%d\n",
>> + srv->service, srv->instance, xa_err(old));
>
> The indentation of the line above is not correct.
> It should be:
>
> pr_err("failed to add server [0x%x:0x%x] ret:%d\n",
> srv->service, srv->instance, xa_err(old));
>
>> + goto err;
>> + } else {
>> + kfree(old);
>> + }
>> }
>>
>> - radix_tree_insert(&node->servers, port, srv);
>> -
>> trace_qrtr_ns_server_add(srv->service, srv->instance,
>> srv->node, srv->port);
>>
>
> ...
>
>> @@ -576,13 +518,12 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from,
>> static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from,
>> unsigned int service, unsigned int instance)
>> {
>> - struct radix_tree_iter node_iter;
>> struct qrtr_server_filter filter;
>> - struct radix_tree_iter srv_iter;
>> struct qrtr_lookup *lookup;
>> struct qrtr_node *node;
>> - void __rcu **node_slot;
>> - void __rcu **srv_slot;
>> + struct qrtr_server *srv;
>
> This breaks reverse xmas tree ordering of local variables.
> The srv line should be directly above rather than below the node line.
>
>> + unsigned long node_idx;
>> + unsigned long srv_idx;
>>
>> /* Accept only local observers */
>> if (from->sq_node != qrtr_ns.local_node)
>
> ...
Powered by blists - more mailing lists