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

[Chris Lew]

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@xxxxxxxxxxx>

> > > 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@xxxxxxxxxxx>
> > > Reviewed-by: Jeffery Hugo <quic_jhugo@xxxxxxxxxxx>
> > > Reviewed-by: Carl Vanderlip <quic_carlv@xxxxxxxxxxx>
> > > ---
> > >    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);