From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1166187AbdEZDh3 (ORCPT ); Thu, 25 May 2017 23:37:29 -0400 Received: from mail-it0-f46.google.com ([209.85.214.46]:38502 "EHLO mail-it0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1163497AbdEZDh2 (ORCPT ); Thu, 25 May 2017 23:37:28 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170508222345.GA52073@beast> <20170525185107.12869-1-manfred@colorfullife.com> <20170525185107.12869-3-manfred@colorfullife.com> From: Kees Cook Date: Thu, 25 May 2017 20:37:25 -0700 X-Google-Sender-Auth: RY5rp6DtuRdqYKKY9ZCCv8fcM-o Message-ID: Subject: Re: [PATCH 02/20] ipc: merge ipc_rcu and kern_ipc_perm To: Manfred Spraul Cc: Michael Kerrisk , Andrew Morton , LKML , 1vier1@web.de, Davidlohr Bueso , Ingo Molnar , Peter Zijlstra , Fabian Frederick Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 25, 2017 at 12:34 PM, Kees Cook wrote: > On Thu, May 25, 2017 at 11:50 AM, Manfred Spraul > wrote: >> ipc has two management structures that exist for every id: >> - struct kern_ipc_perm, it contains e.g. the permissions. >> - struct ipc_rcu, it contains the rcu head for rcu handling and >> the refcount. >> >> The patch merges both structures. >> As a bonus, we may save one cacheline, because both structures are >> cacheline aligned. >> In addition, it reduces the number of casts, instead most codepaths can >> use container_of. >> >> To simplify code, the ipc_rcu_alloc initializes the allocation to 0. > > I don't see this change in the code, only the removal of sem's > memset(..., 0, ...)? > >> diff --git a/ipc/sem.c b/ipc/sem.c >> index fff8337..bdff6d9 100644 >> --- a/ipc/sem.c >> +++ b/ipc/sem.c >> @@ -469,20 +469,20 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params) >> if (ns->used_sems + nsems > ns->sc_semmns) >> return -ENOSPC; >> >> + BUILD_BUG_ON(offsetof(struct sem_array, sem_perm) != 0); >> + >> size = sizeof(*sma) + nsems * sizeof(sma->sems[0]); >> - sma = ipc_rcu_alloc(size); >> + sma = container_of(ipc_rcu_alloc(size), struct sem_array, sem_perm); >> if (!sma) >> return -ENOMEM; >> >> - memset(sma, 0, size); >> - >> sma->sem_perm.mode = (semflg & S_IRWXUGO); >> sma->sem_perm.key = key; >> >> sma->sem_perm.security = NULL; >> retval = security_sem_alloc(sma); >> if (retval) { >> - ipc_rcu_putref(sma, ipc_rcu_free); >> + ipc_rcu_putref(&sma->sem_perm, ipc_rcu_free); >> return retval; >> } >> >> [...] >> diff --git a/ipc/util.c b/ipc/util.c >> index caec7b1..9dcc08b 100644 >> --- a/ipc/util.c >> +++ b/ipc/util.c >> @@ -418,46 +418,43 @@ void ipc_free(void *ptr) >> } >> >> /** >> - * ipc_rcu_alloc - allocate ipc and rcu space >> + * ipc_rcu_alloc - allocate ipc space >> * @size: size desired >> * >> - * Allocate memory for the rcu header structure + the object. >> - * Returns the pointer to the object or NULL upon failure. >> + * Allocate memory for an ipc object. >> + * The first member must be struct kern_ipc_perm. >> */ >> -void *ipc_rcu_alloc(int size) >> +struct kern_ipc_perm *ipc_rcu_alloc(int size) >> { >> /* >> * We prepend the allocation with the rcu struct >> */ >> - struct ipc_rcu *out = ipc_alloc(sizeof(struct ipc_rcu) + size); >> + struct kern_ipc_perm *out = ipc_alloc(size); >> if (unlikely(!out)) >> return NULL; >> atomic_set(&out->refcount, 1); >> - return out + 1; >> + return out; >> } See above: - newary() loses memset() - ipc_rcu_alloc() does not gain it - changelog says "To simplify code, the ipc_rcu_alloc initializes the allocation to 0." This is what's wanted but the patch doesn't do it. The actual change that (temporarily) adds memset to ipc_rcu_alloc() is one of the following patches, but it should be here or bisection may cause unexpected results for semaphores. It's a minor issue, since these will likely all land at the same time, but it's probably still worth fixing. -Kees -- Kees Cook Pixel Security