On Thu, 30 Nov 2017, Philippe Mikoyan wrote:
>As described in the title, this patch fixes <ipc>id_ds inconsistency
>when <ipc>ctl_stat runs concurrently with some ds-changing function,
>e.g. shmat, msgsnd or whatever.
>
>For instance, if shmctl(IPC_STAT) is running concurrently with shmat,
>following data structure can be returned:
>{... shm_lpid = 0, shm_nattch = 1, ...}
Hmm yeah that's pretty fishy, also shm_atime = 0, no?
So I think this patch is fine as we can obviously race at a user level.
This is another justification for converting the ipc lock to rwlock;
performance wise they are the pretty much the same (being queued)...
but that's irrelevant to this patch. I like that you manage to do
security and such checks still only under rcu, like all ipc calls
work; *_stat() is no longer special.
With a nit below:
Reviewed-by: Davidlohr Bueso <[email protected]>
>diff --git a/ipc/util.c b/ipc/util.c
>index 78755873cc5b..8d3c3946c825 100644
>--- a/ipc/util.c
>+++ b/ipc/util.c
>@@ -22,9 +22,12 @@
> * tree.
> * - perform initial checks (capabilities, auditing and permission,
> * etc).
>- * - perform read-only operations, such as STAT, INFO commands.
>+ * - perform read-only operations, such as INFO command, that
>+ * do not demand atomicity
> * acquire the ipc lock (kern_ipc_perm.lock) through
> * ipc_lock_object()
>+ * - perform read-only operatoins that demand atomicity,
^^ typo
>+ * such as STAT command.
> * - perform data updates, such as SET, RMID commands and
> * mechanism-specific operations (semop/semtimedop,
> * msgsnd/msgrcv, shmat/shmdt).
Thanks,
Davidlohr
Hi,
On 12/01/2017 06:20 PM, Davidlohr Bueso wrote:
> On Thu, 30 Nov 2017, Philippe Mikoyan wrote:
>
>> As described in the title, this patch fixes <ipc>id_ds inconsistency
>> when <ipc>ctl_stat runs concurrently with some ds-changing function,
>> e.g. shmat, msgsnd or whatever.
>>
>> For instance, if shmctl(IPC_STAT) is running concurrently with shmat,
>> following data structure can be returned:
>> {... shm_lpid = 0, shm_nattch = 1, ...}
>
The patch appears to be good. I'll try to perform some tests, but I'm
not sure when I will be able to.
Especially: I don't know the shm code good enough to immediately check
the change you make to nattach.
And, perhaps as a side information:
There appears to be a use-after-free in shm, I now got a 2nd mail from
syzbot:
http://lkml.iu.edu/hypermail/linux/kernel/1702.3/02480.html
> Hmm yeah that's pretty fishy, also shm_atime = 0, no?
>
> So I think this patch is fine as we can obviously race at a user level.
> This is another justification for converting the ipc lock to rwlock;
> performance wise they are the pretty much the same (being queued)...
> but that's irrelevant to this patch. I like that you manage to do
> security and such checks still only under rcu, like all ipc calls
> work; *_stat() is no longer special.
>
I don't like rwlock, they add complexity without reducing the cache line
pressure.
What I would like to try is to create a mutex_lock_rcu() function, and
then convert everything to a mutex.
As pseudocode::
rcu_lock();
idr_lookup();
mutex_trylock();
if (failed) {
getref();
rcu_unlock();
mutex_lock();
putref();
} else {
rcu_unlock();
}
Obviously, the getref then within the mutex framework, i.e. only if
mutex_lock() really sleeps.
If the code in ipc gets significantly simpler, then perhaps convert it
to an rw mutex.
On Fri, 1 Dec 2017 09:20:07 -0800
Davidlohr Bueso <[email protected]> wrote:
>
> Hmm yeah that's pretty fishy, also shm_atime = 0, no?
>
Yeah, definitely, other data structure fields can also be
inconsistent, and applying not only to shmem, but also to
other ipc mechanisms.
Thank you for noting the typo, 'll send fixed version in next
message(without another patch, see below).
On Sat, 2 Dec 2017 07:03:30 +0100
Manfred Spraul <[email protected]> wrote:
> Especially: I don't know the shm code good enough to immediately
> check the change you make to nattach.
It seems that I didn't know the shm code good enough too: I've
recently discovered that
[PATCH 1/2] ipc/shm: Fix shm_nattch incorrect value
is, frankly speaking, clearly total crap as it
1) doesn't handle that shmem segment can be already RMID-ed
when entering shm_mmap, when called from 'remap_file_pages'
2) doesn't support (broken) logic of detaching remapped via
'remap_file_pages' shmem segment.
Regardless of handling (deprecated) 'remap_file_pages' call, patch
shall be OK. However, it has to be made over.
Sorry about that, hope I will find at least halfway elegant
solution and send it ASAP.
On Sat, 2 Dec 2017 07:03:30 +0100
Manfred Spraul <[email protected]> wrote:
>
> And, perhaps as a side information:
> There appears to be a use-after-free in shm, I now got a 2nd mail
> from syzbot:
> http://lkml.iu.edu/hypermail/linux/kernel/1702.3/02480.html
>
Will dig into.
Thanks,
Phil
On Sat, 02 Dec 2017, Philippe Mikoyan wrote:
>On Fri, 1 Dec 2017 09:20:07 -0800
>Davidlohr Bueso <[email protected]> wrote:
>
>>
>> Hmm yeah that's pretty fishy, also shm_atime = 0, no?
>>
>
>Yeah, definitely, other data structure fields can also be
>inconsistent, and applying not only to shmem, but also to
>other ipc mechanisms.
Right.
>
>Thank you for noting the typo, 'll send fixed version in next
>message(without another patch, see below).
Ok, I had yet to look at that patch.