Re: [PATCH net-next 1/3] net: qrtr: ns: Change servers radix tree to xarray

[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@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)
>
> ...