Messages in this thread | | | Subject | Re: linux-next 20170519 - semaphores broken | From | Manfred Spraul <> | Date | Sun, 21 May 2017 20:12:48 +0200 |
| |
Hi Kees,
On 05/21/2017 07:20 AM, Kees Cook wrote: > On Sat, May 20, 2017 at 1:18 PM, <valdis.kletnieks@vt.edu> wrote: >> Seeing problems with programs that use semaphores. The one >> that I'm getting bit by is jackd. strace says: >> >> getuid() = 967 >> semget(0x282929, 0, 000) = 229376 >> semop(229376, [{0, -1, SEM_UNDO}], 1) = -1 EIDRM (Identifier removed) >> write(2, "JACK semaphore error: semop (Ide"..., 49JACK semaphore error: semop (Identifier removed) >> ) = 49 >> >> Bisects down to this commit, and reverting it from 20170519 makes things work >> again. No idea why this causes indigestion, there's probably something subtly >> wrong here.... >> >> commit 337f43326737b5eb28eb13f43c27a5788da0f913 >> Author: Manfred Spraul <manfred@colorfullife.com> >> Date: Fri May 19 07:39:23 2017 +1000 >> >> ipc: merge ipc_rcu and kern_ipc_perm >> >> 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. > I think I found the cause of this. Prior to this change, the RCU (with > refcount) is located ahead of the struct sem_array. After this change, > the RCU and refcount is within it, so this is happening: > > sma = container_of(ipc_rcu_alloc(size), struct sem_array, sem_perm); > if (!sma) > return -ENOMEM; > > memset(sma, 0, size); > > ipc_rcu_alloc() initializes the refcount to 1, and the memset bumps it > back to zero. Correct.
> A work-around would be to wrap the memset() like this: > > struct ipc_kern_perm perm; > ... > perm = sma->sem_perm; > memset(sma, 0, size); > sma->sem_perm = perm; No! The quick workaround would be to move the memset into ipc_rcu_alloc().
> I actually have a series that changes things much more, and moves the > refcount set to ipc_addid() which is the only place it needs to happen > (though this requires fixing up the mistaken rcu freeing on error > paths). Here's the lightly tested series, on top of -next: > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/gcc-plugin/randstruct-next-20170519-ipc > > Manfred, I think I could get to the same results in fewer logical > steps, but I'm curious to see what you think of what I've got first. > Mainly I've done the following things: > > - remove unneeded RCU free calls (since the IPC memory is only exposed > to RCUness once ipc_addid() succeeds. This is the step that I'm not 100% sure about: What about the security_sem_alloc? Is it possible that a security module relies on the RCU?
My first idea would be an embedded rcu protected linked list of the allocations. Since the destruction is security_sem_free(); kvfree() such a list would not work anyway. -> not possible.
But are there other reasons why it might be important to do a call_rcu()? > - move refcount init into ipc_addid() since it only needs to be > initialized from that point on Ok. > - remove utility allocators since now nothing special needs to be done > in the general case Yes! The functions predate kvfree, they do not make any sense. > - result is no requirement of ipc_kern_perms position in ipc > structures and cleaner code, IMO Yes.
The series looks good to me, with the exception of the security interface. I think your changes are safe, but for that part I'm not certain.
I'll try to do a deeper review/test during the next week, then I would Ack the patches.
-- Manfred
| |