netpoll + xmit_lock == deadlock

netpoll + xmit_lock == deadlock

[Herbert Xu]

Hi:

While working on TX mitigiation, I noticed that while netpoll
takes care to avoid recursive dead locks on the NAPI path, it
has no protection against the TX path when calling the poll
function.

So if a driver is in the TX path, and a printk occurs, then a
recursive dead lock can occur if that driver tries to take the
xmit lock in its poll function to clean up descriptors.

Fortunately not a lot of drivers do this but at least some are
vulnerable to it, e.g., tg3.

So we need to make it very clear that the poll function must
not take any locks or they must use try_lock if the driver is
to support netpoll.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netpoll + xmit_lock == deadlock

[Matt Mackall]

On Wed, 2009-07-29 at 15:35 +0800, Herbert Xu wrote:
> Hi:
> 
> While working on TX mitigiation, I noticed that while netpoll
> takes care to avoid recursive dead locks on the NAPI path, it
> has no protection against the TX path when calling the poll
> function.
> 
> So if a driver is in the TX path, and a printk occurs, then a
> recursive dead lock can occur if that driver tries to take the
> xmit lock in its poll function to clean up descriptors.
> 
> Fortunately not a lot of drivers do this but at least some are
> vulnerable to it, e.g., tg3.
> 
> So we need to make it very clear that the poll function must
> not take any locks or they must use try_lock if the driver is
> to support netpoll.

What do you propose?

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: netpoll + xmit_lock == deadlock

[Neil Horman]

On Wed, Jul 29, 2009 at 02:07:58PM -0500, Matt Mackall wrote:
> On Wed, 2009-07-29 at 15:35 +0800, Herbert Xu wrote:
> > Hi:
> > 
> > While working on TX mitigiation, I noticed that while netpoll
> > takes care to avoid recursive dead locks on the NAPI path, it
> > has no protection against the TX path when calling the poll
> > function.
> > 
> > So if a driver is in the TX path, and a printk occurs, then a
> > recursive dead lock can occur if that driver tries to take the
> > xmit lock in its poll function to clean up descriptors.
> > 
> > Fortunately not a lot of drivers do this but at least some are
> > vulnerable to it, e.g., tg3.
> > 
> > So we need to make it very clear that the poll function must
> > not take any locks or they must use try_lock if the driver is
> > to support netpoll.
> 
> What do you propose?

I think there is actually some recursion protection.  If you look in
netpoll_send_skb (where all netpoll transmits pass through), we do a
__netif_tx_trylock, and only continue down the tx path if we obtain the lock.
If not, we call netpoll_poll, wait a while, and try again.  I think that should
prevent the deadlock condition you are concerned about.

Regards
Neil (who hates netpoll). :)

> 
> -- 
> http://selenic.com : development and support for Mercurial and Linux
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: netpoll + xmit_lock == deadlock

[Matt Mackall]

On Wed, 2009-07-29 at 15:43 -0400, Neil Horman wrote:
> On Wed, Jul 29, 2009 at 02:07:58PM -0500, Matt Mackall wrote:
> > On Wed, 2009-07-29 at 15:35 +0800, Herbert Xu wrote:
> > > Hi:
> > > 
> > > While working on TX mitigiation, I noticed that while netpoll
> > > takes care to avoid recursive dead locks on the NAPI path, it
> > > has no protection against the TX path when calling the poll
> > > function.
> > > 
> > > So if a driver is in the TX path, and a printk occurs, then a
> > > recursive dead lock can occur if that driver tries to take the
> > > xmit lock in its poll function to clean up descriptors.
> > > 
> > > Fortunately not a lot of drivers do this but at least some are
> > > vulnerable to it, e.g., tg3.
> > > 
> > > So we need to make it very clear that the poll function must
> > > not take any locks or they must use try_lock if the driver is
> > > to support netpoll.
> > 
> > What do you propose?
> 
> I think there is actually some recursion protection.  If you look in
> netpoll_send_skb (where all netpoll transmits pass through), we do a
> __netif_tx_trylock, and only continue down the tx path if we obtain the lock.
> If not, we call netpoll_poll, wait a while, and try again.  I think that should
> prevent the deadlock condition you are concerned about.

Maybe. The general point remains that drivers implementing poll need to
be aware of possible recursion through printk/netconsole in the xmit
path. If there are private locks, netpoll is powerless to prevent
recursive lock attempts.

It occurs to me that we might be able to know when we've moved from core
kernel into a driver's tx path by wrapping the tx method pointer or call
its call sites with something that disabled netconsole until it exited.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: netpoll + xmit_lock == deadlock

