From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2EF7FC1B0D9 for ; Fri, 4 Dec 2020 17:40:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0B8F422B4E for ; Fri, 4 Dec 2020 17:40:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731317AbgLDRkc (ORCPT ); Fri, 4 Dec 2020 12:40:32 -0500 Received: from out02.mta.xmission.com ([166.70.13.232]:58566 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726518AbgLDRka (ORCPT ); Fri, 4 Dec 2020 12:40:30 -0500 Received: from in01.mta.xmission.com ([166.70.13.51]) by out02.mta.xmission.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1klF3a-009PFt-D2; Fri, 04 Dec 2020 10:39:46 -0700 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1klF3Z-0002bV-IP; Fri, 04 Dec 2020 10:39:46 -0700 From: ebiederm@xmission.com (Eric W. Biederman) To: Bernd Edlinger Cc: linux-kernel@vger.kernel.org, Linus Torvalds , Peter Zijlstra , Ingo Molnar , Will Deacon , Jann Horn , Vasiliy Kulikov , Al Viro , Oleg Nesterov , Christopher Yeoh , Cyrill Gorcunov , Sargun Dhillon , Christian Brauner , Arnd Bergmann , Arnaldo Carvalho de Melo , Waiman Long , Davidlohr Bueso References: <87tut2bqik.fsf@x220.int.ebiederm.org> <87ft4mbqen.fsf@x220.int.ebiederm.org> Date: Fri, 04 Dec 2020 11:39:11 -0600 In-Reply-To: (Bernd Edlinger's message of "Fri, 4 Dec 2020 17:08:01 +0100") Message-ID: <87y2id8o8w.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1klF3Z-0002bV-IP;;;mid=<87y2id8o8w.fsf@x220.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18/dyuJ2QBuiFTXqQirv4qseGVz19j5qeg= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 3/3] exec: Transform exec_update_mutex into a rw_semaphore X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Bernd Edlinger writes: > Hi Eric, > > I think I remembered from a previous discussion about this topic, > that it was unclear if the rw_semaphores are working the same > in RT-Linux. Will this fix work in RT as well? The locks should work close enough to the same that correct code is also correct code on RT-linux. If not it is an RT-linux bug. An rw_semaphore may be less than optimal on RT-linux. I do remember that mutexes are prefered. But this change is more about correctness than anything else. > On 12/3/20 9:12 PM, Eric W. Biederman wrote: >> --- a/kernel/kcmp.c >> +++ b/kernel/kcmp.c >> @@ -70,25 +70,25 @@ get_file_raw_ptr(struct task_struct *task, unsigned int idx) >> return file; >> } >> >> -static void kcmp_unlock(struct mutex *m1, struct mutex *m2) >> +static void kcmp_unlock(struct rw_semaphore *l1, struct rw_semaphore *l2) >> { >> - if (likely(m2 != m1)) >> - mutex_unlock(m2); >> - mutex_unlock(m1); >> + if (likely(l2 != l1)) > > is this still necessary ? Yes. Both pids could be threads of the same process or even the same value so yes this is definitely necessary. rw_semaphores don't nest on the same cpu. > >> + up_read(l2); >> + up_read(l1); >> } >> >> -static int kcmp_lock(struct mutex *m1, struct mutex *m2) >> +static int kcmp_lock(struct rw_semaphore *l1, struct rw_semaphore *l2) >> { >> int err; >> >> - if (m2 > m1) >> - swap(m1, m2); >> + if (l2 > l1) >> + swap(l1, l2); > > and this is probably also no longer necessary? I think lockdep needs this, so it can be certain the same rw_semaphore is not nesting on the cpu. Otherwise we will have inconsitencies about which is the nested lock. It won't matter in practice, but I am not certain lockdep knows enough to tell the difference. If anything removing the swap is a candidate for a follow up patch where it can be considered separately from other concerns. For this patch keeping the logic unchanged makes it trivial to verify that the conversion from one lock to another is correct. >> >> - err = mutex_lock_killable(m1); >> - if (!err && likely(m1 != m2)) { >> - err = mutex_lock_killable_nested(m2, SINGLE_DEPTH_NESTING); >> + err = down_read_killable(l1); >> + if (!err && likely(l1 != l2)) { > > and this can now be unconditionally, right? Nope. The two locks can be the same lock, and they don't nest on a single cpu. I tested and verified that lockdep complains bitterly if down_read_killable_nested is replaced with a simple down_read_killable. >> + err = down_read_killable_nested(l2, SINGLE_DEPTH_NESTING); >> if (err) >> - mutex_unlock(m1); >> + up_read(l1); >> } >> >> return err; >> @@ -156,8 +156,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type, >> /* >> * One should have enough rights to inspect task details. >> */ >> - ret = kcmp_lock(&task1->signal->exec_update_mutex, >> - &task2->signal->exec_update_mutex); >> + ret = kcmp_lock(&task1->signal->exec_update_lock, >> + &task2->signal->exec_update_lock); >> if (ret) >> goto err; >> if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) || >> @@ -212,8 +212,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type, >> } >> >> err_unlock: >> - kcmp_unlock(&task1->signal->exec_update_mutex, >> - &task2->signal->exec_update_mutex); >> + kcmp_unlock(&task1->signal->exec_update_lock, >> + &task2->signal->exec_update_lock); >> err: >> put_task_struct(task1); >> put_task_struct(task2); > > > Thanks > Bernd.