On Thu, Apr 25, 2013 at 12:16 AM, Davidlohr Bueso
<davidlohr.bueso@xxxxxx> wrote:
> From: Davidlohr Bueso <davidlohr.bueso@xxxxxx>
>
> Sedat reported an issue leading to a NULL dereference in update_queue():
>
> [ 178.490583] BUG: spinlock bad magic on CPU#1, sh/8066
> [ 178.490595] lock: 0xffff88008b53ea18, .magic: 6b6b6b6b, .owner: make/8068, .owner_cpu: 3
> [ 178.490599] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 178.490608] IP: [<ffffffff812bacd0>] update_queue+0x70/0x210
> [ 178.490610] PGD 0
> [ 178.490612] Oops: 0000 [#1] SMP
> ...
> [ 178.490704] Call Trace:
> [ 178.490710] [<ffffffff812baf51>] do_smart_update+0xe1/0x140
> [ 178.490713] [<ffffffff812bd6e1>] exit_sem+0x2b1/0x350
> [ 178.490718] [<ffffffff8105de80>] do_exit+0x290/0xa70
> [ 178.490721] [<ffffffff8105e6f4>] do_group_exit+0x44/0xa0
> [ 178.490724] [<ffffffff8105e767>] SyS_exit_group+0x17/0x20
> [ 178.490728] [<ffffffff816ce15d>] system_call_fastpath+0x1a/0x1f
>
> Linus pin-pointed the problem to a race in the reference counter. To quote:
>
> "That dmesg spew very much implies that the same RCU head got added twice to the RCU
> freeing list, and the only way that happens is if the refcount goes to
> zero twice. Which implies that either we increment a zero, or we lack
> locking and the coherency of the non-atomic access goes away."
>
> This patch converts the IPC RCU header's reference counter to atomic_t. The return of
> ipc_rcu_getref() is modified to inform the callers if it actually succeeded.
>
> Now all callers return -EIDRM upon failure and abort the current operation. Two exceptions are
> in semaphore code where sem_getref_and_unlock() and sem_getref() trigger a warning but proceed
> to freeing up any held locks.
>
> Signed-off-by: Davidlohr Bueso <davidlohr.bueso@xxxxxx>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> CC: Rik van Riel <riel@xxxxxxxxxxx>
> CC: Paul McKenney <paul.mckenney@xxxxxxxxxx>
> CC: Sedat Dilek <sedat.dilek@xxxxxxxxx>
> CC: Emmanuel Benisty <benisty.e@xxxxxxxxx>
> CC: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Missing my Reported-by ...!
I have tested this patch instead of Linus' suggested patch against
Linux-Next (next-20130423) with known missing IPC-SEM patches in
-next.
Davidlohr Bueso (2):
ipc, sem: do not call sem_lock when bogus sma
ipc: make refcounter atomic
Rik van Riel (3):
ipc,sem: untangle RCU locking with find_alloc_undo
ipc,sem: fix lockdep false positive
ipc,sem: fix locking in semctl_main
...so please add also my Tested-by!
Thanks to all involved people!
- Sedat -
> ---
> ipc/msg.c | 7 ++++++-
> ipc/sem.c | 18 ++++++++++++------
> ipc/util.c | 46 ++++++++++++++++++++++++----------------------
> ipc/util.h | 2 +-
> 4 files changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 9d11955..2978721 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -668,7 +668,12 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> goto out_unlock_free;
> }
> ss_add(msq, &s);
> - ipc_rcu_getref(msq);
> +
> + if (!ipc_rcu_getref(msq)) {
> + err = -EIDRM;
> + goto out_unlock_free;
> + }
> +
> msg_unlock(msq);
> schedule();
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 92f4d0e..9a71b5a 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -460,7 +460,7 @@ static inline void sem_lock_and_putref(struct sem_array *sma)
>
> static inline void sem_getref_and_unlock(struct sem_array *sma)
> {
> - ipc_rcu_getref(sma);
> + WARN_ON_ONCE(!ipc_rcu_getref(sma));
> sem_unlock(sma, -1);
> }
>
> @@ -476,7 +476,7 @@ static inline void sem_putref(struct sem_array *sma)
> static inline void sem_getref(struct sem_array *sma)
> {
> sem_lock(sma, NULL, -1);
> - ipc_rcu_getref(sma);
> + WARN_ON_ONCE(!ipc_rcu_getref(sma));
> sem_unlock(sma, -1);
> }
>
> @@ -1275,7 +1275,10 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> int i;
> struct sem_undo *un;
>
> - ipc_rcu_getref(sma);
> + if (!ipc_rcu_getref(sma)) {
> + rcu_read_unlock();
> + return -EIDRM;
> + }
> rcu_read_unlock();
>
> if(nsems > SEMMSL_FAST) {
> @@ -1544,8 +1547,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
> struct sem_array *sma;
> struct sem_undo_list *ulp;
> struct sem_undo *un, *new;
> - int nsems;
> - int error;
> + int nsems, error;
>
> error = get_undo_list(&ulp);
> if (error)
> @@ -1567,7 +1569,11 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
> }
>
> nsems = sma->sem_nsems;
> - ipc_rcu_getref(sma);
> + if (!ipc_rcu_getref(sma)) {
> + rcu_read_unlock();
> + un = ERR_PTR(-EIDRM);
> + goto out;
> + }
> rcu_read_unlock();
>
> /* step 2: allocate new undo structure */
> diff --git a/ipc/util.c b/ipc/util.c
> index 18135bc..5e60ebd 100644
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -439,9 +439,9 @@ void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
> * NULL is returned if the allocation fails
> */
>
> -void* ipc_alloc(int size)
> +void *ipc_alloc(int size)
> {
> - void* out;
> + void *out;
> if(size > PAGE_SIZE)
> out = vmalloc(size);
> else
> @@ -478,7 +478,7 @@ void ipc_free(void* ptr, int size)
> */
> struct ipc_rcu_hdr
> {
> - int refcount;
> + atomic_t refcount;
> int is_vmalloc;
> void *data[0];
> };
> @@ -516,39 +516,41 @@ static inline int rcu_use_vmalloc(int size)
> * @size: size desired
> *
> * Allocate memory for the rcu header structure + the object.
> - * Returns the pointer to the object.
> - * NULL is returned if the allocation fails.
> + * Returns the pointer to the object or NULL upon failure.
> */
> -
> -void* ipc_rcu_alloc(int size)
> +void *ipc_rcu_alloc(int size)
> {
> void* out;
> - /*
> +
> + /*
> * We prepend the allocation with the rcu struct, and
> - * workqueue if necessary (for vmalloc).
> + * workqueue if necessary (for vmalloc).
> */
> if (rcu_use_vmalloc(size)) {
> out = vmalloc(HDRLEN_VMALLOC + size);
> - if (out) {
> - out += HDRLEN_VMALLOC;
> - container_of(out, struct ipc_rcu_hdr, data)->is_vmalloc = 1;
> - container_of(out, struct ipc_rcu_hdr, data)->refcount = 1;
> - }
> + if (!out)
> + goto done;
> +
> + out += HDRLEN_VMALLOC;
> + container_of(out, struct ipc_rcu_hdr, data)->is_vmalloc = 1;
> } else {
> out = kmalloc(HDRLEN_KMALLOC + size, GFP_KERNEL);
> - if (out) {
> - out += HDRLEN_KMALLOC;
> - container_of(out, struct ipc_rcu_hdr, data)->is_vmalloc = 0;
> - container_of(out, struct ipc_rcu_hdr, data)->refcount = 1;
> - }
> + if (!out)
> + goto done;
> +
> + out += HDRLEN_KMALLOC;
> + container_of(out, struct ipc_rcu_hdr, data)->is_vmalloc = 0;
> }
>
> + /* set reference counter no matter what kind of allocation was done */
> + atomic_set(&container_of(out, struct ipc_rcu_hdr, data)->refcount, 1);
> +done:
> return out;
> }
>
> -void ipc_rcu_getref(void *ptr)
> +int ipc_rcu_getref(void *ptr)
> {
> - container_of(ptr, struct ipc_rcu_hdr, data)->refcount++;
> + return atomic_inc_not_zero(&container_of(ptr, struct ipc_rcu_hdr, data)->refcount);
> }
>
> static void ipc_do_vfree(struct work_struct *work)
> @@ -578,7 +580,7 @@ static void ipc_schedule_free(struct rcu_head *head)
>
> void ipc_rcu_putref(void *ptr)
> {
> - if (--container_of(ptr, struct ipc_rcu_hdr, data)->refcount > 0)
> + if (!atomic_dec_and_test(&container_of(ptr, struct ipc_rcu_hdr, data)->refcount))
> return;
>
> if (container_of(ptr, struct ipc_rcu_hdr, data)->is_vmalloc) {
> diff --git a/ipc/util.h b/ipc/util.h
> index c36b997..2b0bdd5 100644
> --- a/ipc/util.h
> +++ b/ipc/util.h
> @@ -119,7 +119,7 @@ void ipc_free(void* ptr, int size);
> * to 0 schedules the rcu destruction. Caller must guarantee locking.
> */
> void* ipc_rcu_alloc(int size);
> -void ipc_rcu_getref(void *ptr);
> +int ipc_rcu_getref(void *ptr);
> void ipc_rcu_putref(void *ptr);
>
> struct kern_ipc_perm *ipc_lock(struct ipc_ids *, int);
> --
> 1.7.11.7
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
