| Message ID | 20220311080208.45047-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: mcba_usb: fix possible double dev_kfree_skb in mcba_usb_start_xmit | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Gentle ping. On 2022/3/11 16:02, Hangyu Hua wrote: > There is no need to call dev_kfree_skb when usb_submit_urb fails beacause > can_put_echo_skb deletes original skb and can_free_echo_skb deletes the cloned > skb. > > Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > drivers/net/can/usb/mcba_usb.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c > index 77bddff86252..7c198eb5bc9c 100644 > --- a/drivers/net/can/usb/mcba_usb.c > +++ b/drivers/net/can/usb/mcba_usb.c > @@ -364,7 +364,6 @@ static netdev_tx_t mcba_usb_start_xmit(struct sk_buff *skb, > xmit_failed: > can_free_echo_skb(priv->netdev, ctx->ndx, NULL); > mcba_usb_free_ctx(ctx); > - dev_kfree_skb(skb); > stats->tx_dropped++; > > return NETDEV_TX_OK;
Hi Hangyu, On Fri, Mar 11, 2022 at 5:02 PM Hangyu Hua <hbh25y@gmail.com> wrote: > > There is no need to call dev_kfree_skb when usb_submit_urb fails beacause > can_put_echo_skb deletes original skb and can_free_echo_skb deletes the cloned > skb. So, it's more like, "we don't need to call dev_kfree_skb() after can_put_echo_skb() because can_put_echo_skb() consumes the given skb.". It seems it doesn't depend on the condition of usb_submit_urb(). Plus, we don't see the "cloned skb" at the call site. Would you mind adding a comment on can_put_echo_skb(), in a separate patch, saying the fact that it consumes the skb? ems_usb.c, gs_usb.c and possibly some others seem to call dev_kfree_skb() as well. Are they affected? Best,
Hi yashi, You are right. There are a series of the same problems about can_put_echo_skb. This issue was discovered while I was discussing a incorrect patch with Marc. You can check this in https://lore.kernel.org/all/20220225060019.21220-1-hbh25y@gmail.com/ So i submitted a new patch. This was the first place where the problem occurs. You can check this in [1] https://lore.kernel.org/all/20220228083639.38183-1-hbh25y@gmail.com/ After a week, I realized it could be a series of problems. So i submitted the following patches [2] https://lore.kernel.org/all/0d2f9980-fb1d-4068-7868-effc77892a97@gmail.com/ [3] https://lore.kernel.org/all/de416319-c027-837d-4b8c-b8c3c37ed88e@gmail.com/ [4] https://lore.kernel.org/all/20220317081305.739554-1-mkl@pengutronix.de/ I think this is all affected. None of the four have been merged upstream. Do i need to remake all these four patches? Thanks, Hangyu On 2022/3/23 07:08, Yasushi SHOJI wrote: > Hi Hangyu, > > On Fri, Mar 11, 2022 at 5:02 PM Hangyu Hua <hbh25y@gmail.com> wrote: >> >> There is no need to call dev_kfree_skb when usb_submit_urb fails beacause >> can_put_echo_skb deletes original skb and can_free_echo_skb deletes the cloned >> skb. > > So, it's more like, "we don't need to call dev_kfree_skb() after > can_put_echo_skb() > because can_put_echo_skb() consumes the given skb.". It seems it doesn't depend > on the condition of usb_submit_urb(). Plus, we don't see the "cloned > skb" at the > call site. > > Would you mind adding a comment on can_put_echo_skb(), in a separate patch, > saying the fact that it consumes the skb? > > ems_usb.c, gs_usb.c and possibly some others seem to call > dev_kfree_skb() as well. > Are they affected? > > Best,
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c index 77bddff86252..7c198eb5bc9c 100644 --- a/drivers/net/can/usb/mcba_usb.c +++ b/drivers/net/can/usb/mcba_usb.c @@ -364,7 +364,6 @@ static netdev_tx_t mcba_usb_start_xmit(struct sk_buff *skb, xmit_failed: can_free_echo_skb(priv->netdev, ctx->ndx, NULL); mcba_usb_free_ctx(ctx); - dev_kfree_skb(skb); stats->tx_dropped++; return NETDEV_TX_OK;
There is no need to call dev_kfree_skb when usb_submit_urb fails beacause can_put_echo_skb deletes original skb and can_free_echo_skb deletes the cloned skb. Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer") Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- drivers/net/can/usb/mcba_usb.c | 1 - 1 file changed, 1 deletion(-)