[Herbert Xu]

On Wed, Jul 29, 2009 at 03:43:00PM -0400, Neil Horman wrote:
> 
> I think there is actually some recursion protection.  If you look in
> netpoll_send_skb (where all netpoll transmits pass through), we do a
> __netif_tx_trylock, and only continue down the tx path if we obtain the lock.
> If not, we call netpoll_poll, wait a while, and try again.  I think that should
> prevent the deadlock condition you are concerned about.

When netpoll_poll calls dev->poll which then takes the TX lock 
you have no protection whatsoever.  Some drivers do this in order
to ensure that TX descriptor cleanups do not race against the
transmitting side.  See tg3 for an example.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netpoll + xmit_lock == deadlock

[Neil Horman]

On Wed, Jul 29, 2009 at 04:48:17PM -0500, Matt Mackall wrote:
> On Wed, 2009-07-29 at 15:43 -0400, Neil Horman wrote:
> > On Wed, Jul 29, 2009 at 02:07:58PM -0500, Matt Mackall wrote:
> > > On Wed, 2009-07-29 at 15:35 +0800, Herbert Xu wrote:
> > > > Hi:
> > > > 
> > > > While working on TX mitigiation, I noticed that while netpoll
> > > > takes care to avoid recursive dead locks on the NAPI path, it
> > > > has no protection against the TX path when calling the poll
> > > > function.
> > > > 
> > > > So if a driver is in the TX path, and a printk occurs, then a
> > > > recursive dead lock can occur if that driver tries to take the
> > > > xmit lock in its poll function to clean up descriptors.
> > > > 
> > > > Fortunately not a lot of drivers do this but at least some are
> > > > vulnerable to it, e.g., tg3.
> > > > 
> > > > So we need to make it very clear that the poll function must
> > > > not take any locks or they must use try_lock if the driver is
> > > > to support netpoll.
> > > 
> > > What do you propose?
> > 
> > I think there is actually some recursion protection.  If you look in
> > netpoll_send_skb (where all netpoll transmits pass through), we do a
> > __netif_tx_trylock, and only continue down the tx path if we obtain the lock.
> > If not, we call netpoll_poll, wait a while, and try again.  I think that should
> > prevent the deadlock condition you are concerned about.
> 
> Maybe. The general point remains that drivers implementing poll need to
> be aware of possible recursion through printk/netconsole in the xmit
> path. If there are private locks, netpoll is powerless to prevent
> recursive lock attempts.
> 
Not quite.  I agree private locking in a driver is a pain when you consider
netpoll clients, its not the tx/tx recursion you need to worry about though, its
shared locking between the tx and rx path that you need to be worried about.
We should be protected against deadlock on the _xmit_lock from what we discussed
above, but if you take a lock in the driver, then call printk, its possible that
you'll go down the ->poll routine path in the driver.  If there you try to take
the same private lock, the result is then deadlock.


> It occurs to me that we might be able to know when we've moved from core
> kernel into a driver's tx path by wrapping the tx method pointer or call
> its call sites with something that disabled netconsole until it exited.
> 

I was thinking that perhaps what we should do is simply not call netpoll_poll
from within netpoll_send_skb.  That breaks the only spot that I see in which we
call a receive code from within the tx path, breaking the deadlock possibilty.
Perhaps instead we can call netif_rx_schedule on the network interfaces napi
struct.  We already queue the frames and set a timer to try sending again later.
By calling netif_rx_schedule, we move the receive work to the net_rx_action
softirq (where it really should be).

Thoughts?
Neil

> -- 
> http://selenic.com : development and support for Mercurial and Linux
> 
> 
> 

Re: netpoll + xmit_lock == deadlock

[Herbert Xu]

On Wed, Jul 29, 2009 at 07:15:17PM -0400, Neil Horman wrote:
>
> Not quite.  I agree private locking in a driver is a pain when you consider
> netpoll clients, its not the tx/tx recursion you need to worry about though, its
> shared locking between the tx and rx path that you need to be worried about.
> We should be protected against deadlock on the _xmit_lock from what we discussed
> above, but if you take a lock in the driver, then call printk, its possible that
> you'll go down the ->poll routine path in the driver.  If there you try to take
> the same private lock, the result is then deadlock.

xmit_lock suffers from exactly the same problem in ->poll.

