: 8139cp: Don't blindly enable interrupts in cp_start_xmit
Subsystems (2)
PATCH 1/1 : 8139cp: Don't blindly enable interrupts in cp_start_xmit
(trying again, this time to the correct maintainer)
All,
Similar to this commit:
It's not safe in cp_start_xmit to blindly call spin_lock_irq and then spin_unlock_irq, since it may very well be the case that cp_start_xmit was called with interrupts already disabled (I came across this bug in the context of netdump in RedHat kernels, but the same issue holds, for example, in netconsole). Therefore, replace all instances of spin_lock_irq and spin_unlock_irq with spin_lock_irqsave and spin_unlock_irqrestore, respectively, in cp_start_xmit(). I tested this against a fully-virtualized Xen guest, which happens to use the 8139cp driver to talk to the emulated hardware. I don't have a real piece of 8139cp hardware to test on, so someone else will have to do that.
drivers/net/8139cp.c +4 -3
@@ -765,17 +765,18 @@ static int cp_start_xmit (struct sk_buff *skb, struct net_device *dev)struct cp_private *cp =netdev_priv (dev);unsigned entry;u32 eor, flags; +unsigned long intr_flags;#if CP_VLAN_TAG_USED u32 vlan_tag =0 ;#endif int mss =0 ; -spin_lock_irq (&cp->lock); +spin_lock_irqsave (&cp->lock, intr_flags);/* This is a hard error, log it. */ if (TX_BUFFS_AVAIL (cp) <= (skb_shinfo (skb)->nr_frags +1 )) {netif_stop_queue (dev); -spin_unlock_irq (&cp->lock); +spin_unlock_irqrestore (&cp->lock, intr_flags); printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n", dev->name);return 1 ;
@@ -908,7 +909,7 @@ static int cp_start_xmit (struct sk_buff *skb, struct net_device *dev)if (TX_BUFFS_AVAIL (cp) <= (MAX_SKB_FRAGS +1 ))netif_stop_queue (dev); -spin_unlock_irq (&cp->lock); +spin_unlock_irqrestore (&cp->lock, intr_flags);cpw8 (TxPoll, NormalTxPoll); dev->trans_start = jiffies;
Chris Lalancette <[email protected]> :
[...]
Quoted 4 lines
Similar to this commit:
Quoted 10 lines
It's not safe in
cp_start_xmitto blindly callspin_lock_irqand thenspin_unlock_irq, since it may very well be the case thatcp_start_xmitwas called with interrupts already disabled (I came across this bug in the context of netdump in RedHat kernels, but the same issue holds, for example, in netconsole). Therefore, replace all instances ofspin_lock_irqandspin_unlock_irqwithspin_lock_irqsaveandspin_unlock_irqrestore, respectively, incp_start_xmit(). I tested this against a fully-virtualized Xen guest, which happens to use the 8139cp driver to talk to the emulated hardware. I don't have a real piece of 8139cp hardware to test on, so someone else will have to do that.
(message reformated to fit in 80 columns, please fix your mailer)
As I understand http://lkml.org/lkml/2006/12/12/239, something like the patch below should had been sent instead. Herbert, ack/nak ?
net/core/netpoll.c +0 -4
@@ -55,7 +55,6 @@ static void queue_process(struct work_struct *work)struct netpoll_info *npinfo = container_of(work, struct netpoll_info, tx_work.work);struct sk_buff *skb; -unsigned long flags;while ((skb =skb_dequeue (&npinfo->txq))) {struct net_device *dev = skb->dev;
@@ -65,19 +64,16 @@ static void queue_process(struct work_struct *work)continue ; } -local_irq_save (flags);netif_tx_lock (dev);if (netif_queue_stopped (dev) || dev->hard_start_xmit (skb, dev) !=NETDEV_TX_OK ) {skb_queue_head (&npinfo->txq, skb);netif_tx_unlock (dev); -local_irq_restore (flags);schedule_delayed_work (&npinfo->tx_work,HZ /10 );return ; }netif_tx_unlock (dev); -local_irq_restore (flags); } }
Quoted 3 lines · Francois Romieu
As I understand http://lkml.org/lkml/2006/12/12/239, something like the patch below should had been sent instead. Herbert, ack/nak ?
Sorry, what I said in that thread is in error. Netpoll may unfortunately call the transmit routine with IRQs off. So the drivers can't currently use spin_lock_irq and must save the current flags instead.
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtQuoted 19 lines · Francois Romieu
Chris Lalancette <[email protected]> :
[...]Quoted 4 lines
Similar to this commit:
Quoted 10 lines
It's not safe in
cp_start_xmitto blindly callspin_lock_irqand thenspin_unlock_irq, since it may very well be the case thatcp_start_xmitwas called with interrupts already disabled (I came across this bug in the context of netdump in RedHat kernels, but the same issue holds, for example, in netconsole). Therefore, replace all instances ofspin_lock_irqandspin_unlock_irqwithspin_lock_irqsaveandspin_unlock_irqrestore, respectively, incp_start_xmit(). I tested this against a fully-virtualized Xen guest, which happens to use the 8139cp driver to talk to the emulated hardware. I don't have a real piece of 8139cp hardware to test on, so someone else will have to do that.
Quoted 5 lines
(message reformated to fit in 80 columns, please fix your mailer)
As I understand http://lkml.org/lkml/2006/12/12/239, something like the patch below should had been sent instead. Herbert, ack/nak ?
Francois,
Thanks for the comments. While the patch you sent will help, there are
still other places that will have problems. For example, in netpoll_send_skb, we call local_irq_save(flags), then call dev->hard_start_xmit(), and then call local_irq_restore(flags). This is a similar situation to what I described above; we will re-enable interrupts in cp_start_xmit(), when netpoll_send_skb doesn't expect that, and will probably run into issues.
Is there a problem with changing cp_start_xmit to use thespin_lock_irqsave(), besides the extra instructions it needs?
Thanks,
Chris Lalancette
Chris Lalancette <[email protected]> :
[...]
Quoted 8 lines
Thanks for the comments. While the patch you sent will help, there are
still other places that will have problems. For example, innetpoll_send_skb, we calllocal_irq_save(flags), then calldev->hard_start_xmit(), and then calllocal_irq_restore(flags). This is a similar situation to what I described above; we will re-enable interrupts incp_start_xmit(), whennetpoll_send_skbdoesn't expect that, and will probably run into issues.
Is there a problem with changingcp_start_xmitto use thespin_lock_irqsave(), besides the extra instructions it needs?
No. Given the history of locking in netpoll and the content of Documentation/networking/netdevices.txt, asking Herbert which rule(s) the code is supposed to follow seemed safer to me.
You can forget my patch.
Please resend your patch inlined to Jeff as described in
http://linux.yyz.us/patch-format.html.
Signature · 1 line
Ueimor
PATCH 1/1 : 8139cp: Don't blindly enable interrupts
Quoted 15 lines · Francois Romieu
Chris Lalancette <[email protected]> :
[...]Quoted 10 lines
Thanks for the comments. While the patch you sent will help, there are
still other places that will have problems. For example, innetpoll_send_skb, we calllocal_irq_save(flags), then calldev->hard_start_xmit(), and then calllocal_irq_restore(flags). This is a similar situation to what I described above; we will re-enable interrupts incp_start_xmit(), whennetpoll_send_skbdoesn't expect that, and will probably run into issues.
Is there a problem with changingcp_start_xmitto use thespin_lock_irqsave(), besides the extra instructions it needs?
Quoted 11 lines
No. Given the history of locking in netpoll and the content of
Documentation/networking/netdevices.txt, asking Herbert which rule(s) the code is supposed to follow seemed safer to me.You can forget my patch.
Please resend your patch inlined to Jeff as described in
http://linux.yyz.us/patch-format.html.
Francois,
Great. Resending mail, shortening subject to < 65 characters and
inlining the patch.
Thanks,
Chris Lalancette
Similar to this commit:
It's not safe in cp_start_xmit to blindly call spin_lock_irq and then spin_unlock_irq, since it may very well be the case that cp_start_xmit was called with interrupts already disabled (I came across this bug in the context of netdump in RedHat kernels, but the same issue holds, for example, in netconsole). Therefore, replace all instances of spin_lock_irq and spin_unlock_irq with spin_lock_irqsave and spin_unlock_irqrestore, respectively, in cp_start_xmit(). I tested this against a fully-virtualized Xen guest using netdump, which happens to use the 8139cp driver to talk to the emulated hardware. I don't have a real piece of 8139cp hardware to test on, so someone else will have to do that.
drivers/net/8139cp.c +4 -3
@@ -765,17 +765,18 @@ static int cp_start_xmit (struct sk_buff *skb, struct net_device *dev)struct cp_private *cp =netdev_priv (dev);unsigned entry;u32 eor, flags; +unsigned long intr_flags;#if CP_VLAN_TAG_USED u32 vlan_tag =0 ;#endif int mss =0 ; -spin_lock_irq (&cp->lock); +spin_lock_irqsave (&cp->lock, intr_flags);/* This is a hard error, log it. */ if (TX_BUFFS_AVAIL (cp) <= (skb_shinfo (skb)->nr_frags +1 )) {netif_stop_queue (dev); -spin_unlock_irq (&cp->lock); +spin_unlock_irqrestore (&cp->lock, intr_flags); printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n", dev->name);return 1 ;
@@ -908,7 +909,7 @@ static int cp_start_xmit (struct sk_buff *skb, struct net_device *dev)if (TX_BUFFS_AVAIL (cp) <= (MAX_SKB_FRAGS +1 ))netif_stop_queue (dev); -spin_unlock_irq (&cp->lock); +spin_unlock_irqrestore (&cp->lock, intr_flags);cpw8 (TxPoll, NormalTxPoll); dev->trans_start = jiffies;
Jeff Garzik
AppliedRe: [PATCH 2.6.20-rc3]: 8139cp: Don't blindly enable interrupts
Quoted 50 lines · Chris Lalancette
Quoted 14 lines · Francois Romieu
Chris Lalancette <[email protected]> :
[...]Quoted 10 lines
Thanks for the comments. While the patch you sent will help, there are
still other places that will have problems. For example, innetpoll_send_skb, we calllocal_irq_save(flags), then calldev->hard_start_xmit(), and then calllocal_irq_restore(flags). This is a similar situation to what I described above; we will re-enable interrupts incp_start_xmit(), whennetpoll_send_skbdoesn't expect that, and will probably run into issues.
Is there a problem with changingcp_start_xmitto use thespin_lock_irqsave(), besides the extra instructions it needs?Quoted 11 lines
No. Given the history of locking in netpoll and the content of
Documentation/networking/netdevices.txt, asking Herbert which rule(s) the code is supposed to follow seemed safer to me.You can forget my patch.
Please resend your patch inlined to Jeff as described in
http://linux.yyz.us/patch-format.html.Francois,
Great. Resending mail, shortening subject to < 65 characters and
inlining the patch.Thanks,
Chris LalancetteSimilar to this commit:
It's not safe in
cp_start_xmitto blindly callspin_lock_irqand thenspin_unlock_irq, since it may very well be the case thatcp_start_xmitwas called with interrupts already disabled (I came across this bug in the context of netdump in RedHat kernels, but the same issue holds, for example, in netconsole). Therefore, replace all instances ofspin_lock_irqandspin_unlock_irqwithspin_lock_irqsaveandspin_unlock_irqrestore, respectively, incp_start_xmit(). I tested this against a fully-virtualized Xen guest using netdump, which happens to use the 8139cp driver to talk to the emulated hardware. I don't have a real piece of 8139cp hardware to test on, so someone else will have to do that.
Quoted 1 line
Signed-off-by Chris Lalancette <[email protected]>
applied.
In the future, please remove the quoted emails stuff, and anything else that does not belong in the kernel changelog. It must be hand-edited out, before using git-am to merge your patch into the kernel tree.
Jeff
Re: [PATCH 2.6.20-rc3]: 8139cp: Don't blindly enable interrupts
Quoted 61 lines · Jeff Garzik
Quoted 56 lines · Chris Lalancette
Quoted 19 lines · Francois Romieu
Chris Lalancette <[email protected]> :
[...]Quoted 5 lines
Thanks for the comments. While the patch you sent will help,
there are
still other places that will have problems. For example, innetpoll_send_skb,
we calllocal_irq_save(flags), then calldev->hard_start_xmit(),Quoted 9 lines
and then call
local_irq_restore(flags). This is a similar situation to what I described
above; we will re-enable interrupts incp_start_xmit(), whennetpoll_send_skb
doesn't expect that, and will probably run into issues.
Is there a problem with changingcp_start_xmitto use thespin_lock_irqsave(), besides the extra instructions it needs?Quoted 11 lines
No. Given the history of locking in netpoll and the content of
Documentation/networking/netdevices.txt, asking Herbert which rule(s) the code is supposed to follow seemed safer to me.You can forget my patch.
Please resend your patch inlined to Jeff as described in
http://linux.yyz.us/patch-format.html.Francois,
Great. Resending mail, shortening subject to < 65 characters and
inlining the patch.Thanks,
Chris LalancetteSimilar to this commit:
It's not safe in
cp_start_xmitto blindly callspin_lock_irqand thenspin_unlock_irq, since it may very well be the case thatcp_start_xmitwas called with interrupts already disabled (I came across this bug in the context of netdump in RedHat kernels, but the same issue holds, for example, in netconsole). Therefore, replace all instances ofspin_lock_irqandspin_unlock_irqwithspin_lock_irqsaveandspin_unlock_irqrestore, respectively, incp_start_xmit(). I tested this against a fully-virtualized Xen guest using netdump, which happens to use the 8139cp driver to talk to the emulated hardware. I don't have a real piece of 8139cp hardware to test on, so someone else will have to do that.Quoted 1 line
Signed-off-by Chris Lalancette <[email protected]>
Quoted 11 lines
applied.
In the future, please remove the quoted emails stuff, and anything else that does not belong in the kernel changelog. It must be hand-edited out, before using git-am to merge your patch into the kernel tree.
Jeff
Jeff,
Ah, I see. Noted. Thanks.
Chris Lalancette