On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@xxxxxxxxxx> 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 exeption. > > > > 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? Yes this would work, this seems the safest way, loopback_xmit() is a good template for this. > > Otherwise we should probably fix this in enqueue_to_backlog(). This is dangerous. If we drop packets under flood because the per-cpu backlog is full, we might also be in _big_ trouble if the per-cpu softnet_data.completion_queue is filling, since we do not have a limit on this list. What could happen is that when the memory is finally exhausted and no more skb can be fed to netif_rx(), a big latency spike would happen when softnet_data.completion_queue can finally be purged in one shot. So skb_orphan(skb) in CAN before calling netif_rx() is better IMO. > > > 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; > > } >
