I think the unlock should be before bugging?
--
unlock before bug returns
Signed-off-by: Roel Kluin <[email protected]>
---
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5a4cc20..c910170 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -357,9 +357,8 @@ void gpmc_cs_free(int cs)
spin_lock(&gpmc_mem_lock);
if (cs >= GPMC_CS_NUM || !gpmc_cs_reserved(cs)) {
printk(KERN_ERR "Trying to free non-reserved GPMC CS%d\n", cs);
- BUG();
spin_unlock(&gpmc_mem_lock);
- return;
+ BUG();
}
gpmc_cs_disable_mem(cs);
release_resource(&gpmc_cs_mem[cs]);
Roel Kluin wrote:
> unlock before bug returns
> if (cs >= GPMC_CS_NUM || !gpmc_cs_reserved(cs)) {
> printk(KERN_ERR "Trying to free non-reserved GPMC CS%d\n", cs);
> - BUG();
> spin_unlock(&gpmc_mem_lock);
> - return;
> + BUG();
should we bother to unlock before panicking, or is the unlock not required either?
On Mon, 22 Oct 2007 04:58:45 +0200
Roel Kluin <[email protected]> wrote:
> Roel Kluin wrote:
>
> > unlock before bug returns
>
> > if (cs >= GPMC_CS_NUM || !gpmc_cs_reserved(cs)) {
> > printk(KERN_ERR "Trying to free non-reserved GPMC
> > CS%d\n", cs);
> > - BUG();
> > spin_unlock(&gpmc_mem_lock);
> > - return;
> > + BUG();
>
>
> should we bother to unlock before panicking, or is the unlock not
> required either?
BUG() kills the current process, but not the whole system.
Unlocking the lock means that the rest of the system has somewhat
of a chance of surviving. Not unlocking means a guaranteed hang
for the rest of the system, making a BUG() no better than panic.
Please keep the unlock.
--
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan
>> should we bother to unlock before panicking, or is the unlock not
>> required either?
>
> BUG() kills the current process, but not the whole system.
>
> Unlocking the lock means that the rest of the system has somewhat
> of a chance of surviving. Not unlocking means a guaranteed hang
> for the rest of the system, making a BUG() no better than panic.
>
> Please keep the unlock.
In that case I guess the pathc below fixes some more unlockings before
bugging.
in the third patch: maybe BUG before "page_cache_release(page)"?
I also spotted some cases where it was attempted to free after BUG().
should that occur before BUG() as well?
--
Unlock before BUG()
Signed-off-by: Roel Kluin <[email protected]>
---
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 88629a3..679c8b4 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -100,8 +100,10 @@ static ssize_t vol_attribute_show(struct device *dev,
ret = sprintf(buf, "%lld\n", vol->used_bytes);
else if (attr == &attr_vol_upd_marker)
ret = sprintf(buf, "%d\n", vol->upd_marker);
- else
+ else {
+ spin_unlock(&vol->ubi->volumes_lock);
BUG();
+ }
spin_unlock(&vol->ubi->volumes_lock);
return ret;
}
diff --git a/drivers/net/wireless/ipw2200.c b/drivers/net/wireless/ipw2200.c
index e3c8284..67ed205 100644
--- a/drivers/net/wireless/ipw2200.c
+++ b/drivers/net/wireless/ipw2200.c
@@ -8722,6 +8722,7 @@ static int ipw_wx_get_freq(struct net_device *dev,
break;
default:
+ mutex_unlock(&priv->mutex);
BUG();
}
} else
diff --git a/fs/buffer.c b/fs/buffer.c
index 76403b1..460c17d 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1051,9 +1051,9 @@ grow_dev_page(struct block_device *bdev, sector_t block,
return page;
failed:
- BUG();
unlock_page(page);
page_cache_release(page);
+ BUG();
return NULL;
}
diff --git a/mm/slab.c b/mm/slab.c
index cfa6be4..20c58dc 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1606,8 +1606,10 @@ void __init kmem_cache_init(void)
struct kmem_cache *cachep;
mutex_lock(&cache_chain_mutex);
list_for_each_entry(cachep, &cache_chain, next)
- if (enable_cpucache(cachep))
+ if (enable_cpucache(cachep)) {
+ mutex_unlock(&cache_chain_mutex);
BUG();
+ }
mutex_unlock(&cache_chain_mutex);
}
diff --git a/net/rxrpc/ar-ack.c b/net/rxrpc/ar-ack.c
index 657ee69..c56e773 100644
--- a/net/rxrpc/ar-ack.c
+++ b/net/rxrpc/ar-ack.c
@@ -752,8 +752,10 @@ all_acked:
sp->call = call;
rxrpc_get_call(call);
spin_lock_bh(&call->lock);
- if (rxrpc_queue_rcv_skb(call, skb, true, true) < 0)
+ if (rxrpc_queue_rcv_skb(call, skb, true, true) < 0) {
+ spin_unlock_bh(&call->lock);
BUG();
+ }
spin_unlock_bh(&call->lock);
goto process_further;
}
diff --git a/net/rxrpc/ar-call.c b/net/rxrpc/ar-call.c
index 3c04b00..0d30466 100644
--- a/net/rxrpc/ar-call.c
+++ b/net/rxrpc/ar-call.c
@@ -426,8 +426,10 @@ void rxrpc_release_call(struct rxrpc_call *call)
call->rx_first_oos);
spin_lock_bh(&call->lock);
- if (test_and_set_bit(RXRPC_CALL_RELEASED, &call->flags))
+ if (test_and_set_bit(RXRPC_CALL_RELEASED, &call->flags)) {
+ spin_unlock_bh(&call->lock);
BUG();
+ }
spin_unlock_bh(&call->lock);
/* dissociate from the socket
Hi Roel,
On 10/22/07, Roel Kluin <[email protected]> wrote:
> diff --git a/mm/slab.c b/mm/slab.c
> index cfa6be4..20c58dc 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1606,8 +1606,10 @@ void __init kmem_cache_init(void)
> struct kmem_cache *cachep;
> mutex_lock(&cache_chain_mutex);
> list_for_each_entry(cachep, &cache_chain, next)
> - if (enable_cpucache(cachep))
> + if (enable_cpucache(cachep)) {
> + mutex_unlock(&cache_chain_mutex);
> BUG();
> + }
> mutex_unlock(&cache_chain_mutex);
> }
NAK. This will cause double-unlock when CONFIG_BUG is disabled. It's
incorrect to assume that BUG() will always terminate the current
process.
On 10/22/2007 02:40 PM, Pekka Enberg wrote:
> On 10/22/07, Roel Kluin <[email protected]> wrote:
>> diff --git a/mm/slab.c b/mm/slab.c
>> index cfa6be4..20c58dc 100644
>> --- a/mm/slab.c
>> +++ b/mm/slab.c
>> @@ -1606,8 +1606,10 @@ void __init kmem_cache_init(void)
>> struct kmem_cache *cachep;
>> mutex_lock(&cache_chain_mutex);
>> list_for_each_entry(cachep, &cache_chain, next)
>> - if (enable_cpucache(cachep))
>> + if (enable_cpucache(cachep)) {
>> + mutex_unlock(&cache_chain_mutex);
>> BUG();
>> + }
>> mutex_unlock(&cache_chain_mutex);
>> }
>
> NAK. This will cause double-unlock when CONFIG_BUG is disabled. It's
> incorrect to assume that BUG() will always terminate the current
> process.
(which by the way also means that the "return;" delete from your original
patch changes behaviour for !CONFIG_BUG, and probably not for the better).
Rene.
Rene Herman wrote:
> On 10/22/2007 02:40 PM, Pekka Enberg wrote:
>
>> NAK. This will cause double-unlock when CONFIG_BUG is disabled. It's
>> incorrect to assume that BUG() will always terminate the current
>> process.
>
> (which by the way also means that the "return;" delete from your
> original patch changes behaviour for !CONFIG_BUG, and probably not for
> the better).
>
> Rene.
Thanks for your comments. A patch containing this suggestion is:
[PATCH retry] return hidden bug and unlock bugs.
Roel