> I was thinking that perhaps what we should do is simply not call netpoll_poll
> from within netpoll_send_skb.  That breaks the only spot that I see in which we
> call a receive code from within the tx path, breaking the deadlock possibilty.
> Perhaps instead we can call netif_rx_schedule on the network interfaces napi
> struct.  We already queue the frames and set a timer to try sending again later.
> By calling netif_rx_schedule, we move the receive work to the net_rx_action
> softirq (where it really should be).
> 
> Thoughts?

Alternatively we can modify the drivers to use try lock or other
mechanisms that do not result in a dead-lock.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netpoll + xmit_lock == deadlock

[Neil Horman]

On Thu, Jul 30, 2009 at 07:17:35AM +0800, Herbert Xu wrote:
> On Wed, Jul 29, 2009 at 07:15:17PM -0400, Neil Horman wrote:
> >
> > Not quite.  I agree private locking in a driver is a pain when you consider
> > netpoll clients, its not the tx/tx recursion you need to worry about though, its
> > shared locking between the tx and rx path that you need to be worried about.
> > We should be protected against deadlock on the _xmit_lock from what we discussed
> > above, but if you take a lock in the driver, then call printk, its possible that
> > you'll go down the ->poll routine path in the driver.  If there you try to take
> > the same private lock, the result is then deadlock.
> 
> xmit_lock suffers from exactly the same problem in ->poll.
> 
Under what conditions do you try to take the xmit_lock from within a drivers
->poll routine?  Do you have an example?

Neil

Re: netpoll + xmit_lock == deadlock

[Neil Horman]

On Thu, Jul 30, 2009 at 06:38:32AM +0800, Herbert Xu wrote:
> On Wed, Jul 29, 2009 at 03:43:00PM -0400, Neil Horman wrote:
> > 
> > I think there is actually some recursion protection.  If you look in
> > netpoll_send_skb (where all netpoll transmits pass through), we do a
> > __netif_tx_trylock, and only continue down the tx path if we obtain the lock.
> > If not, we call netpoll_poll, wait a while, and try again.  I think that should
> > prevent the deadlock condition you are concerned about.
> 
> When netpoll_poll calls dev->poll which then takes the TX lock 
> you have no protection whatsoever.  Some drivers do this in order
> to ensure that TX descriptor cleanups do not race against the
> transmitting side.  See tg3 for an example.

but nothing in that path takes the xmit_lock.  The poll_lock is taken in that
patch, but thats for recieve_side syncronization, not transmit side.  Nothing in
the tg3 driver (to use your example), that I can see takes the xmit_lock in the
rx path either, so I'm not really sure where you comming from here.
Neil

> 
> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: netpoll + xmit_lock == deadlock

[Herbert Xu]

On Wed, Jul 29, 2009 at 09:02:22PM -0400, Neil Horman wrote:
>
> Under what conditions do you try to take the xmit_lock from within a drivers
> ->poll routine?  Do you have an example?

tg3
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netpoll + xmit_lock == deadlock

[Herbert Xu]

On Wed, Jul 29, 2009 at 09:06:39PM -0400, Neil Horman wrote:
>
> but nothing in that path takes the xmit_lock.  The poll_lock is taken in that
> patch, but thats for recieve_side syncronization, not transmit side.  Nothing in
> the tg3 driver (to use your example), that I can see takes the xmit_lock in the
> rx path either, so I'm not really sure where you comming from here.

tg3_poll => tg3_poll_work => tg3_tx => netif_tx_lock
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netpoll + xmit_lock == deadlock

[Neil Horman]

On Fri, Jul 31, 2009 at 09:30:17AM +0800, Herbert Xu wrote:
> On Wed, Jul 29, 2009 at 09:06:39PM -0400, Neil Horman wrote:
> >
> > but nothing in that path takes the xmit_lock.  The poll_lock is taken in that
> > patch, but thats for recieve_side syncronization, not transmit side.  Nothing in
> > the tg3 driver (to use your example), that I can see takes the xmit_lock in the
> > rx path either, so I'm not really sure where you comming from here.
> 
> tg3_poll => tg3_poll_work => tg3_tx => netif_tx_lock

Oh, goodness, thats just asking for disaster.  Setting asside the netpoll issue
for the moment, what if we take an rx interrupt on a cpu while in the middle of
sending a frame?  Whats to stop the NET_RX_SOFTIRQ after the hard interrupt and
recursively taking the _xmit_lock?  With or without netpoll, that seems prone to
deadlock.

Neil

> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 

Re: netpoll + xmit_lock == deadlock

[Herbert Xu]

