[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZLAQCTcRd2uoHE9i@corigine.com>
Date: Thu, 13 Jul 2023 15:54:01 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Vignesh Viswanathan <quic_viswanat@...cinc.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 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.
...
> @@ -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