2.6.23-rc6-mm1: IPC: sleeping function called ...

I'm getting tons of this, and X fails to start

CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_PREEMPT_NONE is not set
# CONFIG_PREEMPT_VOLUNTARY is not set
CONFIG_PREEMPT=y
CONFIG_PREEMPT_BKL=y
CONFIG_DEBUG_PREEMPT=y

BUG: sleeping function called from invalid context at kernel/rwsem.c:47
in_atomic():1, irqs_disabled():0
no locks held by X/5879.
 [<c012bcb1>] down_write+0x15/0x50
 [<c01b53af>] do_shmat+0x235/0x3a0
 [<c0106be2>] sys_ipc+0x146/0x263
 [<c0102892>] sysenter_past_esp+0xa7/0xb5
 [<c0102856>] sysenter_past_esp+0x6b/0xb5

I'm getting tons of this, and X fails to start

CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_PREEMPT_NONE is not set
# CONFIG_PREEMPT_VOLUNTARY is not set
CONFIG_PREEMPT=y
CONFIG_PREEMPT_BKL=y
CONFIG_DEBUG_PREEMPT=y

BUG: sleeping function called from invalid context at kernel/rwsem.c:47
in_atomic():1, irqs_disabled():0
no locks held by X/5879.
 [<c012bcb1>] down_write+0x15/0x50
 [<c01b53af>] do_shmat+0x235/0x3a0
 [<c0106be2>] sys_ipc+0x146/0x263
 [<c0102892>] sysenter_past_esp+0xa7/0xb5
 [<c0102856>] sysenter_past_esp+0x6b/0xb5
 =======================

Quoted 10 lines · On Tue, 18 Sep 2007 13:17:28 +0400 Alexey Dobriyan <[email protected]> wrote:

I'm getting tons of this, and X fails to start

CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_PREEMPT_NONE is not set
# CONFIG_PREEMPT_VOLUNTARY is not set
CONFIG_PREEMPT=y
CONFIG_PREEMPT_BKL=y
CONFIG_DEBUG_PREEMPT=y

Quoted 9 lines

BUG: sleeping function called from invalid context at kernel/rwsem.c:47
in_atomic():1, irqs_disabled():0
no locks held by X/5879.
[<c012bcb1>] down_write+0x15/0x50
[<c01b53af>] do_shmat+0x235/0x3a0
[<c0106be2>] sys_ipc+0x146/0x263
[<c0102892>] sysenter_past_esp+0xa7/0xb5
[<c0102856>] sysenter_past_esp+0x6b/0xb5
=======================

Here's a bug:

I'm getting tons of this, and X fails to start

CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_PREEMPT_NONE is not set
# CONFIG_PREEMPT_VOLUNTARY is not set
CONFIG_PREEMPT=y
CONFIG_PREEMPT_BKL=y
CONFIG_DEBUG_PREEMPT=y

BUG: sleeping function called from invalid context at kernel/rwsem.c:47

in_atomic():1, irqs_disabled():0

Quoted 10 lines · On Tue, 18 Sep 2007 13:17:28 +0400 Alexey Dobriyan <[email protected]> wrote:

I'm getting tons of this, and X fails to start

CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_PREEMPT_NONE is not set
# CONFIG_PREEMPT_VOLUNTARY is not set
CONFIG_PREEMPT=y
CONFIG_PREEMPT_BKL=y
CONFIG_DEBUG_PREEMPT=y

Quoted 2 lines

BUG: sleeping function called from invalid context at kernel/rwsem.c:47

in_atomic():1, irqs_disabled():0

OK, this fixes the locking here:

ipc/util.c renamed from ipc/util.c~ipc-integrate-ipc_checkid-into-ipc_lock-fix-2 +1 -2

@@ -295,7 +295,6 @@ int ipc_addid(struct ipc_ids* ids, struc
 
 	spin_lock_init(&new->lock);
 	new->deleted = 0;
-	rcu_read_lock();
 	spin_lock(&new->lock);
 	return id;
 }

@@ -691,7 +690,7 @@ struct kern_ipc_perm *ipc_lock(struct ip
 		rcu_read_unlock();
 		return ERR_PTR(-EINVAL);
 	}
-
+	rcu_read_unlock();
 	return out;
 }

Quoted 29 lines · Andrew Morton

Quoted 10 lines · On Tue, 18 Sep 2007 13:17:28 +0400 Alexey Dobriyan <[email protected]> wrote:

I'm getting tons of this, and X fails to start

CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_PREEMPT_NONE is not set
# CONFIG_PREEMPT_VOLUNTARY is not set
CONFIG_PREEMPT=y
CONFIG_PREEMPT_BKL=y
CONFIG_DEBUG_PREEMPT=y

Quoted 9 lines

BUG: sleeping function called from invalid context at kernel/rwsem.c:47
in_atomic():1, irqs_disabled():0
no locks held by X/5879.
[<c012bcb1>] down_write+0x15/0x50
[<c01b53af>] do_shmat+0x235/0x3a0
[<c0106be2>] sys_ipc+0x146/0x263
[<c0102892>] sysenter_past_esp+0xa7/0xb5
[<c0102856>] sysenter_past_esp+0x6b/0xb5
=======================

Here's a bug:

ipc/util.c struct kern_ipc_perm *ipc_lock(struct ip · line 697

Quoted 8 lines

ipc/util.c renamed from ipc/util.c~ipc-integrate-ipc_checkid-into-ipc_lock-fix-2 +1 -1

@@ -691,7 +691,7 @@ struct kern_ipc_perm *ipc_lock(struct ip
 		rcu_read_unlock();
 		return ERR_PTR(-EINVAL);
 	}
-
+	rcu_read_unlock();
 	return out;
 }
 

Andrew,

Actually the rcu_read_lock is released in ipc_unlock().

So I think we
shouldn't add an rcu_read_unlock() before leaving ipc_lock(). This is a part that has not changed since the ref code.

Quoted 10 lines · On Tue, 18 Sep 2007 13:17:28 +0400 Alexey Dobriyan <[email protected]> wrote:

I'm getting tons of this, and X fails to start

CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_PREEMPT_NONE is not set
# CONFIG_PREEMPT_VOLUNTARY is not set
CONFIG_PREEMPT=y
CONFIG_PREEMPT_BKL=y
CONFIG_DEBUG_PREEMPT=y

Quoted 2 lines

BUG: sleeping function called from invalid context at kernel/rwsem.c:47

in_atomic():1, irqs_disabled():0

OK, this fixes the locking here:

ipc/util.c renamed from ipc/util.c~ipc-integrate-ipc_checkid-into-ipc_lock-fix-2 +0 -1

@@ -295,7 +295,6 @@ int ipc_addid(struct ipc_ids* ids, struc
 
 	spin_lock_init(&new->lock);
 	new->deleted = 0;
-	rcu_read_lock();
 	spin_lock(&new->lock);
 	return id;
 }

Quoted 2 lines · On Tue, 18 Sep 2007 03:34:00 -0700 Andrew Morton <[email protected]>…

Well, it was an optimisation. spin_lock() implies rcu_read_lock(). That's a bit dirty and we might choose to not do that.

Not true for the preemptible-rcu work. All such sites should be fixed, or at the very least heavily annotated.

This patch fixes the missing rcu_read_(un)lock in the ipc code

Quoted 3 lines · Peter Zijlstra

On Tue, 18 Sep 2007 03:34:00 -0700 Andrew Morton
<[email protected]> wrote:

Quoted 5 lines

Quoted 2 lines

Well, it was an optimisation. spin_lock() implies rcu_read_lock(). That's a bit dirty and we might choose to not do that.

Not true for the preemptible-rcu work. All such sites should be fixed, or at the very least heavily annotated.

What he said!!!

Quoted 10 lines · On Tue, 18 Sep 2007 09:13:37 -0700 "Paul E. McKenney"…

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · On Tue, 18 Sep 2007 03:34:00 -0700 Andrew Morton <[email protected]>…

Well, it was an optimisation. spin_lock() implies rcu_read_lock(). That's a bit dirty and we might choose to not do that.

Not true for the preemptible-rcu work. All such sites should be fixed, or at the very least heavily annotated.

Quoted 2 lines

What he said!!!

What he said!

How are you going to find all such sites?

Quoted 15 lines · Andrew Morton

Quoted 12 lines · On Tue, 18 Sep 2007 09:13:37 -0700 "Paul E. McKenney"…

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · On Tue, 18 Sep 2007 03:34:00 -0700 Andrew Morton <[email protected]>…

Well, it was an optimisation. spin_lock() implies rcu_read_lock(). That's a bit dirty and we might choose to not do that.

Not true for the preemptible-rcu work. All such sites should be fixed, or at the very least heavily annotated.

What he said!!!

Quoted 3 lines

What he said!

How are you going to find all such sites?

A first step would be to look for patches in -rt that add rcu_read_lock() or friends. A second step would be to look for rcu_dereference() without an enclosing rcu_read_lock(), rcu_read_lock_bh(), or preempt_disable().

warn when rcu_dereference() is used outside of rcu_read_lock()

