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