Re: [PATCH] net: korina: remove busy skb free
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(>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.
Re: [PATCH] net: korina: remove busy skb free
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(>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.
Re: [PATCH] net: korina: remove busy skb free
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(>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.
Re: [PATCH] net: korina: remove busy skb free
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é
> Cc: David S. Miller
> Cc: Jakub Kicinski
> Cc: Florian Fainelli
> ---
> 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(>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.
[PATCH] net: korina: remove busy skb free
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é
Cc: David S. Miller
Cc: Jakub Kicinski
Cc: Florian Fainelli
---
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(>lock, flags);
return NETDEV_TX_BUSY;
--
2.29.2
