* [PATCH] net: korina: remove busy skb free
@ 2020-12-13 17:20 Vincent Stehlé
2020-12-14 10:03 ` Julian Wiedmann
0 siblings, 1 reply; 9+ messages in thread
From: Vincent Stehlé @ 2020-12-13 17:20 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: Vincent Stehlé, David S . Miller, Jakub Kicinski, Florian Fainelli
The ndo_start_xmit() method must not attempt to free the skb to transmit
when returning NETDEV_TX_BUSY. Fix the korina_send_packet() function
accordingly.
Fixes: ef11291bcd5f ("Add support the Korina (IDT RC32434) Ethernet MAC")
Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Florian Fainelli <florian.fainelli@telecomint.eu>
---
drivers/net/ethernet/korina.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
index bf48f0ded9c7d..9d84191de6824 100644
--- a/drivers/net/ethernet/korina.c
+++ b/drivers/net/ethernet/korina.c
@@ -216,7 +216,6 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
netif_stop_queue(dev);
else {
dev->stats.tx_dropped++;
- dev_kfree_skb_any(skb);
spin_unlock_irqrestore(&lp->lock, flags);
return NETDEV_TX_BUSY;
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] net: korina: remove busy skb free
2020-12-13 17:20 [PATCH] net: korina: remove busy skb free Vincent Stehlé
@ 2020-12-14 10:03 ` Julian Wiedmann
2020-12-14 21:08 ` Jakub Kicinski
2020-12-14 21:14 ` [PATCH] net: korina: remove busy skb free Vincent Stehlé
0 siblings, 2 replies; 9+ messages in thread
From: Julian Wiedmann @ 2020-12-14 10:03 UTC (permalink / raw)
To: Vincent Stehlé, netdev, linux-kernel
Cc: David S . Miller, Jakub Kicinski, Florian Fainelli
On 13.12.20 18:20, Vincent Stehlé wrote:
> The ndo_start_xmit() method must not attempt to free the skb to transmit
> when returning NETDEV_TX_BUSY. Fix the korina_send_packet() function
> accordingly.
>
> Fixes: ef11291bcd5f ("Add support the Korina (IDT RC32434) Ethernet MAC")
> Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Florian Fainelli <florian.fainelli@telecomint.eu>
> ---
> drivers/net/ethernet/korina.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
> index bf48f0ded9c7d..9d84191de6824 100644
> --- a/drivers/net/ethernet/korina.c
> +++ b/drivers/net/ethernet/korina.c
> @@ -216,7 +216,6 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
> netif_stop_queue(dev);
> else {
> dev->stats.tx_dropped++;
> - dev_kfree_skb_any(skb);
> spin_unlock_irqrestore(&lp->lock, flags);
>
> return NETDEV_TX_BUSY;
>
As this skb is returned to the stack (and not dropped), the tx_dropped
statistics increment looks bogus too.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] net: korina: remove busy skb free
2020-12-14 10:03 ` Julian Wiedmann
@ 2020-12-14 21:08 ` Jakub Kicinski
2020-12-14 21:27 ` Vincent Stehlé
2020-12-14 22:09 ` [PATCH v2] net: korina: fix return value Vincent Stehlé
2020-12-14 21:14 ` [PATCH] net: korina: remove busy skb free Vincent Stehlé
1 sibling, 2 replies; 9+ messages in thread
From: Jakub Kicinski @ 2020-12-14 21:08 UTC (permalink / raw)
To: Julian Wiedmann
Cc: Vincent Stehlé,
netdev, linux-kernel, David S . Miller, Florian Fainelli
On Mon, 14 Dec 2020 11:03:12 +0100 Julian Wiedmann wrote:
> > diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
> > index bf48f0ded9c7d..9d84191de6824 100644
> > --- a/drivers/net/ethernet/korina.c
> > +++ b/drivers/net/ethernet/korina.c
> > @@ -216,7 +216,6 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
> > netif_stop_queue(dev);
> > else {
> > dev->stats.tx_dropped++;
> > - dev_kfree_skb_any(skb);
> > spin_unlock_irqrestore(&lp->lock, flags);
> >
> > return NETDEV_TX_BUSY;
> >
>
> As this skb is returned to the stack (and not dropped), the tx_dropped
> statistics increment looks bogus too.
Since this is clearly an ugly use after free, and nobody complained we
can assume that the driver correctly stops its TX queue ahead of time.
So perhaps we can change the return value to NETDEV_TX_OK instead.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] net: korina: remove busy skb free
2020-12-14 21:08 ` Jakub Kicinski
@ 2020-12-14 21:27 ` Vincent Stehlé
2020-12-14 22:09 ` [PATCH v2] net: korina: fix return value Vincent Stehlé
1 sibling, 0 replies; 9+ messages in thread
From: Vincent Stehlé @ 2020-12-14 21:27 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Julian Wiedmann, netdev, linux-kernel, David S . Miller,
Florian Fainelli
On Mon, Dec 14, 2020 at 01:08:32PM -0800, Jakub Kicinski wrote:
> On Mon, 14 Dec 2020 11:03:12 +0100 Julian Wiedmann wrote:
> > > diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
> > > index bf48f0ded9c7d..9d84191de6824 100644
> > > --- a/drivers/net/ethernet/korina.c
> > > +++ b/drivers/net/ethernet/korina.c
> > > @@ -216,7 +216,6 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
> > > netif_stop_queue(dev);
> > > else {
> > > dev->stats.tx_dropped++;
> > > - dev_kfree_skb_any(skb);
> > > spin_unlock_irqrestore(&lp->lock, flags);
> > >
> > > return NETDEV_TX_BUSY;
> > >
> >
> > As this skb is returned to the stack (and not dropped), the tx_dropped
> > statistics increment looks bogus too.
>
> Since this is clearly an ugly use after free, and nobody complained we
> can assume that the driver correctly stops its TX queue ahead of time.
> So perhaps we can change the return value to NETDEV_TX_OK instead.
Hi Jakub,
Thanks for the review.
Ok, if this is the preferred fix I will respin the patch this way.
Best regards,
Vincent.
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2] net: korina: fix return value
2020-12-14 21:08 ` Jakub Kicinski
2020-12-14 21:27 ` Vincent Stehlé
@ 2020-12-14 22:09 ` Vincent Stehlé
2020-12-16 20:43 ` Jakub Kicinski
1 sibling, 1 reply; 9+ messages in thread
From: Vincent Stehlé @ 2020-12-14 22:09 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: Julian Wiedmann, Vincent Stehlé,
Jakub Kicinski, David S . Miller, Florian Fainelli
The ndo_start_xmit() method must not attempt to free the skb to transmit
when returning NETDEV_TX_BUSY. Therefore, make sure the
korina_send_packet() function returns NETDEV_TX_OK when it frees a packet.
Fixes: ef11291bcd5f ("Add support the Korina (IDT RC32434) Ethernet MAC")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: Florian Fainelli <florian.fainelli@telecomint.eu>
---
Changes since v1:
- Keep freeing the packet but return NETDEV_TX_OK, as suggested by Jakub
drivers/net/ethernet/korina.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
index bf48f0ded9c7d..925161959b9ba 100644
--- a/drivers/net/ethernet/korina.c
+++ b/drivers/net/ethernet/korina.c
@@ -219,7 +219,7 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
dev_kfree_skb_any(skb);
spin_unlock_irqrestore(&lp->lock, flags);
- return NETDEV_TX_BUSY;
+ return NETDEV_TX_OK;
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] net: korina: fix return value
2020-12-14 22:09 ` [PATCH v2] net: korina: fix return value Vincent Stehlé
@ 2020-12-16 20:43 ` Jakub Kicinski
2020-12-16 21:32 ` Florian Fainelli
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2020-12-16 20:43 UTC (permalink / raw)
To: Vincent Stehlé
Cc: netdev, linux-kernel, Julian Wiedmann, David S . Miller,
Florian Fainelli
On Mon, 14 Dec 2020 23:09:52 +0100 Vincent Stehlé wrote:
> The ndo_start_xmit() method must not attempt to free the skb to transmit
> when returning NETDEV_TX_BUSY. Therefore, make sure the
> korina_send_packet() function returns NETDEV_TX_OK when it frees a packet.
>
> Fixes: ef11291bcd5f ("Add support the Korina (IDT RC32434) Ethernet MAC")
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Florian Fainelli <florian.fainelli@telecomint.eu>
Let me CC Florian's more recent email just in case he wants to review.
> Changes since v1:
> - Keep freeing the packet but return NETDEV_TX_OK, as suggested by Jakub
>
>
> drivers/net/ethernet/korina.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
> index bf48f0ded9c7d..925161959b9ba 100644
> --- a/drivers/net/ethernet/korina.c
> +++ b/drivers/net/ethernet/korina.c
> @@ -219,7 +219,7 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
> dev_kfree_skb_any(skb);
> spin_unlock_irqrestore(&lp->lock, flags);
>
> - return NETDEV_TX_BUSY;
> + return NETDEV_TX_OK;
> }
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] net: korina: fix return value
2020-12-16 20:43 ` Jakub Kicinski
@ 2020-12-16 21:32 ` Florian Fainelli
2020-12-16 23:02 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2020-12-16 21:32 UTC (permalink / raw)
To: Jakub Kicinski, Vincent Stehlé
Cc: netdev, linux-kernel, Julian Wiedmann, David S . Miller
On 12/16/20 12:43 PM, Jakub Kicinski wrote:
> On Mon, 14 Dec 2020 23:09:52 +0100 Vincent Stehlé wrote:
>> The ndo_start_xmit() method must not attempt to free the skb to transmit
>> when returning NETDEV_TX_BUSY. Therefore, make sure the
>> korina_send_packet() function returns NETDEV_TX_OK when it frees a packet.
>>
>> Fixes: ef11291bcd5f ("Add support the Korina (IDT RC32434) Ethernet MAC")
>> Suggested-by: Jakub Kicinski <kuba@kernel.org>
>> Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Florian Fainelli <florian.fainelli@telecomint.eu>
>
> Let me CC Florian's more recent email just in case he wants to review.
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] net: korina: fix return value
2020-12-16 21:32 ` Florian Fainelli
@ 2020-12-16 23:02 ` Jakub Kicinski
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2020-12-16 23:02 UTC (permalink / raw)
To: Florian Fainelli
Cc: Vincent Stehlé,
netdev, linux-kernel, Julian Wiedmann, David S . Miller
On Wed, 16 Dec 2020 13:32:26 -0800 Florian Fainelli wrote:
> On 12/16/20 12:43 PM, Jakub Kicinski wrote:
> > On Mon, 14 Dec 2020 23:09:52 +0100 Vincent Stehlé wrote:
> >> The ndo_start_xmit() method must not attempt to free the skb to transmit
> >> when returning NETDEV_TX_BUSY. Therefore, make sure the
> >> korina_send_packet() function returns NETDEV_TX_OK when it frees a packet.
> >>
> >> Fixes: ef11291bcd5f ("Add support the Korina (IDT RC32434) Ethernet MAC")
> >> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> >> Signed-off-by: Vincent Stehlé <vincent.stehle@laposte.net>
> >> Cc: David S. Miller <davem@davemloft.net>
> >> Cc: Florian Fainelli <florian.fainelli@telecomint.eu>
> >
> > Let me CC Florian's more recent email just in case he wants to review.
>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
😬
Applied, thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: korina: remove busy skb free
2020-12-14 10:03 ` Julian Wiedmann
2020-12-14 21:08 ` Jakub Kicinski
@ 2020-12-14 21:14 ` Vincent Stehlé
1 sibling, 0 replies; 9+ messages in thread
From: Vincent Stehlé @ 2020-12-14 21:14 UTC (permalink / raw)
To: Julian Wiedmann
Cc: netdev, linux-kernel, David S. Miller, Jakub Kicinski, Florian Fainelli
On Mon, Dec 14, 2020 at 11:03:12AM +0100, Julian Wiedmann wrote:
> On 13.12.20 18:20, Vincent Stehlé wrote:
...
> > @@ -216,7 +216,6 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
> > netif_stop_queue(dev);
> > else {
> > dev->stats.tx_dropped++;
> > - dev_kfree_skb_any(skb);
> > spin_unlock_irqrestore(&lp->lock, flags);
> >
> > return NETDEV_TX_BUSY;
> >
>
> As this skb is returned to the stack (and not dropped), the tx_dropped
> statistics increment looks bogus too.
Hi Julian,
Thanks for the review.
I will respin the patch to remove the statistics increment as well.
Best regards,
Vincent.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-12-16 23:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-13 17:20 [PATCH] net: korina: remove busy skb free Vincent Stehlé
2020-12-14 10:03 ` Julian Wiedmann
2020-12-14 21:08 ` Jakub Kicinski
2020-12-14 21:27 ` Vincent Stehlé
2020-12-14 22:09 ` [PATCH v2] net: korina: fix return value Vincent Stehlé
2020-12-16 20:43 ` Jakub Kicinski
2020-12-16 21:32 ` Florian Fainelli
2020-12-16 23:02 ` Jakub Kicinski
2020-12-14 21:14 ` [PATCH] net: korina: remove busy skb free Vincent Stehlé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).