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
| ||
|
Date: Wed, 25 Sep 2013 11:05:58 +0800 From: Jia He <jiakernel@...il.com> To: Manfred Spraul <manfred@...orfullife.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 Hi Manfred IIUC after reivewing your patch and src code, does it seem sem_otime lost the chance to be updated when calling semctl_main/semctl_setval? In old codes, even whendo_smart_update(sma, NULL, 0, 0, &tasks), the otime can be updated after several conditions checking. On Tue, 24 Sep 2013 23:09:33 +0200 from manfred@...orfullife.com wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists