* [PATCH 01/12] ipc: reorganize initialization of kern_ipc_perm.id
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-10 22:08 ` Davidlohr Bueso
2018-07-09 15:10 ` [PATCH 02/12] ipc: reorganize initialization of kern_ipc_perm.seq Manfred Spraul
` (11 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul
ipc_addid() initializes kern_ipc_perm.id after having called
ipc_idr_alloc().
Thus a parallel semop() or msgrcv() that uses ipc_obtain_object_idr()
may see an uninitialized value.
The patch moves all accesses to kern_ipc_perm.id under the spin_lock().
The issues is related to the finding of
syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com:
syzbot found an issue with kern_ipc_perm.seq
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
---
ipc/msg.c | 19 ++++++++++++++-----
ipc/sem.c | 18 +++++++++++++-----
ipc/shm.c | 19 ++++++++++++++-----
3 files changed, 41 insertions(+), 15 deletions(-)
diff --git a/ipc/msg.c b/ipc/msg.c
index 3b6545302598..829c2062ded4 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -491,7 +491,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
int cmd, struct msqid64_ds *p)
{
struct msg_queue *msq;
- int id = 0;
int err;
memset(p, 0, sizeof(*p));
@@ -503,7 +502,6 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
err = PTR_ERR(msq);
goto out_unlock;
}
- id = msq->q_perm.id;
} else { /* IPC_STAT */
msq = msq_obtain_object_check(ns, msqid);
if (IS_ERR(msq)) {
@@ -548,10 +546,21 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
p->msg_lspid = pid_vnr(msq->q_lspid);
p->msg_lrpid = pid_vnr(msq->q_lrpid);
- ipc_unlock_object(&msq->q_perm);
- rcu_read_unlock();
- return id;
+ if (cmd == IPC_STAT) {
+ /*
+ * As defined in SUS:
+ * Return 0 on success
+ */
+ err = 0;
+ } else {
+ /*
+ * MSG_STAT and MSG_STAT_ANY (both Linux specific)
+ * Return the full id, including the sequence counter
+ */
+ err = msq->q_perm.id;
+ }
+ ipc_unlock_object(&msq->q_perm);
out_unlock:
rcu_read_unlock();
return err;
diff --git a/ipc/sem.c b/ipc/sem.c
index 5af1943ad782..e8971fa1d847 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1222,7 +1222,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
{
struct sem_array *sma;
time64_t semotime;
- int id = 0;
int err;
memset(semid64, 0, sizeof(*semid64));
@@ -1234,7 +1233,6 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
err = PTR_ERR(sma);
goto out_unlock;
}
- id = sma->sem_perm.id;
} else { /* IPC_STAT */
sma = sem_obtain_object_check(ns, semid);
if (IS_ERR(sma)) {
@@ -1274,10 +1272,20 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
#endif
semid64->sem_nsems = sma->sem_nsems;
+ if (cmd == IPC_STAT) {
+ /*
+ * As defined in SUS:
+ * Return 0 on success
+ */
+ err = 0;
+ } else {
+ /*
+ * SEM_STAT and SEM_STAT_ANY (both Linux specific)
+ * Return the full id, including the sequence counter
+ */
+ err = sma->sem_perm.id;
+ }
ipc_unlock_object(&sma->sem_perm);
- rcu_read_unlock();
- return id;
-
out_unlock:
rcu_read_unlock();
return err;
diff --git a/ipc/shm.c b/ipc/shm.c
index 051a3e1fb8df..59fe8b3b3794 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -949,7 +949,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
int cmd, struct shmid64_ds *tbuf)
{
struct shmid_kernel *shp;
- int id = 0;
int err;
memset(tbuf, 0, sizeof(*tbuf));
@@ -961,7 +960,6 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
err = PTR_ERR(shp);
goto out_unlock;
}
- id = shp->shm_perm.id;
} else { /* IPC_STAT */
shp = shm_obtain_object_check(ns, shmid);
if (IS_ERR(shp)) {
@@ -1011,10 +1009,21 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
tbuf->shm_lpid = pid_vnr(shp->shm_lprid);
tbuf->shm_nattch = shp->shm_nattch;
- ipc_unlock_object(&shp->shm_perm);
- rcu_read_unlock();
- return id;
+ if (cmd == IPC_STAT) {
+ /*
+ * As defined in SUS:
+ * Return 0 on success
+ */
+ err = 0;
+ } else {
+ /*
+ * SHM_STAT and SHM_STAT_ANY (both Linux specific)
+ * Return the full id, including the sequence counter
+ */
+ err = shp->shm_perm.id;
+ }
+ ipc_unlock_object(&shp->shm_perm);
out_unlock:
rcu_read_unlock();
return err;
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 01/12] ipc: reorganize initialization of kern_ipc_perm.id
2018-07-09 15:10 ` [PATCH 01/12] ipc: reorganize initialization of kern_ipc_perm.id Manfred Spraul
@ 2018-07-10 22:08 ` Davidlohr Bueso
0 siblings, 0 replies; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-10 22:08 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
On Mon, 09 Jul 2018, Manfred Spraul wrote:
>ipc_addid() initializes kern_ipc_perm.id after having called
>ipc_idr_alloc().
>
>Thus a parallel semop() or msgrcv() that uses ipc_obtain_object_idr()
>may see an uninitialized value.
>
>The patch moves all accesses to kern_ipc_perm.id under the spin_lock().
Yeah this is a lot better than my kzalloc() idea. I was considering
completions to avoid these races with ->getnew(), but careful init beats
that idea for obvious reasons.
The only thing I would say is that the title should be more descriptive.
Unlike the next patch, this one isn't really reorganizing anything.
How about:
ipc: compute kern_ipc_perm.id under the ipc lock for stat cmds
>
>The issues is related to the finding of
>syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com:
>syzbot found an issue with kern_ipc_perm.seq
>
>Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
>Cc: Dmitry Vyukov <dvyukov@google.com>
>Cc: Kees Cook <keescook@chromium.org>
>Cc: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Davidlohr Bueso <dbueso@suse.de>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 02/12] ipc: reorganize initialization of kern_ipc_perm.seq
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
2018-07-09 15:10 ` [PATCH 01/12] ipc: reorganize initialization of kern_ipc_perm.id Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 15:10 ` [PATCH 03/12] ipc/util.c: Use ipc_rcu_putref() for failues in ipc_addid() Manfred Spraul
` (10 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul, Michael Kerrisk
ipc_addid() initializes kern_ipc_perm.seq after having called
ipc_idr_alloc().
Thus a parallel semop() or msgrcv() that uses ipc_obtain_object_check()
may see an uninitialized value.
The patch moves the initialization of kern_ipc_perm.seq before the
calls of ipc_idr_alloc().
Notes:
1) This patch has a user space visible side effect:
If /proc/sys/kernel/*_next_id is used (i.e.: checkpoint/restore) and
if semget()/msgget()/shmget() fails in the final step of adding the id
to the rhash tree, then .._next_id is cleared. Before the patch, is
remained unmodified.
There is no change of the behavior after a successful ..get() call:
It always clears .._next_id, there is no impact to non checkpoint/restore
code as that code does not use .._next_id.
2) The patch correctly documents that after a call to ipc_idr_alloc(),
the full tear-down sequence must be used. The callers of ipc_addid()
do not fullfill that, i.e. more bugfixes are required.
Reported-by: syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
---
Documentation/sysctl/kernel.txt | 3 ++-
ipc/util.c | 45 +++++++++++++++++++++++----------
2 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index eded671d55eb..b2d4a8f8fe97 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -440,7 +440,8 @@ Notes:
1) kernel doesn't guarantee, that new object will have desired id. So,
it's up to userspace, how to handle an object with "wrong" id.
2) Toggle with non-default value will be set back to -1 by kernel after
-successful IPC object allocation.
+successful IPC object allocation. If an IPC object allocation syscall
+fails, it is undefined if the value remains unmodified or is reset to -1.
==============================================================
diff --git a/ipc/util.c b/ipc/util.c
index 4e81182fa0ac..662c28c6c9fa 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -197,13 +197,24 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
/*
* Specify desired id for next allocated IPC object.
*/
-#define ipc_idr_alloc(ids, new) \
- idr_alloc(&(ids)->ipcs_idr, (new), \
- (ids)->next_id < 0 ? 0 : ipcid_to_idx((ids)->next_id),\
- 0, GFP_NOWAIT)
+static inline int ipc_idr_alloc(struct ipc_ids *ids,
+ struct kern_ipc_perm *new)
+{
+ int key;
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
- struct kern_ipc_perm *new)
+ if (ids->next_id < 0) {
+ key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
+ } else {
+ key = idr_alloc(&ids->ipcs_idr, new,
+ ipcid_to_idx(ids->next_id),
+ 0, GFP_NOWAIT);
+ ids->next_id = -1;
+ }
+ return key;
+}
+
+static inline void ipc_set_seq(struct ipc_ids *ids,
+ struct kern_ipc_perm *new)
{
if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */
new->seq = ids->seq++;
@@ -211,24 +222,19 @@ static inline int ipc_buildid(int id, struct ipc_ids *ids,
ids->seq = 0;
} else {
new->seq = ipcid_to_seqx(ids->next_id);
- ids->next_id = -1;
}
-
- return SEQ_MULTIPLIER * new->seq + id;
}
#else
#define ipc_idr_alloc(ids, new) \
idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT)
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
+static inline void ipc_set_seq(struct ipc_ids *ids,
struct kern_ipc_perm *new)
{
new->seq = ids->seq++;
if (ids->seq > IPCID_SEQ_MAX)
ids->seq = 0;
-
- return SEQ_MULTIPLIER * new->seq + id;
}
#endif /* CONFIG_CHECKPOINT_RESTORE */
@@ -270,6 +276,19 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
new->cuid = new->uid = euid;
new->gid = new->cgid = egid;
+ ipc_set_seq(ids, new);
+
+ /*
+ * As soon as a new object is inserted into the idr,
+ * ipc_obtain_object_idr() or ipc_obtain_object_check() can find it,
+ * and the lockless preparations for ipc operations can start.
+ * This means especially: permission checks, audit calls, allocation
+ * of undo structures, ...
+ *
+ * Thus the object must be fully initialized, and if something fails,
+ * then the full tear-down sequence must be followed.
+ * (i.e.: set new->deleted, reduce refcount, call_rcu())
+ */
id = ipc_idr_alloc(ids, new);
idr_preload_end();
@@ -291,7 +310,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
if (id > ids->max_id)
ids->max_id = id;
- new->id = ipc_buildid(id, ids, new);
+ new->id = SEQ_MULTIPLIER * new->seq + id;
return id;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 03/12] ipc/util.c: Use ipc_rcu_putref() for failues in ipc_addid()
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
2018-07-09 15:10 ` [PATCH 01/12] ipc: reorganize initialization of kern_ipc_perm.id Manfred Spraul
2018-07-09 15:10 ` [PATCH 02/12] ipc: reorganize initialization of kern_ipc_perm.seq Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 15:10 ` [PATCH 04/12] ipc: Rename ipcctl_pre_down_nolock() Manfred Spraul
` (9 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul
ipc_addid() is impossible to use:
- for certain failures, the caller must not use ipc_rcu_putref(),
because the reference counter is not yet initialized.
- for other failures, the caller must use ipc_rcu_putref(),
because parallel operations could be ongoing already.
The patch cleans that up, by initializing the refcount early,
and by modifying all callers.
The issues is related to the finding of
syzbot+2827ef6b3385deb07eaf@syzkaller.appspotmail.com:
syzbot found an issue with reading kern_ipc_perm.seq,
here both read and write to already released memory could happen.
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
---
ipc/msg.c | 2 +-
ipc/sem.c | 2 +-
ipc/shm.c | 2 ++
ipc/util.c | 12 ++++++++++--
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/ipc/msg.c b/ipc/msg.c
index 829c2062ded4..5bf5cb8017ea 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -162,7 +162,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
/* ipc_addid() locks msq upon success. */
retval = ipc_addid(&msg_ids(ns), &msq->q_perm, ns->msg_ctlmni);
if (retval < 0) {
- call_rcu(&msq->q_perm.rcu, msg_rcu_free);
+ ipc_rcu_putref(&msq->q_perm, msg_rcu_free);
return retval;
}
diff --git a/ipc/sem.c b/ipc/sem.c
index e8971fa1d847..9d49efeac2e5 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -556,7 +556,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
/* ipc_addid() locks sma upon success. */
retval = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);
if (retval < 0) {
- call_rcu(&sma->sem_perm.rcu, sem_rcu_free);
+ ipc_rcu_putref(&sma->sem_perm, sem_rcu_free);
return retval;
}
ns->used_sems += nsems;
diff --git a/ipc/shm.c b/ipc/shm.c
index 59fe8b3b3794..06b7bf11a011 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -671,6 +671,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
if (is_file_hugepages(file) && shp->mlock_user)
user_shm_unlock(size, shp->mlock_user);
fput(file);
+ ipc_rcu_putref(&shp->shm_perm, shm_rcu_free);
+ return error;
no_file:
call_rcu(&shp->shm_perm.rcu, shm_rcu_free);
return error;
diff --git a/ipc/util.c b/ipc/util.c
index 662c28c6c9fa..8b09496ed720 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -248,7 +248,9 @@ static inline void ipc_set_seq(struct ipc_ids *ids,
* Add an entry 'new' to the ipc ids idr. The permissions object is
* initialised and the first free entry is set up and the id assigned
* is returned. The 'new' entry is returned in a locked state on success.
+ *
* On failure the entry is not locked and a negative err-code is returned.
+ * The caller must use ipc_rcu_putref() to free the identifier.
*
* Called with writer ipc_ids.rwsem held.
*/
@@ -258,6 +260,9 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
kgid_t egid;
int id, err;
+ /* 1) Initialize the refcount so that ipc_rcu_putref works */
+ refcount_set(&new->refcount, 1);
+
if (limit > IPCMNI)
limit = IPCMNI;
@@ -266,9 +271,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
idr_preload(GFP_KERNEL);
- refcount_set(&new->refcount, 1);
spin_lock_init(&new->lock);
- new->deleted = false;
rcu_read_lock();
spin_lock(&new->lock);
@@ -277,6 +280,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
new->gid = new->cgid = egid;
ipc_set_seq(ids, new);
+ new->deleted = false;
/*
* As soon as a new object is inserted into the idr,
@@ -288,6 +292,9 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
* Thus the object must be fully initialized, and if something fails,
* then the full tear-down sequence must be followed.
* (i.e.: set new->deleted, reduce refcount, call_rcu())
+ *
+ * This function sets new->deleted, the caller must use ipc_rcu_putef()
+ * for the remaining steps.
*/
id = ipc_idr_alloc(ids, new);
idr_preload_end();
@@ -301,6 +308,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
}
}
if (id < 0) {
+ new->deleted = true;
spin_unlock(&new->lock);
rcu_read_unlock();
return id;
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 04/12] ipc: Rename ipcctl_pre_down_nolock().
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (2 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 03/12] ipc/util.c: Use ipc_rcu_putref() for failues in ipc_addid() Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 19:49 ` Davidlohr Bueso
2018-07-09 15:10 ` [PATCH 05/12] ipc/util.c: correct comment in ipc_obtain_object_check Manfred Spraul
` (8 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul
Both the comment and the name of ipcctl_pre_down_nolock()
are misleading: The function must be called while holdling
the rw semaphore.
Therefore the patch renames the function to ipcctl_obtain_check():
This name matches the other names used in util.c:
- "obtain" function look up a pointer in the idr, without
acquiring the object lock.
- The caller is responsible for locking.
- _check means that the sequence number is checked.
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
---
ipc/msg.c | 2 +-
ipc/sem.c | 2 +-
ipc/shm.c | 2 +-
ipc/util.c | 8 ++++----
ipc/util.h | 2 +-
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/ipc/msg.c b/ipc/msg.c
index 5bf5cb8017ea..ba85d8849e8d 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -385,7 +385,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
down_write(&msg_ids(ns).rwsem);
rcu_read_lock();
- ipcp = ipcctl_pre_down_nolock(ns, &msg_ids(ns), msqid, cmd,
+ ipcp = ipcctl_obtain_check(ns, &msg_ids(ns), msqid, cmd,
&msqid64->msg_perm, msqid64->msg_qbytes);
if (IS_ERR(ipcp)) {
err = PTR_ERR(ipcp);
diff --git a/ipc/sem.c b/ipc/sem.c
index 9d49efeac2e5..9742e9a1c0c2 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1595,7 +1595,7 @@ static int semctl_down(struct ipc_namespace *ns, int semid,
down_write(&sem_ids(ns).rwsem);
rcu_read_lock();
- ipcp = ipcctl_pre_down_nolock(ns, &sem_ids(ns), semid, cmd,
+ ipcp = ipcctl_obtain_check(ns, &sem_ids(ns), semid, cmd,
&semid64->sem_perm, 0);
if (IS_ERR(ipcp)) {
err = PTR_ERR(ipcp);
diff --git a/ipc/shm.c b/ipc/shm.c
index 06b7bf11a011..426ba1039a7b 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -868,7 +868,7 @@ static int shmctl_down(struct ipc_namespace *ns, int shmid, int cmd,
down_write(&shm_ids(ns).rwsem);
rcu_read_lock();
- ipcp = ipcctl_pre_down_nolock(ns, &shm_ids(ns), shmid, cmd,
+ ipcp = ipcctl_obtain_check(ns, &shm_ids(ns), shmid, cmd,
&shmid64->shm_perm, 0);
if (IS_ERR(ipcp)) {
err = PTR_ERR(ipcp);
diff --git a/ipc/util.c b/ipc/util.c
index 8b09496ed720..bbb1ce212a0d 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -703,7 +703,7 @@ int ipc_update_perm(struct ipc64_perm *in, struct kern_ipc_perm *out)
}
/**
- * ipcctl_pre_down_nolock - retrieve an ipc and check permissions for some IPC_XXX cmd
+ * ipcctl_obtain_check - retrieve an ipc object and check permissions
* @ns: ipc namespace
* @ids: the table of ids where to look for the ipc
* @id: the id of the ipc to retrieve
@@ -713,16 +713,16 @@ int ipc_update_perm(struct ipc64_perm *in, struct kern_ipc_perm *out)
*
* This function does some common audit and permissions check for some IPC_XXX
* cmd and is called from semctl_down, shmctl_down and msgctl_down.
- * It must be called without any lock held and:
*
- * - retrieves the ipc with the given id in the given table.
+ * It:
+ * - retrieves the ipc object with the given id in the given table.
* - performs some audit and permission check, depending on the given cmd
* - returns a pointer to the ipc object or otherwise, the corresponding
* error.
*
* Call holding the both the rwsem and the rcu read lock.
*/
-struct kern_ipc_perm *ipcctl_pre_down_nolock(struct ipc_namespace *ns,
+struct kern_ipc_perm *ipcctl_obtain_check(struct ipc_namespace *ns,
struct ipc_ids *ids, int id, int cmd,
struct ipc64_perm *perm, int extra_perm)
{
diff --git a/ipc/util.h b/ipc/util.h
index 0aba3230d007..fcf81425ae98 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -148,7 +148,7 @@ struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id);
void kernel_to_ipc64_perm(struct kern_ipc_perm *in, struct ipc64_perm *out);
void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out);
int ipc_update_perm(struct ipc64_perm *in, struct kern_ipc_perm *out);
-struct kern_ipc_perm *ipcctl_pre_down_nolock(struct ipc_namespace *ns,
+struct kern_ipc_perm *ipcctl_obtain_check(struct ipc_namespace *ns,
struct ipc_ids *ids, int id, int cmd,
struct ipc64_perm *perm, int extra_perm);
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] ipc: Rename ipcctl_pre_down_nolock().
2018-07-09 15:10 ` [PATCH 04/12] ipc: Rename ipcctl_pre_down_nolock() Manfred Spraul
@ 2018-07-09 19:49 ` Davidlohr Bueso
0 siblings, 0 replies; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-09 19:49 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
On Mon, 09 Jul 2018, Manfred Spraul wrote:
>Both the comment and the name of ipcctl_pre_down_nolock()
>are misleading: The function must be called while holdling
>the rw semaphore.
>Therefore the patch renames the function to ipcctl_obtain_check():
>This name matches the other names used in util.c:
>- "obtain" function look up a pointer in the idr, without
> acquiring the object lock.
>- The caller is responsible for locking.
>- _check means that the sequence number is checked.
>
>Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
>Cc: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Davidlohr Bueso <dbueso@suse.de>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 05/12] ipc/util.c: correct comment in ipc_obtain_object_check
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (3 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 04/12] ipc: Rename ipcctl_pre_down_nolock() Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 18:46 ` Davidlohr Bueso
2018-07-09 15:10 ` [PATCH 06/12] ipc: rename ipc_lock() to ipc_lock_idr() Manfred Spraul
` (7 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul
The comment that explains ipc_obtain_object_check is wrong:
The function checks the sequence number, not the reference
counter.
Note that checking the reference counter would be meaningless:
The reference counter is decreased without holding any locks,
thus an object with kern_ipc_perm.deleted=true may disappear at
the end of the next rcu grace period.
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
---
ipc/util.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipc/util.c b/ipc/util.c
index bbb1ce212a0d..8133f10832a9 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -644,8 +644,8 @@ struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)
* @ids: ipc identifier set
* @id: ipc id to look for
*
- * Similar to ipc_obtain_object_idr() but also checks
- * the ipc object reference counter.
+ * Similar to ipc_obtain_object_idr() but also checks the ipc object
+ * sequence number.
*
* Call inside the RCU critical section.
* The ipc object is *not* locked on exit.
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 05/12] ipc/util.c: correct comment in ipc_obtain_object_check
2018-07-09 15:10 ` [PATCH 05/12] ipc/util.c: correct comment in ipc_obtain_object_check Manfred Spraul
@ 2018-07-09 18:46 ` Davidlohr Bueso
0 siblings, 0 replies; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-09 18:46 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
On Mon, 09 Jul 2018, Manfred Spraul wrote:
>The comment that explains ipc_obtain_object_check is wrong:
>The function checks the sequence number, not the reference
>counter.
>Note that checking the reference counter would be meaningless:
>The reference counter is decreased without holding any locks,
>thus an object with kern_ipc_perm.deleted=true may disappear at
>the end of the next rcu grace period.
>
>Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
>Cc: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Davidlohr Bueso <dbueso@suse.de>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 06/12] ipc: rename ipc_lock() to ipc_lock_idr()
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (4 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 05/12] ipc/util.c: correct comment in ipc_obtain_object_check Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-11 19:38 ` Davidlohr Bueso
2018-07-09 15:10 ` [PATCH 07/12] ipc_idr_alloc refactoring Manfred Spraul
` (6 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul
ipc/util.c contains multiple functions to get the ipc object
pointer given an id number.
There are two sets of function: One set verifies the sequence
counter part of the id number, other functions do not check
the sequence counter.
The standard for function names in ipc/util.c is
- ..._check() functions verify the sequence counter
- ..._idr() functions do not verify the sequence counter
ipc_lock() is an exception: It does not verify the sequence
counter value, but this is not obvious from the function name.
Therefore: Rename the function to ipc_lock_idr(), to make it
obvious that it does not check the sequence counter.
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
---
ipc/shm.c | 4 ++--
ipc/util.c | 10 ++++++----
ipc/util.h | 2 +-
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/ipc/shm.c b/ipc/shm.c
index 426ba1039a7b..cd8655c7bb77 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -179,11 +179,11 @@ static inline struct shmid_kernel *shm_obtain_object_check(struct ipc_namespace
*/
static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
{
- struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
+ struct kern_ipc_perm *ipcp = ipc_lock_idr(&shm_ids(ns), id);
/*
* Callers of shm_lock() must validate the status of the returned ipc
- * object pointer (as returned by ipc_lock()), and error out as
+ * object pointer (as returned by ipc_lock_idr()), and error out as
* appropriate.
*/
if (IS_ERR(ipcp))
diff --git a/ipc/util.c b/ipc/util.c
index 8133f10832a9..8bc166bb4981 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -604,15 +604,17 @@ struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id)
}
/**
- * ipc_lock - lock an ipc structure without rwsem held
+ * ipc_lock_idr - lock an ipc structure without rwsem held
* @ids: ipc identifier set
* @id: ipc id to look for
*
* Look for an id in the ipc ids idr and lock the associated ipc object.
+ * The function does not check if the sequence counter matches the
+ * found ipc object.
*
* The ipc object is locked on successful exit.
*/
-struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)
+struct kern_ipc_perm *ipc_lock_idr(struct ipc_ids *ids, int id)
{
struct kern_ipc_perm *out;
@@ -624,8 +626,8 @@ struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)
spin_lock(&out->lock);
/*
- * ipc_rmid() may have already freed the ID while ipc_lock()
- * was spinning: here verify that the structure is still valid.
+ * ipc_rmid() may have already freed the ID while waiting for
+ * the lock. Here verify that the structure is still valid.
* Upon races with RMID, return -EIDRM, thus indicating that
* the ID points to a removed identifier.
*/
diff --git a/ipc/util.h b/ipc/util.h
index fcf81425ae98..25d8ee052ac9 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -142,7 +142,7 @@ int ipc_rcu_getref(struct kern_ipc_perm *ptr);
void ipc_rcu_putref(struct kern_ipc_perm *ptr,
void (*func)(struct rcu_head *head));
-struct kern_ipc_perm *ipc_lock(struct ipc_ids *, int);
+struct kern_ipc_perm *ipc_lock_idr(struct ipc_ids *ids, int id);
struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id);
void kernel_to_ipc64_perm(struct kern_ipc_perm *in, struct ipc64_perm *out);
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 06/12] ipc: rename ipc_lock() to ipc_lock_idr()
2018-07-09 15:10 ` [PATCH 06/12] ipc: rename ipc_lock() to ipc_lock_idr() Manfred Spraul
@ 2018-07-11 19:38 ` Davidlohr Bueso
0 siblings, 0 replies; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-11 19:38 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
On Mon, 09 Jul 2018, Manfred Spraul wrote:
>ipc/util.c contains multiple functions to get the ipc object
>pointer given an id number.
>
>There are two sets of function: One set verifies the sequence
>counter part of the id number, other functions do not check
>the sequence counter.
>
>The standard for function names in ipc/util.c is
>- ..._check() functions verify the sequence counter
>- ..._idr() functions do not verify the sequence counter
>
>ipc_lock() is an exception: It does not verify the sequence
>counter value, but this is not obvious from the function name.
Agreed.
>
>Therefore: Rename the function to ipc_lock_idr(), to make it
>obvious that it does not check the sequence counter.
The rename doesn't make it more obvious either I don't think. Since
the only user of ipc_lock() is shm, how about we just move the
logic in there. Sure, we'd now have an 'ipc_unlock()' counter part,
but that's just ipc_unlock_object + rcu unlock, so leaving it as is
wouldn't be the end of the world methinks. Unlike the ipc_lock(),
the unlocking is also used by util.c.
Thanks.
----8<----------------------------------------------------------------
[PATCH] ipc: drop ipc_lock()
ipc/util.c contains multiple functions to get the ipc object
pointer given an id number.
There are two sets of function: One set verifies the sequence
counter part of the id number, other functions do not check
the sequence counter.
The standard for function names in ipc/util.c is
- ..._check() functions verify the sequence counter
- ..._idr() functions do not verify the sequence counter
ipc_lock() is an exception: It does not verify the sequence
counter value, but this is not obvious from the function name.
Furthermore, shm.c is the only user of this helper. Thus, we
can simply move the logic into shm_lock() and get rid of the
function altogether.
[changelog mostly by manfred]
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
ipc/shm.c | 29 +++++++++++++++++++++++------
ipc/util.c | 36 ------------------------------------
ipc/util.h | 1 -
3 files changed, 23 insertions(+), 43 deletions(-)
diff --git a/ipc/shm.c b/ipc/shm.c
index 051a3e1fb8df..f3b19bce349d 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -179,16 +179,33 @@ static inline struct shmid_kernel *shm_obtain_object_check(struct ipc_namespace
*/
static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
{
- struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
+ struct kern_ipc_perm *ipcp;
+
+ rcu_read_lock();
+ ipcp = ipc_obtain_object_idr(&shm_ids(ns), id);
+ if (IS_ERR(ipcp))
+ goto err;
+ ipc_lock_object(ipcp);
+ /*
+ * ipc_rmid() may have already freed the ID while ipc_lock_object()
+ * was spinning: here verify that the structure is still valid.
+ * Upon races with RMID, return -EIDRM, thus indicating that
+ * the ID points to a removed identifier.
+ */
+ if (ipc_valid_object(ipcp)) {
+ /* return a locked ipc object upon success */
+ return container_of(ipcp, struct shmid_kernel, shm_perm);
+ }
+
+ ipc_unlock_object(ipcp);
+err:
+ rcu_read_unlock();
/*
* Callers of shm_lock() must validate the status of the returned ipc
- * object pointer (as returned by ipc_lock()), and error out as
- * appropriate.
+ * object pointer and error out as appropriate.
*/
- if (IS_ERR(ipcp))
- return (void *)ipcp;
- return container_of(ipcp, struct shmid_kernel, shm_perm);
+ return (void *)ipcp;
}
static inline void shm_lock_by_ptr(struct shmid_kernel *ipcp)
diff --git a/ipc/util.c b/ipc/util.c
index 4e81182fa0ac..fe61559cd64b 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -576,42 +576,6 @@ struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id)
return out;
}
-/**
- * ipc_lock - lock an ipc structure without rwsem held
- * @ids: ipc identifier set
- * @id: ipc id to look for
- *
- * Look for an id in the ipc ids idr and lock the associated ipc object.
- *
- * The ipc object is locked on successful exit.
- */
-struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)
-{
- struct kern_ipc_perm *out;
-
- rcu_read_lock();
- out = ipc_obtain_object_idr(ids, id);
- if (IS_ERR(out))
- goto err;
-
- spin_lock(&out->lock);
-
- /*
- * ipc_rmid() may have already freed the ID while ipc_lock()
- * was spinning: here verify that the structure is still valid.
- * Upon races with RMID, return -EIDRM, thus indicating that
- * the ID points to a removed identifier.
- */
- if (ipc_valid_object(out))
- return out;
-
- spin_unlock(&out->lock);
- out = ERR_PTR(-EIDRM);
-err:
- rcu_read_unlock();
- return out;
-}
-
/**
* ipc_obtain_object_check
* @ids: ipc identifier set
diff --git a/ipc/util.h b/ipc/util.h
index 0aba3230d007..cdb9259e05d1 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -142,7 +142,6 @@ int ipc_rcu_getref(struct kern_ipc_perm *ptr);
void ipc_rcu_putref(struct kern_ipc_perm *ptr,
void (*func)(struct rcu_head *head));
-struct kern_ipc_perm *ipc_lock(struct ipc_ids *, int);
struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id);
void kernel_to_ipc64_perm(struct kern_ipc_perm *in, struct ipc64_perm *out);
--
2.16.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 07/12] ipc_idr_alloc refactoring
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (5 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 06/12] ipc: rename ipc_lock() to ipc_lock_idr() Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-11 19:52 ` Davidlohr Bueso
2018-07-09 15:10 ` [PATCH 08/12] lib/rhashtable: simplify bucket_table_alloc() Manfred Spraul
` (5 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul
From: Dmitry Vyukov <dvyukov@google.com>
ipc_idr_alloc refactoring
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
ipc/util.c | 51 +++++++++++++--------------------------------------
1 file changed, 13 insertions(+), 38 deletions(-)
diff --git a/ipc/util.c b/ipc/util.c
index 8bc166bb4981..a41b8a69de13 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -193,52 +193,32 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
return NULL;
}
-#ifdef CONFIG_CHECKPOINT_RESTORE
/*
* Specify desired id for next allocated IPC object.
*/
-static inline int ipc_idr_alloc(struct ipc_ids *ids,
- struct kern_ipc_perm *new)
+static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
{
- int key;
+ int key, next_id = -1;
- if (ids->next_id < 0) {
- key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
- } else {
- key = idr_alloc(&ids->ipcs_idr, new,
- ipcid_to_idx(ids->next_id),
- 0, GFP_NOWAIT);
- ids->next_id = -1;
- }
- return key;
-}
+#ifdef CONFIG_CHECKPOINT_RESTORE
+ next_id = ids->next_id;
+ ids->next_id = -1;
+#endif
-static inline void ipc_set_seq(struct ipc_ids *ids,
- struct kern_ipc_perm *new)
-{
- if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */
+ if (next_id < 0) { /* !CHECKPOINT_RESTORE or next_id is unset */
new->seq = ids->seq++;
if (ids->seq > IPCID_SEQ_MAX)
ids->seq = 0;
+ key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
} else {
- new->seq = ipcid_to_seqx(ids->next_id);
+ new->seq = ipcid_to_seqx(next_id);
+ key = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
+ 0, GFP_NOWAIT);
}
+ new->id = SEQ_MULTIPLIER * new->seq + key;
+ return key;
}
-#else
-#define ipc_idr_alloc(ids, new) \
- idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT)
-
-static inline void ipc_set_seq(struct ipc_ids *ids,
- struct kern_ipc_perm *new)
-{
- new->seq = ids->seq++;
- if (ids->seq > IPCID_SEQ_MAX)
- ids->seq = 0;
-}
-
-#endif /* CONFIG_CHECKPOINT_RESTORE */
-
/**
* ipc_addid - add an ipc identifier
* @ids: ipc identifier set
@@ -278,8 +258,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
current_euid_egid(&euid, &egid);
new->cuid = new->uid = euid;
new->gid = new->cgid = egid;
-
- ipc_set_seq(ids, new);
new->deleted = false;
/*
@@ -317,9 +295,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
ids->in_use++;
if (id > ids->max_id)
ids->max_id = id;
-
- new->id = SEQ_MULTIPLIER * new->seq + id;
-
return id;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 07/12] ipc_idr_alloc refactoring
2018-07-09 15:10 ` [PATCH 07/12] ipc_idr_alloc refactoring Manfred Spraul
@ 2018-07-11 19:52 ` Davidlohr Bueso
2018-07-11 20:00 ` Dmitry Vyukov
0 siblings, 1 reply; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-11 19:52 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
On Mon, 09 Jul 2018, Manfred Spraul wrote:
>From: Dmitry Vyukov <dvyukov@google.com>
>
>ipc_idr_alloc refactoring
ENOCHANGELOG
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 07/12] ipc_idr_alloc refactoring
2018-07-11 19:52 ` Davidlohr Bueso
@ 2018-07-11 20:00 ` Dmitry Vyukov
0 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2018-07-11 20:00 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: Manfred Spraul, Andrew Morton, LKML, 1vier1, Kees Cook
On Wed, Jul 11, 2018 at 9:52 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Mon, 09 Jul 2018, Manfred Spraul wrote:
>
>> From: Dmitry Vyukov <dvyukov@google.com>
>>
>> ipc_idr_alloc refactoring
>
> ENOCHANGELOG
Manfred, could you please squash this into your commit? This was meant
to be a code review comment rather than a full-fledged patch.
As far as I remember, there are now 3 patches that can be squashed:
your original change, my refactoring on top, and your fix to my
refactoring.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 08/12] lib/rhashtable: simplify bucket_table_alloc()
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (6 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 07/12] ipc_idr_alloc refactoring Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 15:10 ` [PATCH 09/12] lib/rhashtable: guarantee initial hashtable allocation Manfred Spraul
` (4 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Davidlohr Bueso, Manfred Spraul
From: Davidlohr Bueso <dave@stgolabs.net>
As of commit ce91f6ee5b3b ("mm: kvmalloc does not fallback to vmalloc for
incompatible gfp flags") we can simplify the caller and trust kvzalloc() to
just do the right thing. For the case of the GFP_ATOMIC context, we can
drop the __GFP_NORETRY flag for obvious reasons, and for the __GFP_NOWARN
case, however, it is changed such that the caller passes the flag instead
of making bucket_table_alloc() handle it.
This slightly changes the gfp flags passed on to nested_table_alloc() as
it will now also use GFP_ATOMIC | __GFP_NOWARN. However, I consider this a
positive consequence as for the same reasons we want nowarn semantics in
bucket_table_alloc().
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Acked-by: Michal Hocko <mhocko@suse.com>
(commit id extended to 12 digits, line wraps updated)
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
lib/rhashtable.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 9427b5766134..083f871491a1 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -175,10 +175,7 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
int i;
size = sizeof(*tbl) + nbuckets * sizeof(tbl->buckets[0]);
- if (gfp != GFP_KERNEL)
- tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
- else
- tbl = kvzalloc(size, gfp);
+ tbl = kvzalloc(size, gfp);
size = nbuckets;
@@ -459,7 +456,7 @@ static int rhashtable_insert_rehash(struct rhashtable *ht,
err = -ENOMEM;
- new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC);
+ new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC | __GFP_NOWARN);
if (new_tbl == NULL)
goto fail;
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 09/12] lib/rhashtable: guarantee initial hashtable allocation
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (7 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 08/12] lib/rhashtable: simplify bucket_table_alloc() Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 15:10 ` [PATCH 10/12] ipc: get rid of ids->tables_initialized hack Manfred Spraul
` (3 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Davidlohr Bueso, Manfred Spraul
From: Davidlohr Bueso <dave@stgolabs.net>
rhashtable_init() may fail due to -ENOMEM, thus making the
entire api unusable. This patch removes this scenario,
however unlikely. In order to guarantee memory allocation,
this patch always ends up doing GFP_KERNEL|__GFP_NOFAIL
for both the tbl as well as alloc_bucket_spinlocks().
Upon the first table allocation failure, we shrink the
size to the smallest value that makes sense and retry with
__GFP_NOFAIL semantics. With the defaults, this means that
from 64 buckets, we retry with only 4. Any later issues
regarding performance due to collisions or larger table
resizing (when more memory becomes available) is the least
of our problems.
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
lib/rhashtable.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 083f871491a1..0026cf3e3f27 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -179,10 +179,11 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
size = nbuckets;
- if (tbl == NULL && gfp != GFP_KERNEL) {
+ if (tbl == NULL && (gfp & ~__GFP_NOFAIL) != GFP_KERNEL) {
tbl = nested_bucket_table_alloc(ht, nbuckets, gfp);
nbuckets = 0;
}
+
if (tbl == NULL)
return NULL;
@@ -1065,9 +1066,16 @@ int rhashtable_init(struct rhashtable *ht,
}
}
+ /*
+ * This is api initialization and thus we need to guarantee the
+ * initial rhashtable allocation. Upon failure, retry with the
+ * smallest possible size with __GFP_NOFAIL semantics.
+ */
tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
- if (tbl == NULL)
- return -ENOMEM;
+ if (unlikely(tbl == NULL)) {
+ size = max_t(u16, ht->p.min_size, HASH_MIN_SIZE);
+ tbl = bucket_table_alloc(ht, size, GFP_KERNEL | __GFP_NOFAIL);
+ }
atomic_set(&ht->nelems, 0);
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 10/12] ipc: get rid of ids->tables_initialized hack
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (8 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 09/12] lib/rhashtable: guarantee initial hashtable allocation Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 15:10 ` [PATCH 11/12] ipc: simplify ipc initialization Manfred Spraul
` (2 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Davidlohr Bueso, Manfred Spraul
From: Davidlohr Bueso <dave@stgolabs.net>
In sysvipc we have an ids->tables_initialized regarding the
rhashtable, introduced in:
commit 0cfb6aee70bd ("ipc: optimize semget/shmget/msgget for lots of keys")
It's there, specifically, to prevent nil pointer dereferences,
from using an uninitialized api. Considering how rhashtable_init()
can fail (probably due to ENOMEM, if anything), this made the
overall ipc initialization capable of failure as well. That alone
is ugly, but fine, however I've spotted a few issues regarding the
semantics of tables_initialized (however unlikely they may be):
- There is inconsistency in what we return to userspace: ipc_addid()
returns ENOSPC which is certainly _wrong_, while ipc_obtain_object_idr()
returns EINVAL.
- After we started using rhashtables, ipc_findkey() can return nil upon
!tables_initialized, but the caller expects nil for when the ipc structure
isn't found, and can therefore call into ipcget() callbacks.
Now that rhashtable initialization cannot fail, we can properly
get rid of the hack altogether.
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
(commit id extended to 12 digits)
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
include/linux/ipc_namespace.h | 1 -
ipc/util.c | 23 ++++++++---------------
2 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index b5630c8eb2f3..37f3a4b7c637 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -16,7 +16,6 @@ struct user_namespace;
struct ipc_ids {
int in_use;
unsigned short seq;
- bool tables_initialized;
struct rw_semaphore rwsem;
struct idr ipcs_idr;
int max_id;
diff --git a/ipc/util.c b/ipc/util.c
index a41b8a69de13..ae485b41ea0b 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -125,7 +125,6 @@ int ipc_init_ids(struct ipc_ids *ids)
if (err)
return err;
idr_init(&ids->ipcs_idr);
- ids->tables_initialized = true;
ids->max_id = -1;
#ifdef CONFIG_CHECKPOINT_RESTORE
ids->next_id = -1;
@@ -178,19 +177,16 @@ void __init ipc_init_proc_interface(const char *path, const char *header,
*/
static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
{
- struct kern_ipc_perm *ipcp = NULL;
+ struct kern_ipc_perm *ipcp;
- if (likely(ids->tables_initialized))
- ipcp = rhashtable_lookup_fast(&ids->key_ht, &key,
+ ipcp = rhashtable_lookup_fast(&ids->key_ht, &key,
ipc_kht_params);
+ if (!ipcp)
+ return NULL;
- if (ipcp) {
- rcu_read_lock();
- ipc_lock_object(ipcp);
- return ipcp;
- }
-
- return NULL;
+ rcu_read_lock();
+ ipc_lock_object(ipcp);
+ return ipcp;
}
/*
@@ -246,7 +242,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
if (limit > IPCMNI)
limit = IPCMNI;
- if (!ids->tables_initialized || ids->in_use >= limit)
+ if (ids->in_use >= limit)
return -ENOSPC;
idr_preload(GFP_KERNEL);
@@ -568,9 +564,6 @@ struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id)
struct kern_ipc_perm *out;
int lid = ipcid_to_idx(id);
- if (unlikely(!ids->tables_initialized))
- return ERR_PTR(-EINVAL);
-
out = idr_find(&ids->ipcs_idr, lid);
if (!out)
return ERR_PTR(-EINVAL);
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 11/12] ipc: simplify ipc initialization
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (9 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 10/12] ipc: get rid of ids->tables_initialized hack Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 21:42 ` Andrew Morton
2018-07-09 15:10 ` [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups Manfred Spraul
2018-07-09 20:09 ` [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Davidlohr Bueso
12 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Davidlohr Bueso, Manfred Spraul
From: Davidlohr Bueso <dave@stgolabs.net>
Now that we know that rhashtable_init() will not fail, we
can get rid of a lot of the unnecessary cleanup paths when
the call errored out.
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
(variable name added to util.h to resolve checkpatch warning)
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
ipc/msg.c | 9 ++++-----
ipc/namespace.c | 20 ++++----------------
ipc/sem.c | 10 ++++------
ipc/shm.c | 9 ++++-----
ipc/util.c | 18 +++++-------------
ipc/util.h | 18 +++++++++---------
6 files changed, 30 insertions(+), 54 deletions(-)
diff --git a/ipc/msg.c b/ipc/msg.c
index ba85d8849e8d..346230712259 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -1237,7 +1237,7 @@ COMPAT_SYSCALL_DEFINE5(msgrcv, int, msqid, compat_uptr_t, msgp,
}
#endif
-int msg_init_ns(struct ipc_namespace *ns)
+void msg_init_ns(struct ipc_namespace *ns)
{
ns->msg_ctlmax = MSGMAX;
ns->msg_ctlmnb = MSGMNB;
@@ -1245,7 +1245,7 @@ int msg_init_ns(struct ipc_namespace *ns)
atomic_set(&ns->msg_bytes, 0);
atomic_set(&ns->msg_hdrs, 0);
- return ipc_init_ids(&ns->ids[IPC_MSG_IDS]);
+ ipc_init_ids(&ns->ids[IPC_MSG_IDS]);
}
#ifdef CONFIG_IPC_NS
@@ -1286,12 +1286,11 @@ static int sysvipc_msg_proc_show(struct seq_file *s, void *it)
}
#endif
-int __init msg_init(void)
+void __init msg_init(void)
{
- const int err = msg_init_ns(&init_ipc_ns);
+ msg_init_ns(&init_ipc_ns);
ipc_init_proc_interface("sysvipc/msg",
" key msqid perms cbytes qnum lspid lrpid uid gid cuid cgid stime rtime ctime\n",
IPC_MSG_IDS, sysvipc_msg_proc_show);
- return err;
}
diff --git a/ipc/namespace.c b/ipc/namespace.c
index f59a89966f92..21607791d62c 100644
--- a/ipc/namespace.c
+++ b/ipc/namespace.c
@@ -55,28 +55,16 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
ns->user_ns = get_user_ns(user_ns);
ns->ucounts = ucounts;
- err = sem_init_ns(ns);
+ err = mq_init_ns(ns);
if (err)
goto fail_put;
- err = msg_init_ns(ns);
- if (err)
- goto fail_destroy_sem;
- err = shm_init_ns(ns);
- if (err)
- goto fail_destroy_msg;
- err = mq_init_ns(ns);
- if (err)
- goto fail_destroy_shm;
+ sem_init_ns(ns);
+ msg_init_ns(ns);
+ shm_init_ns(ns);
return ns;
-fail_destroy_shm:
- shm_exit_ns(ns);
-fail_destroy_msg:
- msg_exit_ns(ns);
-fail_destroy_sem:
- sem_exit_ns(ns);
fail_put:
put_user_ns(ns->user_ns);
ns_free_inum(&ns->ns);
diff --git a/ipc/sem.c b/ipc/sem.c
index 9742e9a1c0c2..f3de2f5e7b9b 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -220,14 +220,14 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it);
#define sc_semopm sem_ctls[2]
#define sc_semmni sem_ctls[3]
-int sem_init_ns(struct ipc_namespace *ns)
+void sem_init_ns(struct ipc_namespace *ns)
{
ns->sc_semmsl = SEMMSL;
ns->sc_semmns = SEMMNS;
ns->sc_semopm = SEMOPM;
ns->sc_semmni = SEMMNI;
ns->used_sems = 0;
- return ipc_init_ids(&ns->ids[IPC_SEM_IDS]);
+ ipc_init_ids(&ns->ids[IPC_SEM_IDS]);
}
#ifdef CONFIG_IPC_NS
@@ -239,14 +239,12 @@ void sem_exit_ns(struct ipc_namespace *ns)
}
#endif
-int __init sem_init(void)
+void __init sem_init(void)
{
- const int err = sem_init_ns(&init_ipc_ns);
-
+ sem_init_ns(&init_ipc_ns);
ipc_init_proc_interface("sysvipc/sem",
" key semid perms nsems uid gid cuid cgid otime ctime\n",
IPC_SEM_IDS, sysvipc_sem_proc_show);
- return err;
}
/**
diff --git a/ipc/shm.c b/ipc/shm.c
index cd8655c7bb77..1db4cf91f676 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -95,14 +95,14 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp);
static int sysvipc_shm_proc_show(struct seq_file *s, void *it);
#endif
-int shm_init_ns(struct ipc_namespace *ns)
+void shm_init_ns(struct ipc_namespace *ns)
{
ns->shm_ctlmax = SHMMAX;
ns->shm_ctlall = SHMALL;
ns->shm_ctlmni = SHMMNI;
ns->shm_rmid_forced = 0;
ns->shm_tot = 0;
- return ipc_init_ids(&shm_ids(ns));
+ ipc_init_ids(&shm_ids(ns));
}
/*
@@ -135,9 +135,8 @@ void shm_exit_ns(struct ipc_namespace *ns)
static int __init ipc_ns_init(void)
{
- const int err = shm_init_ns(&init_ipc_ns);
- WARN(err, "ipc: sysv shm_init_ns failed: %d\n", err);
- return err;
+ shm_init_ns(&init_ipc_ns);
+ return 0;
}
pure_initcall(ipc_ns_init);
diff --git a/ipc/util.c b/ipc/util.c
index ae485b41ea0b..d474f2b3b299 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -87,16 +87,12 @@ struct ipc_proc_iface {
*/
static int __init ipc_init(void)
{
- int err_sem, err_msg;
-
proc_mkdir("sysvipc", NULL);
- err_sem = sem_init();
- WARN(err_sem, "ipc: sysv sem_init failed: %d\n", err_sem);
- err_msg = msg_init();
- WARN(err_msg, "ipc: sysv msg_init failed: %d\n", err_msg);
+ sem_init();
+ msg_init();
shm_init();
- return err_msg ? err_msg : err_sem;
+ return 0;
}
device_initcall(ipc_init);
@@ -115,21 +111,17 @@ static const struct rhashtable_params ipc_kht_params = {
* Set up the sequence range to use for the ipc identifier range (limited
* below IPCMNI) then initialise the keys hashtable and ids idr.
*/
-int ipc_init_ids(struct ipc_ids *ids)
+void ipc_init_ids(struct ipc_ids *ids)
{
- int err;
ids->in_use = 0;
ids->seq = 0;
init_rwsem(&ids->rwsem);
- err = rhashtable_init(&ids->key_ht, &ipc_kht_params);
- if (err)
- return err;
+ rhashtable_init(&ids->key_ht, &ipc_kht_params);
idr_init(&ids->ipcs_idr);
ids->max_id = -1;
#ifdef CONFIG_CHECKPOINT_RESTORE
ids->next_id = -1;
#endif
- return 0;
}
#ifdef CONFIG_PROC_FS
diff --git a/ipc/util.h b/ipc/util.h
index 25d8ee052ac9..9a8a40de3682 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -18,8 +18,8 @@
#define IPCMNI 32768 /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
#define SEQ_MULTIPLIER (IPCMNI)
-int sem_init(void);
-int msg_init(void);
+void sem_init(void);
+void msg_init(void);
void shm_init(void);
struct ipc_namespace;
@@ -34,17 +34,17 @@ static inline void mq_put_mnt(struct ipc_namespace *ns) { }
#endif
#ifdef CONFIG_SYSVIPC
-int sem_init_ns(struct ipc_namespace *ns);
-int msg_init_ns(struct ipc_namespace *ns);
-int shm_init_ns(struct ipc_namespace *ns);
+void sem_init_ns(struct ipc_namespace *ns);
+void msg_init_ns(struct ipc_namespace *ns);
+void shm_init_ns(struct ipc_namespace *ns);
void sem_exit_ns(struct ipc_namespace *ns);
void msg_exit_ns(struct ipc_namespace *ns);
void shm_exit_ns(struct ipc_namespace *ns);
#else
-static inline int sem_init_ns(struct ipc_namespace *ns) { return 0; }
-static inline int msg_init_ns(struct ipc_namespace *ns) { return 0; }
-static inline int shm_init_ns(struct ipc_namespace *ns) { return 0; }
+static inline void sem_init_ns(struct ipc_namespace *ns) { }
+static inline void msg_init_ns(struct ipc_namespace *ns) { }
+static inline void shm_init_ns(struct ipc_namespace *ns) { }
static inline void sem_exit_ns(struct ipc_namespace *ns) { }
static inline void msg_exit_ns(struct ipc_namespace *ns) { }
@@ -83,7 +83,7 @@ struct ipc_ops {
struct seq_file;
struct ipc_ids;
-int ipc_init_ids(struct ipc_ids *);
+void ipc_init_ids(struct ipc_ids *ids);
#ifdef CONFIG_PROC_FS
void __init ipc_init_proc_interface(const char *path, const char *header,
int ids, int (*show)(struct seq_file *, void *));
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 11/12] ipc: simplify ipc initialization
2018-07-09 15:10 ` [PATCH 11/12] ipc: simplify ipc initialization Manfred Spraul
@ 2018-07-09 21:42 ` Andrew Morton
2018-07-09 23:55 ` Davidlohr Bueso
0 siblings, 1 reply; 27+ messages in thread
From: Andrew Morton @ 2018-07-09 21:42 UTC (permalink / raw)
To: Manfred Spraul
Cc: Davidlohr Bueso, Dmitry Vyukov, LKML, 1vier1, Kees Cook, Davidlohr Bueso
On Mon, 9 Jul 2018 17:10:18 +0200 Manfred Spraul <manfred@colorfullife.com> wrote:
> From: Davidlohr Bueso <dave@stgolabs.net>
>
> ...
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Should these be From: dbueso@suse.de?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 11/12] ipc: simplify ipc initialization
2018-07-09 21:42 ` Andrew Morton
@ 2018-07-09 23:55 ` Davidlohr Bueso
0 siblings, 0 replies; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-09 23:55 UTC (permalink / raw)
Cc: Manfred Spraul, Dmitry Vyukov, LKML, 1vier1, Kees Cook, Davidlohr Bueso
On Mon, 09 Jul 2018, Andrew Morton wrote:
>On Mon, 9 Jul 2018 17:10:18 +0200 Manfred Spraul <manfred@colorfullife.com> wrote:
>
>> From: Davidlohr Bueso <dave@stgolabs.net>
>>
>> ...
>>
>> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
>
>Should these be From: dbueso@suse.de?
Not really, I've been doing this for years now -- makes backports
easier.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups.
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (10 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 11/12] ipc: simplify ipc initialization Manfred Spraul
@ 2018-07-09 15:10 ` Manfred Spraul
2018-07-09 17:05 ` Dmitry Vyukov
2018-07-09 20:09 ` [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Davidlohr Bueso
12 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 15:10 UTC (permalink / raw)
To: Andrew Morton, Davidlohr Bueso, Dmitry Vyukov
Cc: LKML, 1vier1, Kees Cook, Manfred Spraul
If idr_alloc within ipc_idr_alloc fails, then the return value (-ENOSPC)
is used to calculate new->id.
Technically, this is not a bug, because new->id is never accessed.
But: Clean it up anyways: On error, just return, do not set new->id.
And improve the documentation.
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
ipc/util.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/ipc/util.c b/ipc/util.c
index d474f2b3b299..302c18fc846b 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -182,11 +182,20 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
}
/*
- * Specify desired id for next allocated IPC object.
+ * Insert new IPC object into idr tree, and set sequence number and id
+ * in the correct order.
+ * Especially:
+ * - the sequence number must be set before inserting the object into the idr,
+ * because the sequence number is accessed without a lock.
+ * - the id can/must be set after inserting the object into the idr.
+ * All accesses must be done after getting kern_ipc_perm.lock.
+ *
+ * The caller must own kern_ipc_perm.lock.of the new object.
+ * On error, the function returns a (negative) error code.
*/
static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
{
- int key, next_id = -1;
+ int id, next_id = -1;
#ifdef CONFIG_CHECKPOINT_RESTORE
next_id = ids->next_id;
@@ -197,14 +206,15 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
new->seq = ids->seq++;
if (ids->seq > IPCID_SEQ_MAX)
ids->seq = 0;
- key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
+ id = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
} else {
new->seq = ipcid_to_seqx(next_id);
- key = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
+ id = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
0, GFP_NOWAIT);
}
- new->id = SEQ_MULTIPLIER * new->seq + key;
- return key;
+ if (id >= 0)
+ new->id = SEQ_MULTIPLIER * new->seq + id;
+ return id;
}
/**
--
2.17.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups.
2018-07-09 15:10 ` [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups Manfred Spraul
@ 2018-07-09 17:05 ` Dmitry Vyukov
2018-07-09 18:22 ` Manfred Spraul
0 siblings, 1 reply; 27+ messages in thread
From: Dmitry Vyukov @ 2018-07-09 17:05 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Davidlohr Bueso, LKML, 1vier1, Kees Cook
On Mon, Jul 9, 2018 at 5:10 PM, Manfred Spraul <manfred@colorfullife.com> wrote:
> If idr_alloc within ipc_idr_alloc fails, then the return value (-ENOSPC)
> is used to calculate new->id.
> Technically, this is not a bug, because new->id is never accessed.
>
> But: Clean it up anyways: On error, just return, do not set new->id.
> And improve the documentation.
>
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
> ipc/util.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/ipc/util.c b/ipc/util.c
> index d474f2b3b299..302c18fc846b 100644
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -182,11 +182,20 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
> }
>
> /*
> - * Specify desired id for next allocated IPC object.
> + * Insert new IPC object into idr tree, and set sequence number and id
> + * in the correct order.
> + * Especially:
> + * - the sequence number must be set before inserting the object into the idr,
> + * because the sequence number is accessed without a lock.
> + * - the id can/must be set after inserting the object into the idr.
> + * All accesses must be done after getting kern_ipc_perm.lock.
> + *
> + * The caller must own kern_ipc_perm.lock.of the new object.
> + * On error, the function returns a (negative) error code.
> */
> static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
> {
> - int key, next_id = -1;
> + int id, next_id = -1;
/\/\/\/\
Looks good to me. I was also confused by how key transforms into id,
and then key name is used for something else.
> #ifdef CONFIG_CHECKPOINT_RESTORE
> next_id = ids->next_id;
> @@ -197,14 +206,15 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
> new->seq = ids->seq++;
> if (ids->seq > IPCID_SEQ_MAX)
> ids->seq = 0;
> - key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
> + id = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
> } else {
> new->seq = ipcid_to_seqx(next_id);
> - key = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
> + id = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
> 0, GFP_NOWAIT);
> }
> - new->id = SEQ_MULTIPLIER * new->seq + key;
> - return key;
> + if (id >= 0)
> + new->id = SEQ_MULTIPLIER * new->seq + id;
We still initialize seq in this case. I guess it's ok because the
object is not published at all. But if we are doing this, then perhaps
store seq into a local var first and then:
if (id >= 0) {
new->id = SEQ_MULTIPLIER * seq + id;
new->seq = seq:
}
?
But I don't have a strong preference, so if it's the only reason to
resend series then perhaps it's not worth it.
Thanks
> + return id;
> }
>
> /**
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups.
2018-07-09 17:05 ` Dmitry Vyukov
@ 2018-07-09 18:22 ` Manfred Spraul
2018-07-09 18:31 ` Dmitry Vyukov
0 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-09 18:22 UTC (permalink / raw)
To: Dmitry Vyukov; +Cc: Andrew Morton, Davidlohr Bueso, LKML, 1vier1, Kees Cook
Hello Dmitry,
On 07/09/2018 07:05 PM, Dmitry Vyukov wrote:
> On Mon, Jul 9, 2018 at 5:10 PM, Manfred Spraul <manfred@colorfullife.com> wrote:
>> If idr_alloc within ipc_idr_alloc fails, then the return value (-ENOSPC)
>> is used to calculate new->id.
>> Technically, this is not a bug, because new->id is never accessed.
>>
>> But: Clean it up anyways: On error, just return, do not set new->id.
>> And improve the documentation.
>>
>> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
>> Cc: Dmitry Vyukov <dvyukov@google.com>
>> ---
>> ipc/util.c | 22 ++++++++++++++++------
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/ipc/util.c b/ipc/util.c
>> index d474f2b3b299..302c18fc846b 100644
>> --- a/ipc/util.c
>> +++ b/ipc/util.c
>> @@ -182,11 +182,20 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
>> }
>>
>> /*
>> - * Specify desired id for next allocated IPC object.
>> + * Insert new IPC object into idr tree, and set sequence number and id
>> + * in the correct order.
>> + * Especially:
>> + * - the sequence number must be set before inserting the object into the idr,
>> + * because the sequence number is accessed without a lock.
>> + * - the id can/must be set after inserting the object into the idr.
>> + * All accesses must be done after getting kern_ipc_perm.lock.
>> + *
>> + * The caller must own kern_ipc_perm.lock.of the new object.
>> + * On error, the function returns a (negative) error code.
>> */
>> static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
>> {
>> - int key, next_id = -1;
>> + int id, next_id = -1;
> /\/\/\/\
> Looks good to me. I was also confused by how key transforms into id,
> and then key name is used for something else.
Let's see if there are further findings, perhaps I'll rework the series,
it may make sense to standardize the variable names:
id: user space id. Called semid, shmid, msgid if the type is known.
Most functions use "id" already.
Exception: ipc_checkid(), the function calls is uid.
idx: "index" for the idr lookup
Right now, ipc_rmid() use lid, ipc_addid() use id as variable name
seq: sequence counter, to avoid quick collisions of the user space id
In the comments, it got a mixture of sequence counter and sequence
number.
key: user space key, used for the rhash tree
>> #ifdef CONFIG_CHECKPOINT_RESTORE
>> next_id = ids->next_id;
>> @@ -197,14 +206,15 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
>> new->seq = ids->seq++;
>> if (ids->seq > IPCID_SEQ_MAX)
>> ids->seq = 0;
>> - key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>> + id = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>> } else {
>> new->seq = ipcid_to_seqx(next_id);
>> - key = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
>> + id = idr_alloc(&ids->ipcs_idr, new, ipcid_to_idx(next_id),
>> 0, GFP_NOWAIT);
>> }
>> - new->id = SEQ_MULTIPLIER * new->seq + key;
>> - return key;
>> + if (id >= 0)
>> + new->id = SEQ_MULTIPLIER * new->seq + id;
> We still initialize seq in this case. I guess it's ok because the
> object is not published at all. But if we are doing this, then perhaps
> store seq into a local var first and then:
>
> if (id >= 0) {
> new->id = SEQ_MULTIPLIER * seq + id;
> new->seq = seq:
> }
>
> ?
No!!!
We must initialize ->seq before publication. Otherwise we end up with
the syzcall findings, or in the worst case a strange rare failure of an
ipc operation.
The difference between ->id and ->seq is that we have the valid number
for ->seq.
For the user space ID we cannot have the valid number unless the
idr_alloc is successful.
The patch only avoids that this line is executed:
> new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)
As I wrote, the line shouldn't cause any damage, the code is more or less:
> new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)
> kfree(new);
But this is ugly, it asks for problems.
--
Manfred
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups.
2018-07-09 18:22 ` Manfred Spraul
@ 2018-07-09 18:31 ` Dmitry Vyukov
0 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2018-07-09 18:31 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Davidlohr Bueso, LKML, 1vier1, Kees Cook
On Mon, Jul 9, 2018 at 8:22 PM, Manfred Spraul <manfred@colorfullife.com> wrote:
> Hello Dmitry,
>
>
> On 07/09/2018 07:05 PM, Dmitry Vyukov wrote:
>>
>> On Mon, Jul 9, 2018 at 5:10 PM, Manfred Spraul <manfred@colorfullife.com>
>> wrote:
>>>
>>> If idr_alloc within ipc_idr_alloc fails, then the return value (-ENOSPC)
>>> is used to calculate new->id.
>>> Technically, this is not a bug, because new->id is never accessed.
>>>
>>> But: Clean it up anyways: On error, just return, do not set new->id.
>>> And improve the documentation.
>>>
>>> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
>>> Cc: Dmitry Vyukov <dvyukov@google.com>
>>> ---
>>> ipc/util.c | 22 ++++++++++++++++------
>>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/ipc/util.c b/ipc/util.c
>>> index d474f2b3b299..302c18fc846b 100644
>>> --- a/ipc/util.c
>>> +++ b/ipc/util.c
>>> @@ -182,11 +182,20 @@ static struct kern_ipc_perm *ipc_findkey(struct
>>> ipc_ids *ids, key_t key)
>>> }
>>>
>>> /*
>>> - * Specify desired id for next allocated IPC object.
>>> + * Insert new IPC object into idr tree, and set sequence number and id
>>> + * in the correct order.
>>> + * Especially:
>>> + * - the sequence number must be set before inserting the object into
>>> the idr,
>>> + * because the sequence number is accessed without a lock.
>>> + * - the id can/must be set after inserting the object into the idr.
>>> + * All accesses must be done after getting kern_ipc_perm.lock.
>>> + *
>>> + * The caller must own kern_ipc_perm.lock.of the new object.
>>> + * On error, the function returns a (negative) error code.
>>> */
>>> static inline int ipc_idr_alloc(struct ipc_ids *ids, struct
>>> kern_ipc_perm *new)
>>> {
>>> - int key, next_id = -1;
>>> + int id, next_id = -1;
>>
>> /\/\/\/\
>> Looks good to me. I was also confused by how key transforms into id,
>> and then key name is used for something else.
>
> Let's see if there are further findings, perhaps I'll rework the series, it
> may make sense to standardize the variable names:
>
> id: user space id. Called semid, shmid, msgid if the type is known.
> Most functions use "id" already.
> Exception: ipc_checkid(), the function calls is uid.
> idx: "index" for the idr lookup
> Right now, ipc_rmid() use lid, ipc_addid() use id as variable name
> seq: sequence counter, to avoid quick collisions of the user space id
> In the comments, it got a mixture of sequence counter and sequence
> number.
> key: user space key, used for the rhash tree
>
>>> #ifdef CONFIG_CHECKPOINT_RESTORE
>>> next_id = ids->next_id;
>>> @@ -197,14 +206,15 @@ static inline int ipc_idr_alloc(struct ipc_ids
>>> *ids, struct kern_ipc_perm *new)
>>> new->seq = ids->seq++;
>>> if (ids->seq > IPCID_SEQ_MAX)
>>> ids->seq = 0;
>>> - key = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>>> + id = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>>> } else {
>>> new->seq = ipcid_to_seqx(next_id);
>>> - key = idr_alloc(&ids->ipcs_idr, new,
>>> ipcid_to_idx(next_id),
>>> + id = idr_alloc(&ids->ipcs_idr, new,
>>> ipcid_to_idx(next_id),
>>> 0, GFP_NOWAIT);
>>> }
>>> - new->id = SEQ_MULTIPLIER * new->seq + key;
>>> - return key;
>>> + if (id >= 0)
>>> + new->id = SEQ_MULTIPLIER * new->seq + id;
>>
>> We still initialize seq in this case. I guess it's ok because the
>> object is not published at all. But if we are doing this, then perhaps
>> store seq into a local var first and then:
>>
>> if (id >= 0) {
>> new->id = SEQ_MULTIPLIER * seq + id;
>> new->seq = seq:
>> }
>>
>> ?
>
> No!!!
> We must initialize ->seq before publication. Otherwise we end up with the
Right!
> syzcall findings, or in the worst case a strange rare failure of an ipc
> operation.
> The difference between ->id and ->seq is that we have the valid number for
> ->seq.
>
> For the user space ID we cannot have the valid number unless the idr_alloc
> is successful.
> The patch only avoids that this line is executed:
>
>> new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)
>
>
> As I wrote, the line shouldn't cause any damage, the code is more or less:
>>
>> new->id = SEQ_MULTIPLIER * new->seq + (-ENOSPC)
>> kfree(new);
>
> But this is ugly, it asks for problems.
>
> --
> Manfred
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update
2018-07-09 15:10 [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Manfred Spraul
` (11 preceding siblings ...)
2018-07-09 15:10 ` [PATCH 12/12] ipc/util.c: Further ipc_idr_alloc cleanups Manfred Spraul
@ 2018-07-09 20:09 ` Davidlohr Bueso
2018-07-10 4:44 ` Manfred Spraul
12 siblings, 1 reply; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-09 20:09 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
On Mon, 09 Jul 2018, Manfred Spraul wrote:
>@Davidlohr:
>Please double check that I have taken the correct patches, and
>that I didn't break anything.
Everything seems ok.
Patch 8 had an alternative patch that didn't change nowarn semantics for
the rhashtable resizing operations (https://lkml.org/lkml/2018/6/22/732),
but nobody complained about the one you picked up (and also has Michal's ack).
>Especially, I had to reformat the commit ids, otherwise checkpatch
>complained.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update
2018-07-09 20:09 ` [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update Davidlohr Bueso
@ 2018-07-10 4:44 ` Manfred Spraul
2018-07-10 22:10 ` Davidlohr Bueso
0 siblings, 1 reply; 27+ messages in thread
From: Manfred Spraul @ 2018-07-10 4:44 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
Hi Davidlohr,
On 07/09/2018 10:09 PM, Davidlohr Bueso wrote:
> On Mon, 09 Jul 2018, Manfred Spraul wrote:
>
>> @Davidlohr:
>> Please double check that I have taken the correct patches, and
>> that I didn't break anything.
>
> Everything seems ok.
>
> Patch 8 had an alternative patch that didn't change nowarn semantics for
> the rhashtable resizing operations (https://lkml.org/lkml/2018/6/22/732),
> but nobody complained about the one you picked up (and also has
> Michal's ack).
>
Which patch do you prefer?
I have seen two versions, and if I have picked up the wrong one, then I
can change it.
--
Manfred
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/12 V2] ipc: cleanups & bugfixes, rhashtable update
2018-07-10 4:44 ` Manfred Spraul
@ 2018-07-10 22:10 ` Davidlohr Bueso
0 siblings, 0 replies; 27+ messages in thread
From: Davidlohr Bueso @ 2018-07-10 22:10 UTC (permalink / raw)
To: Manfred Spraul; +Cc: Andrew Morton, Dmitry Vyukov, LKML, 1vier1, Kees Cook
On Tue, 10 Jul 2018, Manfred Spraul wrote:
>Which patch do you prefer?
>I have seen two versions, and if I have picked up the wrong one, then
>I can change it.
Nah, you picked up the right one.
I was only pointing out at the alternative patch so that it didn't come
out of nowhere for the rhashtable folks.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 27+ messages in thread