Hello Dmitry,
On 12/12/18 11:55 AM, Dmitry Vyukov wrote:
On Tue, Dec 11, 2018 at 9:23 PM syzbot
<syzbot+1145ec2e23165570c...@syzkaller.appspotmail.com> wrote:
Hello,
syzbot found the following crash on:
HEAD commit: f5d582777bcb Merge branch 'for-linus' of git://git.kernel...
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=135bc547400000
kernel config: https://syzkaller.appspot.com/x/.config?x=c8970c89a0efbb23
dashboard link: https://syzkaller.appspot.com/bug?extid=1145ec2e23165570c3ac
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16803afb400000
+Manfred, this looks similar to the other few crashes related to
semget$private(0x0, 0x4000, 0x3f) that you looked at.
I found one unexpected (incorrect?) locking, see the attached patch.
But I doubt that this is the root cause of the crashes.
Any remarks on the patch?
I would continue to search, and then send a series with all findings.
--
Manfred
>From 733e888993b71fb3c139f71de61534bc603a2bcb Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manf...@colorfullife.com>
Date: Wed, 19 Dec 2018 09:26:48 +0100
Subject: [PATCH] ipc/sem.c: ensure proper locking during namespace teardown
free_ipcs() only calls ipc_lock_object() before calling the free callback.
This means:
- There is no exclusion against parallel simple semop() calls.
- sma->use_global_lock may underflow (i.e. jump to UNIT_MAX) when
freeary() calls sem_unlock(,,-1).
The patch fixes that, by adding complexmode_enter() before calling
freeary().
There are multiple syzbot crashes in this code area, but I don't see yet
how a missing complexmode_enter() may cause a crash:
- 1) simple semop() calls are not used by these syzbox tests,
and 2) we are in namespace teardown, noone may run in parallel.
- 1) freeary() is the last call (except parallel operations, which
are impossible due to namespace teardown)
and 2) the underflow of use_global_lock merely delays switching to
parallel simple semop handling for the next UINT_MAX semop() calls.
Thus I think the patch is "only" a cleanup, and does not fix
the observed crashes.
Signed-off-by: Manfred Spraul <manf...@colorfullife.com>
Reported-by: syzbot+1145ec2e23165570c...@syzkaller.appspotmail.com
Reported-by: syzbot+c92d3646e35bc5d1a...@syzkaller.appspotmail.com
Reported-by: syzbot+9d8b6fa6ee7636f35...@syzkaller.appspotmail.com
Cc: dvyu...@google.com
Cc: dbu...@suse.de
Cc: Andrew Morton <a...@linux-foundation.org>
---
ipc/sem.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index 745dc6187e84..8ccacd11fb15 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -184,6 +184,9 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it);
*/
#define USE_GLOBAL_LOCK_HYSTERESIS 10
+static void complexmode_enter(struct sem_array *sma);
+static void complexmode_tryleave(struct sem_array *sma);
+
/*
* Locking:
* a) global sem_lock() for read/write
@@ -232,9 +235,24 @@ void sem_init_ns(struct ipc_namespace *ns)
}
#ifdef CONFIG_IPC_NS
+
+static void freeary_lock(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
+{
+ struct sem_array *sma = container_of(ipcp, struct sem_array, sem_perm);
+
+ /*
+ * free_ipcs() isn't aware of sem_lock(), it calls ipc_lock_object()
+ * directly. In order to stay compatible with sem_lock(), we must
+ * upgrade from "simple" ipc_lock_object() to sem_lock(,,-1).
+ */
+ complexmode_enter(sma);
+
+ freeary(ns, ipcp);
+}
+
void sem_exit_ns(struct ipc_namespace *ns)
{
- free_ipcs(ns, &sem_ids(ns), freeary);
+ free_ipcs(ns, &sem_ids(ns), freeary_lock);
idr_destroy(&ns->ids[IPC_SEM_IDS].ipcs_idr);
rhashtable_destroy(&ns->ids[IPC_SEM_IDS].key_ht);
}
@@ -374,7 +392,9 @@ static inline int sem_lock(struct sem_array *sma, struct sembuf *sops,
/* Complex operation - acquire a full lock */
ipc_lock_object(&sma->sem_perm);
- /* Prevent parallel simple ops */
+ /* Prevent parallel simple ops.
+ * This must be identical to freeary_lock().
+ */
complexmode_enter(sma);
return SEM_GLOBAL_LOCK;
}
--
2.17.2
