Re: [PATCH 2/3] uml: fix proc-vs-interrupt context spinlock deadlock

[Jeff Dike]

On Sun, Aug 06, 2006 at 05:47:03PM +0200, Paolo 'Blaisorblade' Giarrusso wrote:
> From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@xxxxxxxx>
> 
> This spinlock can be taken on interrupt too, so spin_lock_irq[save] must be used.
> 
> However, Documentation/networking/netdevices.txt explains we are called with
> rtnl_lock() held - so we don't need to care about other concurrent opens.
> Verified also in LDD3 and by direct checking. Also verified that the network
> layer (through a state machine) guarantees us that nobody will close the
> interface while it's being used. Please correct me if I'm wrong.
> 
> Also, we must check we don't sleep with irqs disabled!!! But anyway, this is not
> news - we already can't sleep while holding a spinlock. Who says this is
> guaranted really by the present code?

This patch looks fairly scary.  It's protecting the device private
data, you're removing the locking from some accesses and leaving it
(albeit with irqs off now) on others.  It seems to me that can't be
right.  It's either always there, or always not.

You observe that open and close are protected by rtnl_lock.  I observe
that uml_net_change_mtu and uml_net_set_mac are as well, in dev_ioctl.

The spinlock protecting this has to be _irqsave because the interrupt
routine takes it, to protect against receiving packets on an interface
that's being closed.  If that's impossible, we should prove that, and
remove the locking from uml_net_interrupt.

I can't decide about uml_net_start_xmit - there's some RCU stuff
around one call that leads to it, but I don't see any sign of
rtnl_lock.

So, I'd say there are some changes needed here, but they're not
entirely the ones in this patch.

				Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/