Re: [PATCH] net: via-velocity.c fix sleep-with-spinlock bug during MTU change
From: Andrew Morton <akpm@linux-foundation.org>
Date: 2008-06-03 20:48:05
Also in:
lkml
On Sat, 31 May 2008 18:46:15 -0700 Arjan van de Ven [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Arjan van de Ven <redacted> Subject: [PATCH] net: via-velocity.c fix sleep-with-spinlock bug during MTU change The via-velocity.c driver reinitializes (frees/allocates) several metadata structures during an MTU change. Unfortunately the allocations of the new versions of the metadata is done with GFP_KERNEL, even though this change of datastructures is (and needs to be) done while holding a spinlock (with irqs off). Clearly that isn't a good thing, and kerneloops.org has trapped a large deal of the resulting warnings. The fix is to use GFP_ATOMIC. While not elegant, avoiding the lock is going to be extremely complex. In addition, this is a "static", long lived allocation (after all, how often do you actually change your mtu) and not something that happens on an ongoing basis. ...diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c index 6b8d882..4bf08fd 100644 --- a/drivers/net/via-velocity.c +++ b/drivers/net/via-velocity.c@@ -1,4 +1,4 @@ -/* +;/*
Cat sat on your keyboard?
quoted hunk ↗ jump to hunk
* This code is derived from the VIA reference driver (copyright message * below) provided to Red Hat by VIA Networking Technologies, Inc. for * addition to the Linux kernel.@@ -1241,6 +1241,9 @@ static int velocity_rx_refill(struct velocity_info *vptr) * * Allocate and set up the receive buffers for each ring slot and * assign them to the network adapter. + * + * Note: This function gets called with irqs off/lock held + * from velocity_change_mtu() */ static int velocity_init_rd_ring(struct velocity_info *vptr)@@ -1251,7 +1254,7 @@ static int velocity_init_rd_ring(struct velocity_info *vptr) vptr->rx_buf_sz = (mtu <= ETH_DATA_LEN) ? PKT_BUF_SZ : mtu + 32; vptr->rd_info = kcalloc(vptr->options.numrx, - sizeof(struct velocity_rd_info), GFP_KERNEL); + sizeof(struct velocity_rd_info), GFP_ATOMIC);
What happens if this allocation fails? I think the driver is dead? We've gone and freed the rd_ring and the td_ring and we _might_ have allocated a new rd_ring and not a new td_ring. And we've set vptr->rx_buf_sz, which may or may not be a problem. And we've gone and left the interface in a downed state. So hrm. It could all be a lot better. Just looking quickly at the code I _think_ we might be able to do all the needed allocations outside the lock and then swizzle them into place after taking the lock. ie, something as simple as: struct velocity_info *temp_vptr; ... velocity_init_rd_ring(temp_vptr); /* Can use GFP_KERNEL! */ spin_lock_irqsave(&vptr->lock, flags); velocity_free_td_ring(vptr); velocity_free_rd_ring(vptr); vptr->foo = temp_vptr->foo; vptr->bar = temp_vptr->bar; ... spin_unlock_irqrestore(&vptr->lock, flags); ?