On Fri, Jul 31, 2009 at 08:56:54AM -0400, Neil Horman wrote:
>
> > tg3_poll => tg3_poll_work => tg3_tx => netif_tx_lock
> 
> Oh, goodness, thats just asking for disaster.  Setting asside the netpoll issue
> for the moment, what if we take an rx interrupt on a cpu while in the middle of
> sending a frame?  Whats to stop the NET_RX_SOFTIRQ after the hard interrupt and
> recursively taking the _xmit_lock?  With or without netpoll, that seems prone to
> deadlock.

No that can't happen because BH is disabled in the xmit function.

This problem is specific to netpoll because it does things that
normally can't happen with BH off.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netpoll + xmit_lock == deadlock

[Neil Horman]

On Fri, Jul 31, 2009 at 09:02:43PM +0800, Herbert Xu wrote:
> On Fri, Jul 31, 2009 at 08:56:54AM -0400, Neil Horman wrote:
> >
> > > tg3_poll => tg3_poll_work => tg3_tx => netif_tx_lock
> > 
> > Oh, goodness, thats just asking for disaster.  Setting asside the netpoll issue
> > for the moment, what if we take an rx interrupt on a cpu while in the middle of
> > sending a frame?  Whats to stop the NET_RX_SOFTIRQ after the hard interrupt and
> > recursively taking the _xmit_lock?  With or without netpoll, that seems prone to
> > deadlock.
> 
> No that can't happen because BH is disabled in the xmit function.
> 
> This problem is specific to netpoll because it does things that
> normally can't happen with BH off.
> 
Ugh, you're right, my bad.  

In fact, looking back, we had a similar problem (but not identical) in RHEL, in
which the netpoll path was removing the device from the poll_list from a
different cpu, leading to a double free.  I fixed it by adding a state bit to
the napi flags that let helper functions know that we were in a netpoll context,
which let us avoid doing stupid things.  Perhaps such a solution might be
usefull here.  Set the flag when calling the poll routine from a netpoll
context, so napi_tx_lock, would know to do a trylock and always return success
or something of that nature.

I'd also be up for simply ripping out netpoll entirely..... ;)

Neil

> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 

Re: netpoll + xmit_lock == deadlock

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Jul 2009 15:35:23 +0800

> So if a driver is in the TX path, and a printk occurs, then a
> recursive dead lock can occur if that driver tries to take the
> xmit lock in its poll function to clean up descriptors.

My position has always been that such printk's are simply
not allowed.  (check archives if you don't believe me :-)

The locking is going to get rediculious if we start having
to account for this.

Re: netpoll + xmit_lock == deadlock

[Ben Hutchings]

On Sun, 2009-08-02 at 13:07 -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 29 Jul 2009 15:35:23 +0800
> 
> > So if a driver is in the TX path, and a printk occurs, then a
> > recursive dead lock can occur if that driver tries to take the
> > xmit lock in its poll function to clean up descriptors.
> 
> My position has always been that such printk's are simply
> not allowed.  (check archives if you don't believe me :-)
>
> The locking is going to get rediculious if we start having
> to account for this.

I agree with that, but this does seem quite restrictive.  How can we be
sure that none of the kernel functions used by a driver's TX path (e.g.
kmalloc or DMA-mapping) will print debug or warning messages?  If such
guarantees exist, they do not seem to be documented.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: netpoll + xmit_lock == deadlock

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 03 Aug 2009 13:05:57 +0100

> On Sun, 2009-08-02 at 13:07 -0700, David Miller wrote:
>> My position has always been that such printk's are simply
>> not allowed.  (check archives if you don't believe me :-)
>>
>> The locking is going to get rediculious if we start having
>> to account for this.
> 
> I agree with that, but this does seem quite restrictive.  How can we be
> sure that none of the kernel functions used by a driver's TX path (e.g.
> kmalloc or DMA-mapping) will print debug or warning messages?  If such
> guarantees exist, they do not seem to be documented.

Indeed, that's a good counter-argument.

Ho hum... so someone show me how ugly the locking is going
to get :-)

Re: netpoll + xmit_lock == deadlock

[Matt Mackall]

On Mon, 2009-08-03 at 12:15 -0700, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Mon, 03 Aug 2009 13:05:57 +0100
> 
> > On Sun, 2009-08-02 at 13:07 -0700, David Miller wrote:
> >> My position has always been that such printk's are simply
> >> not allowed.  (check archives if you don't believe me :-)
> >>
> >> The locking is going to get rediculious if we start having
> >> to account for this.
> > 
> > I agree with that, but this does seem quite restrictive.  How can we be
> > sure that none of the kernel functions used by a driver's TX path (e.g.
> > kmalloc or DMA-mapping) will print debug or warning messages?  If such
> > guarantees exist, they do not seem to be documented.
> 
> Indeed, that's a good counter-argument.
> 
> Ho hum... so someone show me how ugly the locking is going
> to get :-)

