Skip to contentSkip to conversation

unlock before bug returns

PATCH In review Roel Kluin 4 participants 7 messages Oct 22, 2007

Subsystems (12)
Networking Memory management Arm (32-bit) VFS +8 more subsystems
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 unlock before bug returns

I think the unlock should be before bugging?

--
unlock before bug returns

Signed-off-by Roel Kluin <[email protected]>

---

arch/arm/mach-omap2/gpmc.c +1 -2
index 5a4cc20..c910170 100644
@@ -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]);
author
#1 lore raw

arch/arm/mach-omap2/gpmc.c void gpmc_cs_free(int cs) · line 361

Quoted 8 lines · Roel Kluin

unlock before bug returns

arch/arm/mach-omap2/gpmc.c +1 -2
 	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?

#2 lore raw
Quoted 4 lines · On Mon, 22 Oct 2007 04:58:45 +0200 Roel Kluin <[email protected]> wrote:
Quoted 1 line · Roel Kluin

unlock before bug returns

Quoted 11 lines

arch/arm/mach-omap2/gpmc.c void gpmc_cs_free(int cs) · line 361

Quoted 7 lines
(quoted diff) +0 -0
 	if (cs >= GPMC_CS_NUM || !gpmc_cs_reserved(cs)) {
 		printk(KERN_ERR "Trying to free non-reserved GPMC

CS%d\n", cs);

arch/arm/mach-omap2/gpmc.c +1 -2
-		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.

Signature · 3 lines
"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
author
#3 lore raw
Quoted 11 lines
Quoted 3 lines

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]>

---

drivers/mtd/ubi/vmt.c +3 -1
index 88629a3..679c8b4 100644
@@ -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;
 }
drivers/net/wireless/ipw2200.c +1 -0
index e3c8284..67ed205 100644
@@ -8722,6 +8722,7 @@ static int ipw_wx_get_freq(struct net_device *dev,
 			break;
 
 		default:
+			mutex_unlock(&priv->mutex);
 			BUG();
 		}
 	} else
fs/buffer.c +1 -1
index 76403b1..460c17d 100644
@@ -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;
 }
mm/slab.c +3 -1
index cfa6be4..20c58dc 100644
@@ -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);
 	}
net/rxrpc/ar-ack.c +3 -1
index 657ee69..c56e773 100644
@@ -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;
 	}
net/rxrpc/ar-call.c +3 -1
index 3c04b00..0d30466 100644
@@ -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
#4 lore raw

Hi Roel,

Quoted 5 lines · Roel Kluin
mm/slab.c +0 -0
index cfa6be4..20c58dc 100644

mm/slab.c void __init kmem_cache_init(void) · line 1614

Quoted 10 lines
mm/slab.c +3 -1
@@ -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.

#5 lore raw
Quoted 12 lines · Pekka Enberg
Quoted 11 lines · Roel Kluin
mm/slab.c +2 -1
index cfa6be4..20c58dc 100644
@@ -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);
Quoted 8 lines

mm/slab.c void __init kmem_cache_init(void) · line 1614

Quoted 4 lines
mm/slab.c +1 -0
                                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.

author
#6 lore raw
Quoted 11 lines · Rene Herman
Quoted 3 lines · Pekka Enberg

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