| Message ID | 20201213172052.12433-1-vincent.stehle@laposte.net |
|---|---|
| State | New |
| Headers | show |
| Series | net: korina: remove busy skb free | expand |
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.
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; > } > } >
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
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!
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;
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(-)