[PATCH] cgroup: make cgrp->event_list_lock irqsafe

[PATCH] cgroup: make cgrp->event_list_lock irqsafe

[Li Zefan]

cgroup_event_wake() is called with hardirq-safe wqh->lock held, so
the nested cgrp->event_list_lock should also be hardirq-safe.

Fortunately I don't think the deadlock can happen in real life.

Lockdep never complained, maybe because it never found wqh->lock was
held in irq context?

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 7a6c4c7..e72c44e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3845,9 +3845,10 @@ static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
 	struct cgroup_event *event = container_of(wait,
 			struct cgroup_event, wait);
 	struct cgroup *cgrp = event->cgrp;
-	unsigned long flags = (unsigned long)key;
+	unsigned long poll_flags = (unsigned long)key;
+	unsigned long flags;
 
-	if (flags & POLLHUP) {
+	if (poll_flags & POLLHUP) {
 		/*
 		 * If the event has been detached at cgroup removal, we
 		 * can simply return knowing the other side will cleanup
@@ -3857,7 +3858,7 @@ static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
 		 * side will require wqh->lock via remove_wait_queue(),
 		 * which we hold.
 		 */
-		spin_lock(&cgrp->event_list_lock);
+		spin_lock_irqsave(&cgrp->event_list_lock, flags);
 		if (!list_empty(&event->list)) {
 			list_del_init(&event->list);
 			/*
@@ -3866,7 +3867,7 @@ static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
 			 */
 			schedule_work(&event->remove);
 		}
-		spin_unlock(&cgrp->event_list_lock);
+		spin_unlock_irqrestore(&cgrp->event_list_lock, flags);
 	}
 
 	return 0;
@@ -3981,9 +3982,9 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	 */
 	dget(cgrp->dentry);
 
-	spin_lock(&cgrp->event_list_lock);
+	spin_lock_irq(&cgrp->event_list_lock);
 	list_add(&event->list, &cgrp->event_list);
-	spin_unlock(&cgrp->event_list_lock);
+	spin_unlock_irq(&cgrp->event_list_lock);
 
 	fput(cfile);
 	fput(efile);
@@ -4406,12 +4407,12 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)
 	 * Notify userspace about cgroup removing only after rmdir of cgroup
 	 * directory to avoid race between userspace and kernelspace.
 	 */
-	spin_lock(&cgrp->event_list_lock);
+	spin_lock_irq(&cgrp->event_list_lock);
 	list_for_each_entry_safe(event, tmp, &cgrp->event_list, list) {
 		list_del_init(&event->list);
 		schedule_work(&event->remove);
 	}
-	spin_unlock(&cgrp->event_list_lock);
+	spin_unlock_irq(&cgrp->event_list_lock);
 
 	return 0;
 }
-- 
1.8.0.2

Re: [PATCH] cgroup: make cgrp->event_list_lock irqsafe

[Tejun Heo]

Hello, Li.

On Wed, Mar 06, 2013 at 11:28:01AM +0800, Li Zefan wrote:
> cgroup_event_wake() is called with hardirq-safe wqh->lock held, so
> the nested cgrp->event_list_lock should also be hardirq-safe.
> 
> Fortunately I don't think the deadlock can happen in real life.
> 
> Lockdep never complained, maybe because it never found wqh->lock was
> held in irq context?

Why should wqh->lock be hard-irq-safe?  Is it actually grabbed from
irq context?  Locks which are grabbed with irq disabled aren't
necessarily irq context locks as that doesn't lead to deadlocks.  They
need to be actually grabbed from irq context.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: make cgrp->event_list_lock irqsafe

[Li Zefan]

On 2013/3/6 14:22, Tejun Heo wrote:
> Hello, Li.
> 
> On Wed, Mar 06, 2013 at 11:28:01AM +0800, Li Zefan wrote:
>> cgroup_event_wake() is called with hardirq-safe wqh->lock held, so
>> the nested cgrp->event_list_lock should also be hardirq-safe.
>>
>> Fortunately I don't think the deadlock can happen in real life.
>>
>> Lockdep never complained, maybe because it never found wqh->lock was
>> held in irq context?
> 
> Why should wqh->lock be hard-irq-safe?  Is it actually grabbed from
> irq context?

becase cgroup_event_wake() is a callback to a wait queue, and it's wake_up()
that acquires wqh->lock with irq disabled.

> Locks which are grabbed with irq disabled aren't
> necessarily irq context locks as that doesn't lead to deadlocks.  They
> need to be actually grabbed from irq context.
> 
> Thanks.
> 

Re: [PATCH] cgroup: make cgrp->event_list_lock irqsafe

[Tejun Heo]

On Tue, Mar 5, 2013 at 11:00 PM, Li Zefan <lizefan@huawei.com> wrote:
>> Why should wqh->lock be hard-irq-safe?  Is it actually grabbed from
>> irq context?
>
> becase cgroup_event_wake() is a callback to a wait queue, and it's wake_up()
> that acquires wqh->lock with irq disabled.

So, acquiring a lock with irq disabled doesn't make it a irq lock.
Being grabbed *from* irq handler makes it a irq lock. Would the
wake_up() happen from irq handler?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: make cgrp->event_list_lock irqsafe

[Li Zefan]

On 2013/3/6 15:02, Tejun Heo wrote:
> On Tue, Mar 5, 2013 at 11:00 PM, Li Zefan <lizefan@huawei.com> wrote:
>>> Why should wqh->lock be hard-irq-safe?  Is it actually grabbed from
>>> irq context?
>>
>> becase cgroup_event_wake() is a callback to a wait queue, and it's wake_up()
>> that acquires wqh->lock with irq disabled.
> 
> So, acquiring a lock with irq disabled doesn't make it a irq lock.
> Being grabbed *from* irq handler makes it a irq lock. Would the
> wake_up() happen from irq handler?
> 

wqh->lock is used through out fs/eventfd.c. I don't know if currently there's
any kernel user using eventfd APIs in an irq handler, but at least that should
be allowed.

wake_up() is also allowed to be called from irq handler?

"allowed" should be enough reason we forbid:

	spin_lock_irqsave(...)
	spin_lock(...)

Re: [PATCH] cgroup: make cgrp->event_list_lock irqsafe

[Tejun Heo]

Hello, Li.

On Tue, Mar 5, 2013 at 11:15 PM, Li Zefan <lizefan@huawei.com> wrote:
> wqh->lock is used through out fs/eventfd.c. I don't know if currently there's
> any kernel user using eventfd APIs in an irq handler, but at least that should
> be allowed.
>
> wake_up() is also allowed to be called from irq handler?

Yeah, well, we don't consider everything which uses wake_up() to be
irq-safe. I'm not necessarily against making the lock irq-safe but at
least the commit message is misleading as currently posted. I'd
suggest looking at eventfd and see whether the wakeup could actually
happen from irq context. If not, I don't know, I don't think it
matters either way. It's not like it's a difficult one to detect with
lockdep.

-- 
tejun

Re: [PATCH] cgroup: make cgrp->event_list_lock irqsafe

[Tejun Heo]

On Tue, Mar 5, 2013 at 11:21 PM, Tejun Heo <tj@kernel.org> wrote:
>> wake_up() is also allowed to be called from irq handler?
>
> Yeah, well, we don't consider everything which uses wake_up() to be
> irq-safe. I'm not necessarily against making the lock irq-safe but at

More precise way to think about it wiatqueue lock would be that it's
built in a way that also allows it to be used from irq context. It
doesn't *force* irq context on all its users.

-- 
tejun