Date:   Wed, 4 Nov 2020 09:16:57 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>,
        netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, linux-can@...r.kernel.org,
        kernel@...gutronix.de, Vincent Mailhol <mailhol.vincent@...adoo.fr>
Subject: Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to
 kfree_skb() in hard IRQ context

On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue,  3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote:
> > From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> >
> > 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@cetitec.com
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> > Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
> > Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
> > Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
>
> 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;
> >  }
>

Powered by blists - more mailing lists