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@xxxxxxxxxxx>
Signed-off-by: Vignesh Viswanathan <quic_viswanat@xxxxxxxxxxx>
Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>
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)
...
