lkml.org 
[lkml]   [2017]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: linux-next 20170519 - semaphores broken
From
Date
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

\
 
 \ /
  Last update: 2017-05-21 20:13    [W:0.046 / U:0.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site