First, I don't think we can solve the problem of 'reliably deliver
printks inside the TX path'. If the driver needs to printk here, odds
are good that sending isn't possible.

So I think we should be clear up front that the solution we're looking
for is 'don't crash the box when trying to printk from the tx path'.

I think the most straightforward, driver-agnostic way to do this
is to have netpoll wrap a driver's TX entrypoint such that recursion is
disabled while in that path. This might actually simplify netpoll's
internal locking ugliness. Something like:

int netpoll_xmit_wrapper(struct sk_buff *skb, struct net_device *dev)
{
	struct netpoll_info *npinfo = dev->npinfo;
	int ret;

	npinfo->recurse += 1; /* add appropriate locking */
	ret = npinfo->orig_tx(skb, dev);
	npinfo->recurse -= 1;
	
	return ret;
}

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: netpoll + xmit_lock == deadlock

[Herbert Xu]

On Sun, Aug 02, 2009 at 01:07:04PM -0700, David Miller wrote:
>
> My position has always been that such printk's are simply
> not allowed.  (check archives if you don't believe me :-)

Well that means we'd also have to ban printks from all IRQ handlers
etc.

Not that I dislike your way since it would makes things so much
simpler :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: netpoll + xmit_lock == deadlock

[David Miller]

From: Matt Mackall <mpm@selenic.com>
Date: Mon, 03 Aug 2009 14:59:41 -0500

> First, I don't think we can solve the problem of 'reliably deliver
> printks inside the TX path'. If the driver needs to printk here, odds
> are good that sending isn't possible.

Agreed.

> So I think we should be clear up front that the solution we're looking
> for is 'don't crash the box when trying to printk from the tx path'.
> 
> I think the most straightforward, driver-agnostic way to do this
> is to have netpoll wrap a driver's TX entrypoint such that recursion is
> disabled while in that path. This might actually simplify netpoll's
> internal locking ugliness. Something like:
> 
> int netpoll_xmit_wrapper(struct sk_buff *skb, struct net_device *dev)
> {
> 	struct netpoll_info *npinfo = dev->npinfo;
> 	int ret;
> 
> 	npinfo->recurse += 1; /* add appropriate locking */
> 	ret = npinfo->orig_tx(skb, dev);
> 	npinfo->recurse -= 1;
> 	
> 	return ret;
> }

Looks workable.  Probably we even hold the netpoll lock here so
no additional protection would even be needed.

Re: netpoll + xmit_lock == deadlock

[Herbert Xu]

On Mon, Aug 03, 2009 at 09:19:05PM -0700, David Miller wrote:
>
> > So I think we should be clear up front that the solution we're looking
> > for is 'don't crash the box when trying to printk from the tx path'.
> > 
> > I think the most straightforward, driver-agnostic way to do this
> > is to have netpoll wrap a driver's TX entrypoint such that recursion is
> > disabled while in that path. This might actually simplify netpoll's
> > internal locking ugliness. Something like:
> > 
> > int netpoll_xmit_wrapper(struct sk_buff *skb, struct net_device *dev)
> > {
> > 	struct netpoll_info *npinfo = dev->npinfo;
> > 	int ret;
> > 
> > 	npinfo->recurse += 1; /* add appropriate locking */
> > 	ret = npinfo->orig_tx(skb, dev);
> > 	npinfo->recurse -= 1;
> > 	
> > 	return ret;
> > }
> 
> Looks workable.  Probably we even hold the netpoll lock here so
> no additional protection would even be needed.

Yes that should solve the xmit problem.  However, we could still
have similar issues with other parts of the driver.  For instance,
the driver may take a lock in one of the ethtool/control functions
that is also taken on either the xmit or poll path.  If you then
get a printk in the ethtool/control function then you'll be in the
same situation.

Basically anytime we're executing driver code we may be vulnerable
to a netpoll dead-lock.

So I think your suggestion should be extended to cover all entries
into the driver code, and not just the xmit function.  Perhaps
something like

#define netdev_driver_op(op, ...)				\
	({							\
		struct netpoll_info *npinfo = dev->npinfo;	\
		int ret; 					\
								\
		npinfo->recurse++;				\
		ret = op(__VA_ARGS__);				\
		npinfo->recurse--;
								\
		ret;						\
	})

This still leaves driver timers and other async driver code.  We
can add a similar marker for them but someone will have to audit
the drivers.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt