Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a69a750a-884b-c60b-9d52-f4ae2cd1b2ea@quicinc.com>
Date:   Fri, 14 Jul 2023 11:18:58 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.2
Subject: Re: [PATCH net-next 1/3] net: qrtr: ns: Change servers radix tree to
 xarray
To:     Simon Horman <simon.horman@corigine.com>
CC:     <mani@kernel.org>, <davem@davemloft.net>, <edumazet@google.com>,
        <kuba@kernel.org>, <pabeni@redhat.com>,
        <linux-arm-msm@vger.kernel.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_srichara@quicinc.com>,
        <quic_clew@quicinc.com>
References: <20230712112631.3461793-1-quic_viswanat@quicinc.com>
 <20230712112631.3461793-2-quic_viswanat@quicinc.com>
 <ZLAQCTcRd2uoHE9i@corigine.com>
Content-Language: en-US
From:   Vignesh Viswanathan <quic_viswanat@quicinc.com>
In-Reply-To: <ZLAQCTcRd2uoHE9i@corigine.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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