diff mbox series

net: qrtr: Update packets cloning when broadcasting

Message ID 20240916170858.2382247-1-quic_yabdulra@quicinc.com (mailing list archive)
State Accepted
Commit f011b313e8ebd5b7abd8521b5119aecef403de45
Delegated to: Netdev Maintainers
Headers show
Series net: qrtr: Update packets cloning when broadcasting | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: manivannan.sadhasivam@linaro.org
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 19 this patch: 19
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-09-16--18-00 (tests: 764)

Commit Message

Youssef Samir Sept. 16, 2024, 5:08 p.m. UTC
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(-)

Comments

Chris Lew Sept. 17, 2024, 2:59 p.m. UTC | #1
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);
Youssef Samir Sept. 17, 2024, 5:25 p.m. UTC | #2
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);
Chris Lew Sept. 19, 2024, 1:35 p.m. UTC | #3
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);
>
patchwork-bot+netdevbpf@kernel.org Sept. 24, 2024, 9:02 a.m. UTC | #4
Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 16 Sep 2024 19:08:58 +0200 you 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.
> 
> [...]

Here is the summary with links:
  - net: qrtr: Update packets cloning when broadcasting
    https://git.kernel.org/netdev/net/c/f011b313e8eb

You are awesome, thank you!
diff mbox series

Patch

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