net: qrtr: Update packets cloning when broadcasting

[PATCH] net: qrtr: Update packets cloning when broadcasting

Posted by Youssef Samir 2 months, 1 week ago

When broadcasting data to multiple nodes via MHI, using skb_clone()
causes all nodes to receive the same header data. This can result in
packets being discarded by endpoints, leading to lost data.

This issue occurs when a socket is closed, and a QRTR_TYPE_DEL_CLIENT
packet is broadcasted. All nodes receive the same destination node ID,
causing the node connected to the client to discard the packet and
remain unaware of the client's deletion.

Replace skb_clone() with pskb_copy(), to create a separate copy of
the header for each sk_buff.

Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")
Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
Reviewed-by: Jeffery Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
---
 net/qrtr/af_qrtr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 41ece61eb57a..00c51cf693f3 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -884,7 +884,7 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 
 	mutex_lock(&qrtr_node_lock);
 	list_for_each_entry(node, &qrtr_all_nodes, item) {
-		skbn = skb_clone(skb, GFP_KERNEL);
+		skbn = pskb_copy(skb, GFP_KERNEL);
 		if (!skbn)
 			break;
 		skb_set_owner_w(skbn, skb->sk);
-- 
2.25.1

Re: [PATCH] net: qrtr: Update packets cloning when broadcasting

Posted by Chris Lew 2 months, 1 week ago

Hi Youssef,

On 9/16/2024 10:08 AM, Youssef Samir wrote:
> When broadcasting data to multiple nodes via MHI, using skb_clone()
> causes all nodes to receive the same header data. This can result in
> packets being discarded by endpoints, leading to lost data.
> 
> This issue occurs when a socket is closed, and a QRTR_TYPE_DEL_CLIENT
> packet is broadcasted. All nodes receive the same destination node ID,
> causing the node connected to the client to discard the packet and
> remain unaware of the client's deletion.
> 

I guess this never happens for the SMD/RPMSG transport because the skb 
is consumed within the context of qrtr_node_enqueue where as MHI queues 
the skb to be transmitted later.

Does the duplicate destination node ID match the last node in the 
qrtr_all_nodes list?


> Replace skb_clone() with pskb_copy(), to create a separate copy of
> the header for each sk_buff.
> 
> Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Jeffery Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
> ---
>   net/qrtr/af_qrtr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index 41ece61eb57a..00c51cf693f3 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -884,7 +884,7 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>   
>   	mutex_lock(&qrtr_node_lock);
>   	list_for_each_entry(node, &qrtr_all_nodes, item) {
> -		skbn = skb_clone(skb, GFP_KERNEL);
> +		skbn = pskb_copy(skb, GFP_KERNEL);
>   		if (!skbn)
>   			break;
>   		skb_set_owner_w(skbn, skb->sk);

Re: [PATCH] net: qrtr: Update packets cloning when broadcasting

Posted by Youssef Samir 2 months, 1 week ago

Hi Chris,

On 9/17/2024 3:59 PM, Chris Lew wrote:
> Hi Youssef,
> 
> On 9/16/2024 10:08 AM, Youssef Samir wrote:
>> When broadcasting data to multiple nodes via MHI, using skb_clone()
>> causes all nodes to receive the same header data. This can result in
>> packets being discarded by endpoints, leading to lost data.
>>
>> This issue occurs when a socket is closed, and a QRTR_TYPE_DEL_CLIENT
>> packet is broadcasted. All nodes receive the same destination node ID,
>> causing the node connected to the client to discard the packet and
>> remain unaware of the client's deletion.
>>
> 
> I guess this never happens for the SMD/RPMSG transport because the skb is consumed within the context of qrtr_node_enqueue where as MHI queues the skb to be transmitted later.
> 
> Does the duplicate destination node ID match the last node in the qrtr_all_nodes list?

Yes, it always matches the last node in the qrtr_all_nodes list.

> 
> 
>> Replace skb_clone() with pskb_copy(), to create a separate copy of
>> the header for each sk_buff.
>>
>> Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")
>> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
>> Reviewed-by: Jeffery Hugo <quic_jhugo@quicinc.com>
>> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
>> ---
>>   net/qrtr/af_qrtr.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
>> index 41ece61eb57a..00c51cf693f3 100644
>> --- a/net/qrtr/af_qrtr.c
>> +++ b/net/qrtr/af_qrtr.c
>> @@ -884,7 +884,7 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>>         mutex_lock(&qrtr_node_lock);
>>       list_for_each_entry(node, &qrtr_all_nodes, item) {
>> -        skbn = skb_clone(skb, GFP_KERNEL);
>> +        skbn = pskb_copy(skb, GFP_KERNEL);
>>           if (!skbn)
>>               break;
>>           skb_set_owner_w(skbn, skb->sk);

Re: [PATCH] net: qrtr: Update packets cloning when broadcasting

Posted by Chris Lew 2 months, 1 week ago


On 9/17/2024 10:25 AM, Youssef Samir wrote:
> Hi Chris,
> 
> On 9/17/2024 3:59 PM, Chris Lew wrote:
>> Hi Youssef,
>>
>> On 9/16/2024 10:08 AM, Youssef Samir wrote:
>>> When broadcasting data to multiple nodes via MHI, using skb_clone()
>>> causes all nodes to receive the same header data. This can result in
>>> packets being discarded by endpoints, leading to lost data.
>>>
>>> This issue occurs when a socket is closed, and a QRTR_TYPE_DEL_CLIENT
>>> packet is broadcasted. All nodes receive the same destination node ID,
>>> causing the node connected to the client to discard the packet and
>>> remain unaware of the client's deletion.
>>>
>>
>> I guess this never happens for the SMD/RPMSG transport because the skb is consumed within the context of qrtr_node_enqueue where as MHI queues the skb to be transmitted later.
>>
>> Does the duplicate destination node ID match the last node in the qrtr_all_nodes list?
> 
> Yes, it always matches the last node in the qrtr_all_nodes list.
> 

Thanks for the confirmation, we haven't seen this before since most of 
our platforms usually only use one MHI qrtr node.

Reviewed-by: Chris Lew <quic_clew@quicinc.com>

>>
>>
>>> Replace skb_clone() with pskb_copy(), to create a separate copy of
>>> the header for each sk_buff.
>>>
>>> Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")
>>> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
>>> Reviewed-by: Jeffery Hugo <quic_jhugo@quicinc.com>
>>> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
>>> ---
>>>    net/qrtr/af_qrtr.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
>>> index 41ece61eb57a..00c51cf693f3 100644
>>> --- a/net/qrtr/af_qrtr.c
>>> +++ b/net/qrtr/af_qrtr.c
>>> @@ -884,7 +884,7 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
>>>          mutex_lock(&qrtr_node_lock);
>>>        list_for_each_entry(node, &qrtr_all_nodes, item) {
>>> -        skbn = skb_clone(skb, GFP_KERNEL);
>>> +        skbn = pskb_copy(skb, GFP_KERNEL);
>>>            if (!skbn)
>>>                break;
>>>            skb_set_owner_w(skbn, skb->sk);
>