| Message ID | 20201103220636.972106-6-mkl@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote: > From: Vincent Mailhol <mailhol.vincent@wanadoo.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@wanadoo.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@pengutronix.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? Otherwise we should probably fix this in enqueue_to_backlog(). > 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; > }
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@wanadoo.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@wanadoo.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@pengutronix.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
On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote: > > From: Vincent Mailhol <mailhol.vincent@wanadoo.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@wanadoo.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@pengutronix.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; > > } >
Hello Eric, On 04.11.20 09:16, Eric Dumazet wrote: > On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@kernel.org> wrote: >> >> On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote: >>> From: Vincent Mailhol <mailhol.vincent@wanadoo.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@wanadoo.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@pengutronix.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. > Unfortunately you missed the answer from Vincent, why skb_orphan() does not work here: https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/ Best regards, Oliver >> >>> 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; >>> } >>
On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote: > Hello Eric, > > On 04.11.20 09:16, Eric Dumazet wrote: > > On Wed, Nov 4, 2020 at 2:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > >> > >> On Tue, 3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote: > >>> From: Vincent Mailhol <mailhol.vincent@wanadoo.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@wanadoo.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@pengutronix.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. Thanks, that makes sense. So no to the enqueue_to_backlog() idea. > > So skb_orphan(skb) in CAN before calling netif_rx() is better IMO. > > > > Unfortunately you missed the answer from Vincent, why skb_orphan() does > not work here: > > https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/ Okay, we can take this as a quick fix but to me it seems a little strange to be dropping what is effectively locally generated frames. Can we use a NAPI poll model here and back pressure TX if the echo is not keeping up?
On 04.11.20 17:02, Jakub Kicinski wrote: > On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote: >> On 04.11.20 09:16, Eric Dumazet wrote: >>> So skb_orphan(skb) in CAN before calling netif_rx() is better IMO. >>> >> >> Unfortunately you missed the answer from Vincent, why skb_orphan() does >> not work here: >> >> https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/ > > Okay, we can take this as a quick fix but to me it seems a little > strange to be dropping what is effectively locally generated frames. Thanks! So this patch doesn't hinder Marc's PR :-) > Can we use a NAPI poll model here and back pressure TX if the echo > is not keeping up? Some of the CAN network drivers already support NAPI. @Marc: Can we also use NAPI for echo'ing the skbs? Best regards, Oliver
On Wed, 5 Nov 2020 02:46, Oliver Hartkopp wrote: > On 04.11.20 17:02, Jakub Kicinski wrote: >> On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote: >>> On 04.11.20 09:16, Eric Dumazet wrote: > >>>> So skb_orphan(skb) in CAN before calling netif_rx() is better IMO. >>>> >>> >>> Unfortunately you missed the answer from Vincent, why skb_orphan() does >>> not work here: Hope that my answer did not go to the spam box. >>> https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@mail.gmail.com/ >> >> Okay, we can take this as a quick fix but to me it seems a little >> strange to be dropping what is effectively locally generated frames. For those who are not familiar with SocketCAN and to make sure that we are all aligned here, let me give a bit of context of how the echo on CAN skbs is usually implement in the drivers: * In the xmit() branch, the driver would queue the skb using can_put_echo_skb(). * Whenever the driver gets notified of a successful TX, it will loopback the skb using can_get_echo_skb(). This is why the loopback is usually done in hardware IRQ context (but drivers are also free to skip the second step and directly loopback the skb in the xmit() branch). > Thanks! So this patch doesn't hinder Marc's PR :-) > >> Can we use a NAPI poll model here and back pressure TX if the echo >> is not keeping up? > > Some of the CAN network drivers already support NAPI. I had a quick look at NAPI in the past and my understanding is that it requires the ability to turn off hardware interrupts meaning that it can be only used on some NIC, not but not, for example, on USB devices. It would be nice to extend the NAPI with skb loopback for drivers which already supports it but I am not sure how to include the other drivers. > @Marc: Can we also use NAPI for echo'ing the skbs? Yours sincerely, Vincent Mailhol
On 11/5/20 5:47 AM, Vincent MAILHOL wrote: >>> Okay, we can take this as a quick fix but to me it seems a little >>> strange to be dropping what is effectively locally generated frames. > > For those who are not familiar with SocketCAN and to make sure that we > are all aligned here, let me give a bit of context of how the echo on CAN > skbs is usually implement in the drivers: > * In the xmit() branch, the driver would queue the skb using > can_put_echo_skb(). > * Whenever the driver gets notified of a successful TX, it will loopback > the skb using can_get_echo_skb(). > > This is why the loopback is usually done in hardware IRQ context (but > drivers are also free to skip the second step and directly loopback the > skb in the xmit() branch). > >> Thanks! So this patch doesn't hinder Marc's PR :-) >> >>> Can we use a NAPI poll model here and back pressure TX if the echo >>> is not keeping up? >> >> Some of the CAN network drivers already support NAPI. And some drivers (i.e. flexcan and mcp251xfd) already queue the ech_skb via NAPI. > I had a quick look at NAPI in the past and my understanding is that it > requires the ability to turn off hardware interrupts meaning that it can > be only used on some NIC, not but not, for example, on USB devices. > > It would be nice to extend the NAPI with skb loopback for drivers which > already supports it but I am not sure how to include the other drivers. > >> @Marc: Can we also use NAPI for echo'ing the skbs? The flexcan driver does: https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/flexcan.c#L1080 In the interrupt handler, the driver pushes the echo_skb into the rx-offload queue and triggers NAPI. The rx-offload pushed the skb into the networking stack via NAPI. Here the code in the mcp251xfd driver: https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1237 Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
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; }