Re: [PATCH v2] net: qrtr: send msgs from local of same id as broadcast

[王文虎 <wenhu.wang@vivo.com>]

From: Bjorn Andersson <bjorn.andersson@linaro.org>
Date: 2020-04-10 03:36:00
To:  WANG Wenhu <wenhu.wang@vivo.com>
Cc:  "David S. Miller" <davem@davemloft.net>,Jakub Kicinski <kuba@kernel.org>,Carl Huang <cjhuang@codeaurora.org>,
Thomas Gleixner <tglx@linutronix.de>,Arnd Bergmann <arnd@arndb.de>,
Nicholas Mc Guire <hofrat@osadl.org>,netdev@vger.kernel.org,linux-kernel@vger.kernel.org,kernel@vivo.com
Subject: Re: [PATCH v2] net: qrtr: send msgs from local of same id as broadcast>On Tue 07 Apr 20:32 PDT 2020, WANG Wenhu wrote:
>
>> From: Wang Wenhu <wenhu.wang@vivo.com>
>> 
>> If the local node id(qrtr_local_nid) is not modified after its
>> initialization, it equals to the broadcast node id(QRTR_NODE_BCAST).
>> So the messages from local node should not be taken as broadcast
>> and keep the process going to send them out anyway.
>> 
>> The definitions are as follow:
>> static unsigned int qrtr_local_nid = NUMA_NO_NODE;
>> 
>> Fixes: commit fdf5fd397566 ("net: qrtr: Broadcast messages only from control port")
>> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
>> ---
>> Changlog:
>>  - For coding style, line up the newline of the if conditional judgement
>>    with the one exists before.
>> 
>>  net/qrtr/qrtr.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
>> index 5a8e42ad1504..545a61f8ef75 100644
>> --- a/net/qrtr/qrtr.c
>> +++ b/net/qrtr/qrtr.c
>> @@ -907,20 +907,21 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>>  
>>  	node = NULL;
>>  	if (addr->sq_node == QRTR_NODE_BCAST) {
>> -		enqueue_fn = qrtr_bcast_enqueue;
>> -		if (addr->sq_port != QRTR_PORT_CTRL) {
>> +		if (addr->sq_port != QRTR_PORT_CTRL &&
>> +			qrtr_local_nid != QRTR_NODE_BCAST) {
>
>So this would mean that if local_nid is configured to be the bcast
>address then rather than rejecting messages to non-control ports we will
>broadcast them.
>
>What happens when some other node in the network replies? Wouldn't it be
>better to explicitly prohibit usage of the bcast address as our node
>address?

>>
>That said, in torvalds/master qrtr_local_nid is no longer initialized to
>QRTR_NODE_BCAST, but 1. So I don't think you need this patch anymore.
>
>Regards,
>Bjorn

>

Hi Bjorn,
You are right. I see the patch that modified the nid to 1 in mainline v5.7-rc1,
and it is better to solve all the problems. As for this patch, the situation is that
I want to use qrtr in kernel to do something else(to develop another driver I name
it RPMON: Remote Processor Monitor), but the ns or service-route functionality
had been missing, so I write another file qsr.c as you had commetted which
did the same thing with ns.c.

The bad thing was I missed the patch from Manivannan.
So, anyway, this patch is not needed anymore.

Thanks,
Wenhu

>>  			release_sock(sk);
>>  			return -ENOTCONN;
>>  		}
>> +		enqueue_fn = qrtr_bcast_enqueue;
>>  	} else if (addr->sq_node == ipc->us.sq_node) {
>>  		enqueue_fn = qrtr_local_enqueue;
>>  	} else {
>> -		enqueue_fn = qrtr_node_enqueue;
>>  		node = qrtr_node_lookup(addr->sq_node);
>>  		if (!node) {
>>  			release_sock(sk);
>>  			return -ECONNRESET;
>>  		}
>> +		enqueue_fn = qrtr_node_enqueue;
>>  	}
>>  
>>  	plen = (len + 3) & ~3;
>> -- 
>> 2.17.1
>>