linux-kernel - Re: [PATCH] ipc/sem.c: fix update sem_otime when calling sem

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5241FF8D.8000407@colorfullife.com>
Date:	Tue, 24 Sep 2013 23:09:33 +0200
From:	Manfred Spraul <manfred@...orfullife.com>
To:	Jia He <jiakernel@...il.com>
CC:	Mike Galbraith <bitbucket@...ine.de>, linux-kernel@...r.kernel.org,
	Davidlohr Bueso <davidlohr.bueso@...com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] ipc/sem.c: fix update sem_otime when calling sem_op in
 semaphore initialization

On 09/22/2013 05:14 PM, Jia He wrote:
>   Hi Manfred
>
> On Sun, 22 Sep 2013 12:42:05 +0200 from manfred@...orfullife.com wrote:
>> Hi all,
>>
>> On 09/22/2013 10:26 AM, Mike Galbraith wrote:
>>> On Sun, 2013-09-22 at 10:17 +0200, Mike Galbraith wrote:
>>>> On Sun, 2013-09-22 at 10:11 +0800, Jia He wrote:
>>>>> In commit 0a2b9d4c,the update of semaphore's sem_otime(last semop time)
>>>>> was removed because he wanted to move setting sem->sem_otime to one
>>>>> place. But after that, the initial semop() will not set the otime
>>>>> because its sem_op value is 0(in semtimedop,will not change
>>>>> otime if alter == 1).
>>>>>
>>>>> the error case:
>>>>> process_a(server)       process_b(client)
>>>>> semget()
>>>>> semctl(SETVAL)
>>>>> semop()
>>>>>                           semget()
>>>>>                           setctl(IP_STAT)
>>>>>                           for(;;) {               <--not successful here
>>>>>                             check until sem_otime > 0
>>>>>                           }
>> Good catch:
>> Since commit 0a2b9d4c, wait-for-zero semops do not update sem_otime anymore.
>>
>> Let's reverse that part of my commit and move the update of sem_otime back
>> into perform_atomic_semop().
>>
>> Jia: If perform_atomic_semop() updates sem_otime, then the update in
>> do_smart_update() is not necessary anymore.
>> Thus the whole logic with passing arround "semop_completed" can be removed, too.
>> Are you interested in writing that patch?
>>
> Not all perform_atomic_semop() can cover the points of do_smart_update()
> after I move the parts of updating otime.
> Eg. in semctl_setval/exit_sem/etc.    That is, it seems I need to write some
> other codes to update sem_otime outside perform_atomic_semop(). That
> still violate your original goal---let one function do all the update otime
> things.
No. The original goal was an optimization:
The common case is one semop that increases a semaphore value - and that 
allows another semop that is sleeping to proceed.
Before, this caused two get_seconds() calls.
The current (buggy) code avoids the 2nd call.

> IMO, what if just remove the condition alter in sys_semtimedop:
> -        if (alter && error == 0)
> +       if (error == 0)
>              do_smart_update(sma, sops, nsops, 1, &tasks);
> In old codes, it would set the otime even when sem_op == 0
do_smart_update() can be expensive - thus it shouldn't be called if we 
didn't change anything.

Attached is a proposed patch - fully untested. It is intended to be 
applied on top of Jia's patch.

_If_ the performance degradation is too large, then the alternative 
would be to set sem_otime directly in semtimedop() for successfull 
non-alter operations.

--
     Manfred

View attachment "0001-ipc-sem.c-Simplify-sem_otime-handling.patch" of type "text/x-patch" (7743 bytes)