Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp739537ybl; Tue, 28 Jan 2020 11:10:54 -0800 (PST) X-Google-Smtp-Source: APXvYqzNbeHEVH5q+h16KfSXE8okZdfoh4qvRfLYIvvGpjRViWbyYCHIosPNndguhuVSvms/ADhm X-Received: by 2002:aca:bac3:: with SMTP id k186mr4003340oif.19.1580238654490; Tue, 28 Jan 2020 11:10:54 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1580238654; cv=none; d=google.com; s=arc-20160816; b=cqSZxdaIkYIiqb5TqAl/WIivmmHDI9fAL0iXhKlS8kOIiD4B/HefNI6R1xwBZg1KOy 2CIC4n74tNeos9cMlsZC0kOBbX1aunwclEmNUSEqa5EVXxLs5E1lEhqXcZNV/zWq+775 9McDtoP3jEAhQ9BuE3dmUfV93ZXVW8lyl27QDuknI7kDD87CFqQJGE6lOauUtfjd4rQ+ 2OAuOiTpUtZf0fYDLbkx6B2Q4PoXP7gy8EkYWXn3mXYNRNAHVl9jKeLL7JuWMRNIUzCe Nw/dzzEfpQz7YTZbcrCQ54xie6VuL0sb5WFlQ4DD1bebo905KHy1HzMPfrMTaGBtdZrr xndg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=0uDY2QBhfEUdWOh6cPotJ77cqPe1RSmXYbfImdKMurk=; b=MdnYlKaMA7GGPmbBjXdWKS72xkmeOFp3i/eUkzJZIVe/hG9pfgbIVNscYgJe0Hp7Gm c3pjPU0BIyv9cTrmKFZGShk8LlzzoH8ynAikpxK0OkxgGg7bdROoSa7kyhcux8ZgdYki z6gDG3yoTOPCOLxXeppJBJOe36l9QibljxIotLxuWFmayMoCiRT+HdZAxJ8GUkaGM6ph i76lPbnaopF3OOi1m7eF5L4wTvic8aR+ydcyJj8m2MB2HZd9jZ/KaS2h/bGloelgwstD ngl7Gni0+YjtQQ2qfu5HDHUqqev1tZhpUcdrFohqSOiLXu/jLTPmxZ9FlpdJDh+UgGQ1 hHWg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=OjMQ2XmT; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id l14si9444197otk.225.2020.01.28.11.10.41; Tue, 28 Jan 2020 11:10:54 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=OjMQ2XmT; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726590AbgA1TJp (ORCPT + 99 others); Tue, 28 Jan 2020 14:09:45 -0500 Received: from mail-ot1-f68.google.com ([209.85.210.68]:46095 "EHLO mail-ot1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726002AbgA1TJo (ORCPT ); Tue, 28 Jan 2020 14:09:44 -0500 Received: by mail-ot1-f68.google.com with SMTP id g64so13075060otb.13 for ; Tue, 28 Jan 2020 11:09:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0uDY2QBhfEUdWOh6cPotJ77cqPe1RSmXYbfImdKMurk=; b=OjMQ2XmTWVstZOf1eyavV2tz0/5ffAvoB3yOj3xIx5xzt3CKWH8L2jQLHKoUsXQCQv XoTJPvBAF2gGaG1UhrSnqQXYiLfmnOFe8E0RoMbZwdJoo4dgAJFQBenA+XtU0o6T0EgF HYITPw6U+8nEcY4PoWY+NNr1Eq+Jv3a6MCyzmdzjzvSh/QaUSsRR6FVq76JJ6VDHEJr8 UIgg29EKhzl/lzCba8uvoxh7lizoJjJvy9DkECKCgiRpzre3XFtqIaCjjPi3eUTGpRQ7 ZG/2KQ6YHj4mo752Lh+DR0TA4Y5bUn9DSW49bsstpefiCr+eMQzlIwcnrZjkSQ69NtEm fz8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0uDY2QBhfEUdWOh6cPotJ77cqPe1RSmXYbfImdKMurk=; b=MH/KZQafvwbm/yerGt3DGzxgmgXG4v/Jn0AenxXESvrZ9UZ0oR9xnG9QSLrNJoDhBH 4yTXTyEg6l4yq05REvwXxtpcCKKwy3NdvFWFMFbUc/6N0KVI8N3fyTfHU8I3xJcOb+Zv uzV4h2qACH3OzlKJf9j5Q4k8kBZ5JTKFvFZHOt/civ7yX9X4rkMOfrAyCMUKDqoR3ATW eFlSmghuFtAt28JsBkHvIj63klLIk7wdFp1ThMrCjAlfBPU+7+NejfUDuZHV/dwk7r40 IfT3tfg2NUMR0NrAiRlx+5mdYV3pmXrvAYOGkdkrlTJiHaJRf3W2eQo00UkpDTCJVHyH 7gJg== X-Gm-Message-State: APjAAAW8rao5uDInSb9NcPjCK/mOiHsWTVbBxOXRScLqFNBFJ4BXvfEN 9/6WBTT6kqJCW9SmMU7jnJEqPtTWVZZ8gOXw7pNVxA== X-Received: by 2002:a05:6830:1d6e:: with SMTP id l14mr17212146oti.32.1580238583961; Tue, 28 Jan 2020 11:09:43 -0800 (PST) MIME-Version: 1.0 References: <20200128072740.21272-1-frextrite@gmail.com> <20200128170426.GA10277@workstation-portable> In-Reply-To: <20200128170426.GA10277@workstation-portable> From: Jann Horn Date: Tue, 28 Jan 2020 20:09:17 +0100 Message-ID: Subject: Re: [PATCH] cred: Use RCU primitives to access RCU pointers To: Amol Grover Cc: David Howells , Shakeel Butt , James Morris , Oleg Nesterov , Kees Cook , Thomas Gleixner , kernel list , linux-kernel-mentees@lists.linuxfoundation.org, Joel Fernandes , Madhuparna Bhowmik , "Paul E . McKenney" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 28, 2020 at 6:04 PM Amol Grover wrote: > On Tue, Jan 28, 2020 at 10:30:19AM +0100, Jann Horn wrote: > > On Tue, Jan 28, 2020 at 8:28 AM Amol Grover wrote: > > > task_struct.cred and task_struct.real_cred are annotated by __rcu, > > > > task_struct.cred doesn't actually have RCU semantics though, see > > commit d7852fbd0f0423937fa287a598bfde188bb68c22. For task_struct.cred, > > it would probably be more correct to remove the __rcu annotation? > > > > Hi Jann, > > I went through the commit you mentioned. If I understand it correctly, > ->cred was not being accessed concurrently (via RCU), hence, a non_rcu > flag was introduced, which determined if the clean-up should wait for > RCU grace-periods or not. And since, the changes were 'thread local' > there was no need to wait for an entire RCU GP to elapse. Yeah. > The commit too, as you said, mentions the removal of __rcu annotation. > However, simply removing the annotation won't work, as there are quite a > few instances where RCU primitives are used. Even get_current_cred() > uses RCU APIs to get a reference to ->cred. Luckily, there aren't too many places that directly access ->cred, since luckily there are helper functions like get_current_cred() that will do it for you. Grepping through the kernel, I see: Places that need adjustment: include/linux/cred.h: rcu_dereference_protected(current->cred, 1) kernel/auditsc.c: * the only situations where tsk->cred may be accessed without an rcu read lock. kernel/auditsc.c: cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation); kernel/cred.c: rcu_assign_pointer(task->cred, new); kernel/cred.c: rcu_assign_pointer(current->cred, new); kernel/cred.c: rcu_assign_pointer(current->cred, old); Places that already don't use RCU accessors: drivers/virt/vboxguest/vboxguest_linux.c: if (from_kuid(current_user_ns(), current->cred->uid) == 0) kernel/cred.c: BUG_ON(cred == current->cred); kernel/cred.c: kdebug("exit_creds(%u,%p,%p,{%d,%d})", tsk->pid, tsk->real_cred, tsk->cred, kernel/cred.c: atomic_read(&tsk->cred->usage), kernel/cred.c: read_cred_subscribers(tsk->cred)); kernel/cred.c: cred = (struct cred *) tsk->cred; kernel/cred.c: tsk->cred = NULL; kernel/cred.c: old = task->cred; kernel/cred.c: !p->cred->thread_keyring && kernel/cred.c: p->real_cred = get_cred(p->cred); kernel/cred.c: get_cred(p->cred); kernel/cred.c: alter_cred_subscribers(p->cred, 2); kernel/cred.c: p->cred, atomic_read(&p->cred->usage), kernel/cred.c: read_cred_subscribers(p->cred)); kernel/cred.c: atomic_inc(&p->cred->user->processes); kernel/cred.c: p->cred = p->real_cred = get_cred(new); kernel/cred.c: BUG_ON(task->cred != old); kernel/cred.c: const struct cred *old = current->cred; kernel/cred.c: * '->cred' pointer, not the '->real_cred' pointer that is kernel/cred.c: const struct cred *override = current->cred; kernel/cred.c: cred == tsk->cred ? "[eff]" : ""); kernel/cred.c: if (tsk->cred == tsk->real_cred) { kernel/cred.c: if (unlikely(read_cred_subscribers(tsk->cred) < 2 || kernel/cred.c: creds_are_invalid(tsk->cred))) kernel/cred.c: read_cred_subscribers(tsk->cred) < 1 || kernel/cred.c: creds_are_invalid(tsk->cred))) kernel/cred.c: if (tsk->cred != tsk->real_cred) kernel/cred.c: dump_invalid_creds(tsk->cred, "Effective", tsk); kernel/cred.c: tsk->real_cred, tsk->cred, kernel/cred.c: atomic_read(&tsk->cred->usage), kernel/cred.c: read_cred_subscribers(tsk->cred)); kernel/fork.c: atomic_dec(&p->cred->user->processes); security/security.c: lsm_early_cred((struct cred *) current->cred); security/smack/smack_lsm.c: struct cred *cred = (struct cred *) current->cred; security/tomoyo/common.c: (!uid_eq(task->cred->uid, GLOBAL_ROOT_UID) || security/tomoyo/common.c: !uid_eq(task->cred->euid, GLOBAL_ROOT_UID))) Places that don't use RCU and are broken: security/smack/smack_lsm.c: struct smack_known *tkp = smk_of_task(smack_cred(tsk->cred)); So actually, the number of places that already don't use RCU accessors is much higher than the number of places that use them. > So, currently, maybe we > should continue to use RCU APIs and leave the __rcu annotation in? > (Until someone who takes it on himself to remove __rcu annotation and > fix all the instances). Does that sound good? Or do you want me to > remove __rcu annotation and get the process started? I don't think it's a good idea to add more uses of RCU APIs for ->cred; you shouldn't "fix" warnings by making the code more wrong. If you want to fix this, I think it would be relatively easy to fix this properly - as far as I can tell, there are only seven places that you'll have to change, although you may have to split it up into three patches.