On Wed. 4 Nov 2020 10:21, Jakub Kicinski wrote: > On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote: >> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> >> >> If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but >> not always, the case), the 'WARN_ON(in_irq)' in >> net/core/skbuff.c#skb_release_head_state() might be triggered, under network >> congestion circumstances, together with the potential risk of a NULL pointer >> dereference. >> >> The root cause of this issue is the call to kfree_skb() instead of >> dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog(). >> >> This patch prevents the skb to be freed within the call to netif_rx() by >> incrementing its reference count with skb_get(). The skb is finally freed by >> one of the in-irq-context safe functions: dev_consume_skb_any() or >> dev_kfree_skb_any(). The "any" version is used because some drivers might call >> can_get_echo_skb() in a normal context. >> >> The reason for this issue to occur is that initially, in the core network >> stack, loopback skb were not supposed to be received in hardware IRQ context. >> The CAN stack is an exception. >> >> This bug was previously reported back in 2017 in [1] but the proposed patch >> never got accepted. >> >> While [1] directly modifies net/core/dev.c, we try to propose here a >> smoother modification local to CAN network stack (the assumption >> behind is that only CAN devices are affected by this issue). >> >> [1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@xxxxxxxxxxx >> >> Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> >> Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@xxxxxxxxxx >> Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface") >> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > > Hm... Why do we receive a skb with a socket attached? > > At a quick glance this is some loopback, so shouldn't we skb_orphan() > in the xmit function instead? This is a specific feature of SocketCAN. See: https://www.kernel.org/doc/html/latest/networking/can.html#local-loopback-of-sent-frames By default, each socket will receive the loopback packets from other sockets but not its own sent frames. This behaviour can be controlled by the socket option CAN_RAW_RECV_OWN_MSGS (c.f. member 'recv_own_msg' in 'struct raw_sok': https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L88) This feature requires to have the socket attached to the skb. Orphaning the skb would break it (c.f. function raw_rcv(): https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L116). > Otherwise we should probably fix this in enqueue_to_backlog(). To my knowledge, this issue only occurs in SocketCAN, thus the idea to try to fix it locally. But yes, replacing kfree_skb() with dev_kfree_skb_any() in enqueue_to_backlog() would fix the issue as well. >> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c >> index b70ded3760f2..73cfcd7e9517 100644 >> --- a/drivers/net/can/dev.c >> +++ b/drivers/net/can/dev.c >> @@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx) >> if (!skb) >> return 0; >> >> - netif_rx(skb); >> + skb_get(skb); >> + if (netif_rx(skb) == NET_RX_SUCCESS) >> + dev_consume_skb_any(skb); >> + else >> + dev_kfree_skb_any(skb); >> >> return len; >> } > Yours sincerely, Vincent Mailhol