[ generates a _lot_ of output when booted ]

Signed-off-by Peter Zijlstra <[email protected]>

---
 include/linux/lockdep.h  |    3 ++
 include/linux/rcupdate.h |    5 ++++
 kernel/lockdep.c         |   53 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

Index: linux-2.6/include/linux/lockdep.h
===================================================================

include/linux/lockdep.h +3 -0

@@ -303,6 +303,8 @@ extern void lock_acquire(struct lockdep_
 extern void lock_release(struct lockdep_map *lock, int nested,
 			 unsigned long ip);
 
+extern int lock_is_held(struct lockdep_map *lock);
+
 # define INIT_LOCKDEP				.lockdep_recursion = 0,
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)

@@ -319,6 +321,7 @@ static inline void lockdep_on(void)
 
 # define lock_acquire(l, s, t, r, c, i)		do { } while (0)
 # define lock_release(l, n, i)			do { } while (0)
+# define lock_is_held(l)			(0)
 # define lockdep_init()				do { } while (0)
 # define lockdep_info()				do { } while (0)
 # define lockdep_init_map(lock, name, key, sub)	do { (void)(key); } while (0)

Index: linux-2.6/include/linux/rcupdate.h
===================================================================

include/linux/rcupdate.h +5 -0

@@ -138,9 +138,11 @@ extern int rcu_needs_cpu(int cpu);
 extern struct lockdep_map rcu_lock_map;
 # define rcu_read_acquire()	lock_acquire(&rcu_lock_map, 0, 0, 2, 1, _THIS_IP_)
 # define rcu_read_release()	lock_release(&rcu_lock_map, 1, _THIS_IP_)
+# define rcu_read_held()	WARN_ON_ONCE(!lock_is_held(&rcu_lock_map))
 #else
 # define rcu_read_acquire()	do { } while (0)
 # define rcu_read_release()	do { } while (0)
+# define rcu_read_held()	do { } while (0)
 #endif
 
 /**

@@ -216,6 +218,7 @@ extern struct lockdep_map rcu_lock_map;
 	do { \
 		local_bh_disable(); \
 		__acquire(RCU_BH); \
+		rcu_read_acquire(); \
 	} while(0)
 
 /*

@@ -225,6 +228,7 @@ extern struct lockdep_map rcu_lock_map;
  */
 #define rcu_read_unlock_bh() \
 	do { \
+		rcu_read_release(); \
 		__release(RCU_BH); \
 		local_bh_enable(); \
 	} while(0)

@@ -254,6 +258,7 @@ extern struct lockdep_map rcu_lock_map;
 #define rcu_dereference(p)     ({ \
 				typeof(p) _________p1 = ACCESS_ONCE(p); \
 				smp_read_barrier_depends(); \
+				rcu_read_held(); \
 				(_________p1); \
 				})
 

Index: linux-2.6/kernel/lockdep.c
===================================================================

kernel/lockdep.c +53 -0

@@ -2624,6 +2624,36 @@ static int lock_release_nested(struct ta
 	return 1;
 }
 
