Date:   Wed, 4 Nov 2020 13:29:19 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Jakub Kicinski <kuba@...nel.org>,
        linux-can <linux-can@...r.kernel.org>,
        Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kernel@...gutronix.de,
        Eric Dumazet <edumazet@...gle.com>,
        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. 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@...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 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@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?

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

Powered by blists - more mailing lists