| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <047c6ed6-6581-b543-ba3d-cadc543d3d25@redhat.com>
Date: Thu, 15 Mar 2018 13:02:12 -0400
From: Waiman Long <longman@...hat.com>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: "Luis R. Rodriguez" <mcgrof@...nel.org>,
Kees Cook <keescook@...omium.org>,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Al Viro <viro@...iv.linux.org.uk>,
Matthew Wilcox <willy@...radead.org>,
Stanislav Kinsbursky <skinsbursky@...allels.com>,
Linux Containers <containers@...ts.linux-foundation.org>
Subject: Re: [RFC][PATCH] ipc: Remove IPCMNI
On 03/14/2018 08:49 PM, Eric W. Biederman wrote:
> The define IPCMNI was originally the size of a statically sized array in
> the kernel and that has long since been removed. Therefore there is no
> fundamental reason for IPCMNI.
>
> The only remaining use IPCMNI serves is as a convoluted way to format
> the ipc id to userspace. It does not appear that anything except for
> the CHECKPOINT_RESTORE code even cares about this variety of assignment
> and the CHECKPOINT_RESTORE code only cares about this weirdness because
> it has to restore these peculiar ids.
>
> Therefore make the assignment of ipc ids match the description in
> Advanced Programming in the Unix Environment and assign the next id
> until INT_MAX is hit then loop around to the lower ids.
>
> This can be implemented trivially with the current code using idr_alloc_cyclic.
>
> To make it possible to keep checkpoint/restore working I have renamed
> the sysctls from xxx_next_id to xxx_nextid. That is enough change that
> a smart CRIU implementation can see that what is exported has changed,
> and act accordingly. New kernels will be able to restore the old id's.
>
> This code still needs some real world testing to verify my assumptions.
> And some work with the CRIU implementations to actually add the code
> that deals with the new for of id assignment.
>
> Updates: 03f595668017 ("ipc: add sysctl to specify desired next object id")
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> ---
>
> Waiman please take a look at this and run it through some tests etc,
> I am pretty certain something like this patch is all you need to do
> to sort out ipc assignment. Not messing with sysctls needed.
>
> include/linux/ipc.h | 2 --
> include/linux/ipc_namespace.h | 1 -
> ipc/ipc_sysctl.c | 6 ++--
> ipc/namespace.c | 11 ++----
> ipc/util.c | 80 ++++++++++---------------------------------
> ipc/util.h | 11 +-----
> 6 files changed, 25 insertions(+), 86 deletions(-)
>
> diff --git a/include/linux/ipc.h b/include/linux/ipc.h
> index 821b2f260992..6cc2df7f7ac9 100644
> --- a/include/linux/ipc.h
> +++ b/include/linux/ipc.h
> @@ -8,8 +8,6 @@
> #include <uapi/linux/ipc.h>
> #include <linux/refcount.h>
>
> -#define IPCMNI 32768 /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
> -
> /* used by in-kernel data structures */
> struct kern_ipc_perm {
> spinlock_t lock;
> diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
> index b5630c8eb2f3..cab33b6a8236 100644
> --- a/include/linux/ipc_namespace.h
> +++ b/include/linux/ipc_namespace.h
> @@ -15,7 +15,6 @@ struct user_namespace;
>
> struct ipc_ids {
> int in_use;
> - unsigned short seq;
> bool tables_initialized;
> struct rw_semaphore rwsem;
> struct idr ipcs_idr;
> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
> index 8ad93c29f511..a599963d58bf 100644
> --- a/ipc/ipc_sysctl.c
> +++ b/ipc/ipc_sysctl.c
> @@ -176,7 +176,7 @@ static struct ctl_table ipc_kern_table[] = {
> },
> #ifdef CONFIG_CHECKPOINT_RESTORE
> {
> - .procname = "sem_next_id",
> + .procname = "sem_nextid",
> .data = &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
> .maxlen = sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
> .mode = 0644,
> @@ -185,7 +185,7 @@ static struct ctl_table ipc_kern_table[] = {
> .extra2 = &int_max,
> },
> {
> - .procname = "msg_next_id",
> + .procname = "msg_nextid",
> .data = &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
> .maxlen = sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
> .mode = 0644,
> @@ -194,7 +194,7 @@ static struct ctl_table ipc_kern_table[] = {
> .extra2 = &int_max,
> },
> {
> - .procname = "shm_next_id",
> + .procname = "shm_nextid",
> .data = &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
> .maxlen = sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
> .mode = 0644,
So you are changing the names of existing sysctl parameters. Will it be
better to add new sysctl to indicate that the rule has changed instead?
> diff --git a/ipc/namespace.c b/ipc/namespace.c
> index f59a89966f92..84eaeba9e96c 100644
> --- a/ipc/namespace.c
> +++ b/ipc/namespace.c
> @@ -109,20 +109,13 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
> {
> struct kern_ipc_perm *perm;
> int next_id;
> - int total, in_use;
>
> down_write(&ids->rwsem);
> -
> - in_use = ids->in_use;
> -
> - for (total = 0, next_id = 0; total < in_use; next_id++) {
> - perm = idr_find(&ids->ipcs_idr, next_id);
> - if (perm == NULL)
> - continue;
> + next_id = 0;
> + while ((perm = idr_get_next(&ids->ipcs_idr, &next_id))) {
> rcu_read_lock();
> ipc_lock_object(perm);
> free(ns, perm);
> - total++;
> }
> up_write(&ids->rwsem);
> }
> diff --git a/ipc/util.c b/ipc/util.c
> index 4ed5a17dd06f..ce6bf18e54df 100644
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -118,7 +118,6 @@ int 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)
> @@ -192,46 +191,18 @@ 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.
> - */
> -#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_buildid(int id, struct ipc_ids *ids,
> - struct kern_ipc_perm *new)
> +static int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
> {
> - if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */
> - new->seq = ids->seq++;
> - if (ids->seq > IPCID_SEQ_MAX)
> - ids->seq = 0;
> - } else {
> - new->seq = ipcid_to_seqx(ids->next_id);
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> + if (ids->next_id >= 0) {
> + idr_set_cursor(&ids->ipcs_idr, 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,
> - 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
> + return idr_alloc_cyclic(&ids->ipcs_idr, (new), 0, 0, GFP_NOWAIT);
> }
>
> -#endif /* CONFIG_CHECKPOINT_RESTORE */
> -
> /**
> * ipc_addid - add an ipc identifier
> * @ids: ipc identifier set
> @@ -251,9 +222,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
> kgid_t egid;
> int id, err;
>
> - if (limit > IPCMNI)
> - limit = IPCMNI;
> -
> if (!ids->tables_initialized || ids->in_use >= limit)
> return -ENOSPC;
>
> @@ -290,7 +258,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 = id;
>
> return id;
> }
> @@ -430,7 +398,7 @@ static void ipc_kht_remove(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
> */
> void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
> {
> - int lid = ipcid_to_idx(ipcp->id);
> + int lid = ipcp->id;
>
> idr_remove(&ids->ipcs_idr, lid);
> ipc_kht_remove(ids, ipcp);
> @@ -563,7 +531,7 @@ void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out)
> 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);
> + int lid = id;
>
> if (unlikely(!ids->tables_initialized))
> return ERR_PTR(-EINVAL);
> @@ -757,30 +725,20 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
> loff_t *new_pos)
> {
> struct kern_ipc_perm *ipc;
> - int total, id;
> -
> - total = 0;
> - for (id = 0; id < pos && total < ids->in_use; id++) {
> - ipc = idr_find(&ids->ipcs_idr, id);
> - if (ipc != NULL)
> - total++;
> - }
> + int id;
I think you need to initialize id to pos. Right?
>
> - if (total >= ids->in_use)
> + /* Out of range - return NULL to terminate iteration */
> + if (pos > INT_MAX)
> return NULL;
>
> - for (; pos < IPCMNI; pos++) {
> - ipc = idr_find(&ids->ipcs_idr, pos);
> - if (ipc != NULL) {
> - *new_pos = pos + 1;
> - rcu_read_lock();
> - ipc_lock_object(ipc);
> - return ipc;
> - }
> - }
> + ipc = idr_get_next(&ids->ipcs_idr, &id);
> + if (!ipc)
> + return NULL;
>
> - /* Out of range - return NULL to terminate iteration */
> - return NULL;
> + *new_pos = id + 1;
> + rcu_read_lock();
> + ipc_lock_object(ipc);
> + return ipc;
> }
>
> static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)
> diff --git a/ipc/util.h b/ipc/util.h
> index 89b8ec176fc4..de8e27367f0c 100644
> --- a/ipc/util.h
> +++ b/ipc/util.h
> @@ -15,8 +15,6 @@
> #include <linux/err.h>
> #include <linux/ipc_namespace.h>
>
> -#define SEQ_MULTIPLIER (IPCMNI)
> -
> int sem_init(void);
> int msg_init(void);
> void shm_init(void);
> @@ -93,10 +91,6 @@ void __init ipc_init_proc_interface(const char *path, const char *header,
> #define IPC_MSG_IDS 1
> #define IPC_SHM_IDS 2
>
> -#define ipcid_to_idx(id) ((id) % SEQ_MULTIPLIER)
> -#define ipcid_to_seqx(id) ((id) / SEQ_MULTIPLIER)
> -#define IPCID_SEQ_MAX min_t(int, INT_MAX/SEQ_MULTIPLIER, USHRT_MAX)
> -
> /* must be called with ids->rwsem acquired for writing */
> int ipc_addid(struct ipc_ids *, struct kern_ipc_perm *, int);
>
> @@ -120,9 +114,6 @@ static inline int ipc_get_maxid(struct ipc_ids *ids)
> if (ids->in_use == 0)
> return -1;
>
> - if (ids->in_use == IPCMNI)
> - return IPCMNI - 1;
> -
> return ids->max_id;
> }
>
> @@ -163,7 +154,7 @@ extern int store_msg(void __user *dest, struct msg_msg *msg, size_t len);
>
> static inline int ipc_checkid(struct kern_ipc_perm *ipcp, int uid)
> {
> - return uid / SEQ_MULTIPLIER != ipcp->seq;
> + return uid != ipcp->seq;
> }
>
> static inline void ipc_lock_object(struct kern_ipc_perm *perm)
I don't know the history why the id management of SysV IPC was designed
in such a convoluted way, but the patch does make sense to me.
Cheers,
Longman
Powered by blists - more mailing lists