+static int __lock_is_held(struct lockdep_map *lock)
+{
+	struct task_struct *curr = current;
+	struct held_lock *hlock, *prev_hlock;
+	unsigned int depth;
+       	int i;
+
+	/*
+	 * Check whether the lock exists in the current stack
+	 * of held locks:
+	 */
+	depth = curr->lockdep_depth;
+	if (DEBUG_LOCKS_WARN_ON(!depth))
+		return 0;
+
+	prev_hlock = NULL;
+	for (i = depth-1; i >= 0; i--) {
+		hlock = curr->held_locks + i;
+		/*
+		 * We must not cross into another context:
+		 */
+		if (prev_hlock && prev_hlock->irq_context != hlock->irq_context)
+			break;
+		if (hlock->instance == lock)
+			return 1;
+		prev_hlock = hlock;
+	}
+	return 0;
+}
+
 /*
  * Remove the lock to the list of currently held locks - this gets
  * called on mutex_unlock()/spin_unlock*() (or on a failed

@@ -2727,6 +2757,29 @@ void lock_release(struct lockdep_map *lo
 
 EXPORT_SYMBOL_GPL(lock_release);
 
+int lock_is_held(struct lockdep_map *lock)
+{
+	int ret = 0;
+	unsigned long flags;
+
+	if (unlikely(!lock_stat && !prove_locking))
+		return 0;
+
+	if (unlikely(current->lockdep_recursion))
+		return -EBUSY;
+
+	raw_local_irq_save(flags);
+	check_flags(flags);
+	current->lockdep_recursion = 1;
+	ret = __lock_is_held(lock);
+	current->lockdep_recursion = 0;
+	raw_local_irq_restore(flags);
+
+	return ret;
+}
+
+EXPORT_SYMBOL_GPL(lock_is_held);
+
 #ifdef CONFIG_LOCK_STAT
 static int
 print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,

Well, reviewing the code I found another place where the rcu_read_unlock() was missing.
I'm so sorry for the inconvenience. It's true that I should have tested with CONFIG_PREEMPT=y :-(
Now, the ltp tests pass even with this option set...

In attachment you'll find a patch thhat
1) adds the missing rcu_read_unlock()
2) replaces Andrew's fix with a new one: the rcu_read_lock() is now taken in ipc_lock() / ipc_lock_by_ptr() and released in ipc_unlock(), exactly as it was done in the ref code.

On 18-09-2007 16:55, Nadia Derbey wrote:
...

Quoted 7 lines

Well, reviewing the code I found another place where the rcu_read_unlock() was missing. I'm so sorry for the inconvenience. It's true that I should have tested with CONFIG_PREEMPT=y :-( Now, the ltp tests pass even with this option set...

In attachment you'll find a patch thhat
1) adds the missing rcu_read_unlock()
2) replaces Andrew's fix with a new one: the rcu_read_lock() is now taken in ipc_lock() / ipc_lock_by_ptr() and released in ipc_unlock(), exactly as it was done in the ref code.

BTW, probably I miss something, but I wonder, how this RCU is working here. E.g. in msg.c do_msgsnd() there is:

msq = msg_lock_check(ns, msqid);
...

msg_unlock(msq);
schedule();

ipc_lock_by_ptr(&msq->q_perm);

Since msq_lock_check() gets msq with ipc_lock_check() under rcu_read_lock(), and then goes msg_unlock(msq) (i.e. ipc_unlock()) with rcu_read_unlock(), is it valid to use this with ipc_lock_by_ptr() yet?

Quoted 25 lines · Jarek Poplawski

On 18-09-2007 16:55, Nadia Derbey wrote:
...

Quoted 11 lines

Well, reviewing the code I found another place where the rcu_read_unlock() was missing.
I'm so sorry for the inconvenience. It's true that I should have tested with CONFIG_PREEMPT=y :-(
Now, the ltp tests pass even with this option set...

In attachment you'll find a patch thhat
1) adds the missing rcu_read_unlock()
2) replaces Andrew's fix with a new one: the rcu_read_lock() is now taken in ipc_lock() / ipc_lock_by_ptr() and released in ipc_unlock(), exactly as it was done in the ref code.

BTW, probably I miss something, but I wonder, how this RCU is working here. E.g. in msg.c do_msgsnd() there is:

msq = msg_lock_check(ns, msqid);
...

msg_unlock(msq);
schedule();

Quoted 6 lines

ipc_lock_by_ptr(&msq->q_perm);

Since msq_lock_check() gets msq with ipc_lock_check() under rcu_read_lock(), and then goes msg_unlock(msq) (i.e. ipc_unlock()) with rcu_read_unlock(), is it valid to use this with ipc_lock_by_ptr() yet?

Before Calling msg_unlock() they call ipc_rcu_getref() that increments a refcount in the rcu header for the msg structure. This guarantees that the the structure won't be freed before they relock it. Once the structure is relocked by ipc_lock_by_ptr(), they do the symmetric operation i.e. ipc_rcu_putref().

On Thu, Sep 20, 2007 at 08:24:58AM +0200, Nadia Derbey wrote:

Quoted 5 lines

Before Calling msg_unlock() they call ipc_rcu_getref() that increments a refcount in the rcu header for the msg structure. This guarantees that the the structure won't be freed before they relock it. Once the structure is relocked by ipc_lock_by_ptr(), they do the symmetric operation i.e. ipc_rcu_putref().

which seems to suggest "out" is an RCU protected pointer, so, I thought these refcounts were for something else. But, after looking at how it's used it turns out to be ~90% wrong: probably 9 out of 10 places use refcouning around this, so, these rcu_read_locks() don't work here at all. So, probably I miss something again, but IMHO, these rcu_read_locks/unlocks could be removed here or in

Quoted 24 lines · Nadia Derbey

Quoted 22 lines · Jarek Poplawski

On 18-09-2007 16:55, Nadia Derbey wrote:
...

Quoted 7 lines

Well, reviewing the code I found another place where the rcu_read_unlock() was missing. I'm so sorry for the inconvenience. It's true that I should have tested with CONFIG_PREEMPT=y :-( Now, the ltp tests pass even with this option set...

In attachment you'll find a patch thhat
1) adds the missing rcu_read_unlock()
2) replaces Andrew's fix with a new one: the rcu_read_lock() is now taken in ipc_lock() / ipc_lock_by_ptr() and released in ipc_unlock(), exactly as it was done in the ref code.

BTW, probably I miss something, but I wonder, how this RCU is working here. E.g. in msg.c do_msgsnd() there is:

msq = msg_lock_check(ns, msqid);
...

msg_unlock(msq);
schedule();

Quoted 8 lines

Quoted 6 lines

ipc_lock_by_ptr(&msq->q_perm);

Since msq_lock_check() gets msq with ipc_lock_check() under rcu_read_lock(), and then goes msg_unlock(msq) (i.e. ipc_unlock()) with rcu_read_unlock(), is it valid to use this with ipc_lock_by_ptr() yet?

Before Calling msg_unlock() they call ipc_rcu_getref() that increments a refcount in the rcu header for the msg structure. This guarantees that the the structure won't be freed before they relock it. Once the structure is relocked by ipc_lock_by_ptr(), they do the symmetric operation i.e. ipc_rcu_putref().

Yes, I've found this later too - sorry for bothering. I was mislead by the code like this:

struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)

{
        struct kern_ipc_perm *out;
        int lid = ipcid_to_idx(id);

        rcu_read_lock();
        out = idr_find(&ids->ipcs_idr, lid);
        if (out == NULL) {
                rcu_read_unlock();
                return ERR_PTR(-EINVAL);
        }

which seems to suggest "out" is an RCU protected pointer, so, I thought these refcounts were for something else. But, after looking at how it's used it turns out to be ~90% wrong: probably 9 out of 10 places use refcouning around this,

so, these rcu_read_locks() don't
work here at all. So, probably I miss something again, but IMHO, these rcu_read_locks/unlocks could be removed here or in ipc_lock_by_ptr() and it should be enough to use them directly, where really needed, e.g., in msg.c do_msgrcv().

Quoted 54 lines · Jarek Poplawski

Quoted 27 lines · Nadia Derbey

Quoted 25 lines · Jarek Poplawski

On 18-09-2007 16:55, Nadia Derbey wrote:
...

Quoted 7 lines

Well, reviewing the code I found another place where the rcu_read_unlock() was missing. I'm so sorry for the inconvenience. It's true that I should have tested with CONFIG_PREEMPT=y :-( Now, the ltp tests pass even with this option set...

In attachment you'll find a patch thhat
1) adds the missing rcu_read_unlock()
2) replaces Andrew's fix with a new one: the rcu_read_lock() is now taken in ipc_lock() / ipc_lock_by_ptr() and released in ipc_unlock(), exactly as it was done in the ref code.

BTW, probably I miss something, but I wonder, how this RCU is working here. E.g. in msg.c do_msgsnd() there is:

msq = msg_lock_check(ns, msqid);
...

msg_unlock(msq);
schedule();

ipc_lock_by_ptr(&msq->q_perm);

Quoted 7 lines

Quoted 4 lines

Since msq_lock_check() gets msq with ipc_lock_check() under rcu_read_lock(), and then goes msg_unlock(msq) (i.e. ipc_unlock()) with rcu_read_unlock(), is it valid to use this with ipc_lock_by_ptr() yet?

Before Calling msg_unlock() they call ipc_rcu_getref() that increments a refcount in the rcu header for the msg structure. This guarantees that the the structure won't be freed before they relock it. Once the structure is relocked by ipc_lock_by_ptr(), they do the symmetric operation i.e. ipc_rcu_putref().

Yes, I've found this later too - sorry for bothering. I was mislead by the code like this:

struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)

{
        struct kern_ipc_perm *out;
        int lid = ipcid_to_idx(id);

        rcu_read_lock();
        out = idr_find(&ids->ipcs_idr, lid);
        if (out == NULL) {
                rcu_read_unlock();
                return ERR_PTR(-EINVAL);
        }

Quoted 4 lines

which seems to suggest "out" is an RCU protected pointer, so, I thought these refcounts were for something else. But, after looking at how it's used it turns out to be ~90% wrong: probably 9 out of 10 places use refcouning around this,

Actually, ipc_lock() is called most of the time without the ipc_ids.mutex held and without refcounting (maybe you didn't look for the msg_lock() sem_lock() and shm_lock() too). So I think disabling preemption is needed, isn't it?

Quoted 6 lines

so, these rcu_read_locks() don't
work here at all. So, probably I miss something again, but IMHO, these rcu_read_locks/unlocks could be removed here or in ipc_lock_by_ptr() and it should be enough to use them directly, where really needed, e.g., in msg.c do_msgrcv().

I have to check for the ipc_lock_by_ptr(): may be you're right!

Jarek Poplawski wrote:

Quoted 4 lines

which seems to suggest "out" is an RCU protected pointer, so, I thought these refcounts were for something else. But, after looking at how it's used it turns out to be ~90% wrong: probably 9 out of 10 places use refcouning around this,

Actually, ipc_lock() is called most of the time without the ipc_ids.mutex held and without refcounting (maybe you didn't look for the msg_lock() sem_lock() and shm_lock() too).

So I think disabling preemption is needed, isn't it?

Quoted 7 lines

so, these rcu_read_locks() don't
work here at all. So, probably I miss something again, but IMHO, these rcu_read_locks/unlocks could be removed here or in ipc_lock_by_ptr() and it should be enough to use them directly, where really needed, e.g., in msg.c do_msgrcv().

I have to check for the ipc_lock_by_ptr(): may be you're right!

So, here is the ipc_lock_by_ptr() status:
1) do_msgsnd(), semctl_main(GETALL), semctl_main(SETALL) and find_undo() call it inside a refcounting.
==> no rcu read section needed.

2) *_exit_ns(), ipc_findkey() and sysvipc_find_ipc() call it under the ipc_ids mutex lock.
==> no rcu read section needed.

3) do_msgrcv() is the only path where ipc_lock_by_ptr() is not called under refcounting
==> rcu read section + some more checks needed once the spnlock is
taken.

So I completely agree with you: we might remove the rcu_read_lock() from the ipc_lock_by_ptr() and explicitley call it when needed (actually, it is already explicitly called in do_msgrcv()).

Quoted 3 lines · Nadia Derbey

Quoted 1 line · Jarek Poplawski

On Thu, Sep 20, 2007 at 08:24:58AM +0200, Nadia Derbey wrote:

Quoted 5 lines

Actually, ipc_lock() is called most of the time without the ipc_ids.mutex held and without refcounting (maybe you didn't look for the msg_lock() sem_lock() and shm_lock() too).
So I think disabling preemption is needed, isn't it?

Quoted 9 lines

Quoted 6 lines

so, these rcu_read_locks() don't
work here at all. So, probably I miss something again, but IMHO, these rcu_read_locks/unlocks could be removed here or in ipc_lock_by_ptr() and it should be enough to use them directly, where really needed, e.g., in msg.c do_msgrcv().

I have to check for the ipc_lock_by_ptr(): may be you're right!

So, here is the ipc_lock_by_ptr() status:
1) do_msgsnd(), semctl_main(GETALL), semctl_main(SETALL) and find_undo() call it inside a refcounting.
==> no rcu read section needed.

2) *_exit_ns(), ipc_findkey() and sysvipc_find_ipc() call it under the ipc_ids mutex lock.
==> no rcu read section needed.

3) do_msgrcv() is the only path where ipc_lock_by_ptr() is not called under refcounting
==> rcu read section + some more checks needed once the spnlock is
taken.

So I completely agree with you: we might remove the rcu_read_lock() from the ipc_lock_by_ptr() and explicitley call it when needed (actually, it is already explicitly called in do_msgrcv()).

Quoted 1 line · On Tue, 18 Sep 2007 16:55:19 +0200 Nadia Derbey <[email protected]> wrote:

This patch fixes the missing rcu_read_(un)lock in the ipc code

Thanks. Could you please check the code comments in ipc/util.c for accuracy and completeness sometime?

Quoted 6 lines · Nadia Derbey

Quoted 4 lines · Nadia Derbey

Quoted 1 line · Jarek Poplawski

On Thu, Sep 20, 2007 at 08:24:58AM +0200, Nadia Derbey wrote:

...

Quoted 18 lines

Quoted 4 lines

Actually, ipc_lock() is called most of the time without the ipc_ids.mutex held and without refcounting (maybe you didn't look for the msg_lock() sem_lock() and shm_lock() too). So I think disabling preemption is needed, isn't it?

Quoted 9 lines

Quoted 6 lines

so, these rcu_read_locks() don't
work here at all. So, probably I miss something again, but IMHO, these rcu_read_locks/unlocks could be removed here or in ipc_lock_by_ptr() and it should be enough to use them directly, where really needed, e.g., in msg.c do_msgrcv().

I have to check for the ipc_lock_by_ptr(): may be you're right!

So, here is the ipc_lock_by_ptr() status:
1) do_msgsnd(), semctl_main(GETALL), semctl_main(SETALL) and find_undo() call it inside a refcounting. ==> no rcu read section needed.

Quoted 8 lines

2) *_exit_ns(), ipc_findkey() and sysvipc_find_ipc() call it under the ipc_ids mutex lock. ==> no rcu read section needed.

3) do_msgrcv() is the only path where ipc_lock_by_ptr() is not called under refcounting ==> rcu read section + some more checks needed once the spnlock is taken.

So I completely agree with you: we might remove the rcu_read_lock() from the ipc_lock_by_ptr() and explicitley call it when needed (actually, it is already explicitly called in do_msgrcv()).

Yes, IMHO, it should be at least more readable when we can see where this RCU is really needed.

But, after 3-rd look, I have a few more doubts (btw., 3 looks are still not enough for me with this code, so I cerainly can miss many things here, and, alas, I manged to see util and msg code only):

1. ipc_lock() and ipc_lock_check() are used without ipc_ids.mutex, but it's probably wrong: they call idr_find() with ipc_ids pointer which needs this mutex, just like in similar code in: ipc_findkey(), ipc_get_maxid() or sysvipc_find_ipc().

2. I'm not sure this refcounting with ipc_rcu_getref/putref is SMP safe (memory barriers): it's not atomic, so locking is needed, but e.g. in do_msgsnd() kern_ipc_perm lock is used for this, while freeque() calls ipc_rcu_putref() with ipc_ids mutex only.

Jarek Poplawski wrote:

Quoted 4 lines

2. I'm not sure this refcounting with ipc_rcu_getref/putref is SMP safe (memory barriers): it's not atomic, so locking is needed, but e.g. in do_msgsnd() kern_ipc_perm lock is used for this, while freeque() calls ipc_rcu_putref() with ipc_ids mutex only.

OK, but freeque() freeary() and shm_destroy() are special cases:
we have the following path:

mutex_lock(ipc_ids.mutex)
...
ipc_lock(ipcp)
... do whatever cleaning is needed ...
ipc_rmid(ipcp)
ipc_unlock(ipcp)
....
ipc_rcu_putref(ipcp)

Once the rmid has been done the ipc structure is considered as not visible anymore from the user side ==> any syscall called with the corresponding id will return invalid.
The only thing that could happen is that this structure be reused for a newly allocated ipc structure. But this too cannot happen since we are under the ipc_ids mutex lock.

Am I wrong?

any of them does ipc_rcu_getref() a bit later and sees old (cached)

I hope not! But, then it would be probably another logical trick:
ipc_rcu_getref/putref() seems to prevent kfreeing of a structure, so if it's used in do_msgsnd() there should be a risk something can do this kfree at this moment, and it seems freeque() is the only one, which both: can do this and cares for this refcount. Then, e.g., if any of them does ipc_rcu_getref() a bit later and sees old (cached) value - kfree can be skipped forever. [...]

After rethinking, this scenario seems to be wrong or very unprobable (I'm not sure of all ways "if (--container...)" could be compiled), so there should be no such risk - double kfree/vfree is more probable, so no danger. More likely is such refcount abuse: ipc_rcu_getref() in do_msgsnd() done a bit after ipc_rcu_putref() in freeque() (msq pointer acquired by do_msgsend() before freeque() started); then, after schedule(), do_msgsnd() can work with kfreed msq_queue structure (at least considering classic RCU).

On Fri, Sep 21, 2007 at 01:03:47PM +0200, Jarek Poplawski wrote:
...

Quoted 7 lines

I hope not! But, then it would be probably another logical trick:
ipc_rcu_getref/putref() seems to prevent kfreeing of a structure, so if it's used in do_msgsnd() there should be a risk something can do this kfree at this moment, and it seems freeque() is the only one, which both: can do this and cares for this refcount. Then, e.g., if any of them does ipc_rcu_getref() a bit later and sees old (cached) value - kfree can be skipped forever. [...]

After rethinking, this scenario seems to be wrong or very unprobable (I'm not sure of all ways "if (--container...)" could be compiled), so there should be no such risk - double kfree/vfree is more probable, so no danger. More likely is such refcount abuse: ipc_rcu_getref() in do_msgsnd() done a bit after ipc_rcu_putref() in freeque() (msq pointer acquired by do_msgsend() before freeque() started); then, after schedule(), do_msgsnd() can work with kfreed msq_queue structure (at least considering classic RCU).

Quoted 6 lines · Nadia Derbey

Quoted 4 lines · Nadia Derbey

Quoted 1 line · Jarek Poplawski

On Thu, Sep 20, 2007 at 08:24:58AM +0200, Nadia Derbey wrote:

...

Quoted 18 lines

Quoted 4 lines

Actually, ipc_lock() is called most of the time without the ipc_ids.mutex held and without refcounting (maybe you didn't look for the msg_lock() sem_lock() and shm_lock() too). So I think disabling preemption is needed, isn't it?

Quoted 9 lines

Quoted 6 lines

so, these rcu_read_locks() don't
work here at all. So, probably I miss something again, but IMHO, these rcu_read_locks/unlocks could be removed here or in ipc_lock_by_ptr() and it should be enough to use them directly, where really needed, e.g., in msg.c do_msgrcv().

I have to check for the ipc_lock_by_ptr(): may be you're right!

So, here is the ipc_lock_by_ptr() status:
1) do_msgsnd(), semctl_main(GETALL), semctl_main(SETALL) and find_undo() call it inside a refcounting. ==> no rcu read section needed.

Quoted 8 lines

2) *_exit_ns(), ipc_findkey() and sysvipc_find_ipc() call it under the ipc_ids mutex lock. ==> no rcu read section needed.

3) do_msgrcv() is the only path where ipc_lock_by_ptr() is not called under refcounting ==> rcu read section + some more checks needed once the spnlock is taken.

So I completely agree with you: we might remove the rcu_read_lock() from the ipc_lock_by_ptr() and explicitley call it when needed (actually, it is already explicitly called in do_msgrcv()).

Yes, IMHO, it should be at least more readable when we can see where this RCU is really needed.

But, after 3-rd look, I have a few more doubts (btw., 3 looks are still not enough for me with this code, so I cerainly can miss many things here, and, alas, I manged to see util and msg code only):

1. ipc_lock() and ipc_lock_check() are used without ipc_ids.mutex, but it's probably wrong: they call idr_find() with ipc_ids pointer which needs this mutex, just like in similar code in: ipc_findkey(), ipc_get_maxid() or sysvipc_find_ipc().

2. I'm not sure this refcounting with ipc_rcu_getref/putref is SMP safe (memory barriers): it's not atomic, so locking is needed, but e.g. in do_msgsnd() kern_ipc_perm lock is used for this, while freeque() calls ipc_rcu_putref() with ipc_ids mutex only.

3. Probably similar problem is possible with msr_d.r_msg which is read in do_msgrcv() under rcu_read_lock() only.

Jarek Poplawski wrote:

Quoted 8 lines

2. I'm not sure this refcounting with ipc_rcu_getref/putref is SMP safe (memory barriers): it's not atomic, so locking is needed, but e.g. in do_msgsnd() kern_ipc_perm lock is used for this, while freeque() calls ipc_rcu_putref() with ipc_ids mutex only.

3. Probably similar problem is possible with msr_d.r_msg which is read in do_msgrcv() under rcu_read_lock() only.

In think here they have avoided refcoutning by using r_msg:
r_msg is initialzed to -EAGAIN before releasing the msq lock. if freequeue() is called it sets r_msg to EIDRM (see expunge_all(-EIDRM)). Setting r_msg is always done under the msq lock (expunge_all() / pipelined_Sned()).
Since rcu_read_lock is called right after schedule, they are sure the
msq pointer is still valid when they re-lock it once a msg is present in the receive queue.

Please tell me if I'm not clear ;-)

Quoted 1 line · Nadia Derbey

Jarek Poplawski wrote:

...

Quoted 18 lines

Quoted 8 lines

2. I'm not sure this refcounting with ipc_rcu_getref/putref is SMP safe (memory barriers): it's not atomic, so locking is needed, but e.g. in do_msgsnd() kern_ipc_perm lock is used for this, while freeque() calls ipc_rcu_putref() with ipc_ids mutex only.

3. Probably similar problem is possible with msr_d.r_msg which is read in do_msgrcv() under rcu_read_lock() only.

In think here they have avoided refcoutning by using r_msg:
r_msg is initialzed to -EAGAIN before releasing the msq lock. if freequeue() is called it sets r_msg to EIDRM (see expunge_all(-EIDRM)). Setting r_msg is always done under the msq lock (expunge_all() / pipelined_Sned()).
Since rcu_read_lock is called right after schedule, they are sure the
msq pointer is still valid when they re-lock it once a msg is present in the receive queue.

Quoted 1 line

Please tell me if I'm not clear ;-)

All clear!

Except... this r_msg is still unclear to me. Since it's read without msq lock I doubt this first check after schedule() is of any value. A
comment: "Lockless receive, part 2" tells about some safety against a race with pipeline_send() and expunge_all() when in wake_up_process(), but how can we be sure this r_msg is not just to be changed and this wake_up_process() is running while "while" check still sees ERR_PTR(-EAGAIN) instead of NULL?