| Message ID | 20240916170858.2382247-1-quic_yabdulra@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | net: qrtr: Update packets cloning when broadcasting | expand |
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);
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);
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); >
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 --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);
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(-)