Messages in this thread | | | Date | Fri, 14 Jul 2023 11:18:58 +0530 | Subject | Re: [PATCH net-next 1/3] net: qrtr: ns: Change servers radix tree to xarray | From | Vignesh Viswanathan <> |
| |
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@quicinc.com> >> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com> > > Reviewed-by: Simon Horman <simon.horman@corigine.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) > > ...
| |