diff mbox

Change the spin_lock/unlock_irq interface in proc_alloc_inum() function

Message ID 1456886879-28128-1-git-send-email-majun258@huawei.com
State New
Headers show

Commit Message

majun (F) March 2, 2016, 2:47 a.m. UTC
From: Ma Jun <majun258@huawei.com>


The spin_lock/unlock_irq interface is not safe when this function is called
at some case which need irq disabled.

For example:
	spin_lock_irqsave()
	|
	request_irq() --> proc_alloc_inum()
	|
	spin_unlock_irqrestore()

Reported-by: Fan Jinke <fanjinke1@huawei.com>
Signed-off-by: Ma Jun <majun258@huawei.com>

---
 fs/proc/generic.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

-- 
1.7.1

Comments

majun (F) March 2, 2016, 6:32 a.m. UTC | #1
在 2016/3/2 11:09, Al Viro 写道:
> On Wed, Mar 02, 2016 at 10:47:59AM +0800, MaJun wrote:

>> From: Ma Jun <majun258@huawei.com>

>>

>> The spin_lock/unlock_irq interface is not safe when this function is called

>> at some case which need irq disabled.

> 

>> For example:

>> 	spin_lock_irqsave()

>> 	|

>> 	request_irq() --> proc_alloc_inum()

>> 	|

>> 	spin_unlock_irqrestore()

> 

> Do you even read your own patch?

> 

>>  	if (!ida_pre_get(&proc_inum_ida, GFP_KERNEL))

> 					 ^^^^^^^^^^

> 					 This.

> 

> It can block.  You *can't* call that under spin_lock_irqsave().  At all.

> You also can't do request_irq() under a spinlock, no matter whether you

> disable irqs or not - it also blocks.  So does proc_mkdir(), for that

> matter, and not only in proc_alloc_inum().

> 

> NAKed.  Don't do it.  request_irq() is not to be called under spinlocks,

> with or without irqs disabled.

> 


Sorry,I made a wrong example for this problem.
I want to say this interface may change the irq status after this function
be called.

Thanks!
MaJun

> .

>
diff mbox

Patch

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index ff3ffc7..4fc1502 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -191,23 +191,24 @@  int proc_alloc_inum(unsigned int *inum)
 {
 	unsigned int i;
 	int error;
+	unsigned long flags;
 
 retry:
 	if (!ida_pre_get(&proc_inum_ida, GFP_KERNEL))
 		return -ENOMEM;
 
-	spin_lock_irq(&proc_inum_lock);
+	spin_lock_irqsave(&proc_inum_lock, flags);
 	error = ida_get_new(&proc_inum_ida, &i);
-	spin_unlock_irq(&proc_inum_lock);
+	spin_unlock_irqrestore(&proc_inum_lock, flags);
 	if (error == -EAGAIN)
 		goto retry;
 	else if (error)
 		return error;
 
 	if (i > UINT_MAX - PROC_DYNAMIC_FIRST) {
-		spin_lock_irq(&proc_inum_lock);
+		spin_lock_irqsave(&proc_inum_lock, flags);
 		ida_remove(&proc_inum_ida, i);
-		spin_unlock_irq(&proc_inum_lock);
+		spin_unlock_irqrestore(&proc_inum_lock, flags);
 		return -ENOSPC;
 	}
 	*inum = PROC_DYNAMIC_FIRST + i;