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=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 83601C433DF for ; Fri, 21 Aug 2020 19:37:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4FEE22072D for ; Fri, 21 Aug 2020 19:37:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=paul-moore-com.20150623.gappssmtp.com header.i=@paul-moore-com.20150623.gappssmtp.com header.b="sD6uHtx5" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726630AbgHUThH (ORCPT ); Fri, 21 Aug 2020 15:37:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35612 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726431AbgHUThE (ORCPT ); Fri, 21 Aug 2020 15:37:04 -0400 Received: from mail-ej1-x643.google.com (mail-ej1-x643.google.com [IPv6:2a00:1450:4864:20::643]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2958DC061755 for ; Fri, 21 Aug 2020 12:37:04 -0700 (PDT) Received: by mail-ej1-x643.google.com with SMTP id qc22so3689210ejb.4 for ; Fri, 21 Aug 2020 12:37:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jQURKxDjRWMiMTxRUVV2iQZ2ZeobDW7AH50DwoJj7f8=; b=sD6uHtx5WmZbLrlelrAxJJlU8yrwBa7XdhUItXD/MUiuplcXnFHNbs0uCDwUQgzFtN jurCNLEA/E4vr3a4yeIje3FqyDQI8sRF2c9YCo0QgyXFJuEqVPmkW2lS0/NsyrCgrxja tHGI+6w07Ab8+AjC5II2mNiy2hdZ+3qmZyA6KHRLmlNmhG0wcFb6d/3S35p6poEvsoRd vlwL/zS/4H3Jx2D9+lvUJjrS4TpaS1QO3eL2d4amih7Ujdzt9QbFU97rAayMqUXL50px JXF3OtgHHiAAnV5emKHZq4o9IjgjW6eSdpoPKf4Z/6Md2pN6nt5uPCWgvaPZOR/vzBsW mzSw== 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=jQURKxDjRWMiMTxRUVV2iQZ2ZeobDW7AH50DwoJj7f8=; b=McjT6N5kVUtvA4qqGtU7M/zb3BUN6xdQQfYzhIdKM8pEGNsD+xdXG0wVMaEWcuuJc3 OGj33BZbXK10DgoYRGhx4dBvrfNtLVsaU09jmbA1VeBZP6SgjQhkHR6w64s/VXmPpvzW vW90EFRTD71KwouPpiu7gzYRMHLseisC7eqC2z0vysDB5EwQlnlwKY+zQmLopuzNYr+n JjVJnk4xAIwtiEDx01mnLYHKAwn6Fhi9Lr213wyoaaY+45XVJKZZwv74U98FZuJm9Bx1 UJHupFeORL7i5cXP3N8zQwlUrxm2WxWsJ+LYWVYrpLesO1eBKsuV3A1kbh1HQX+kSCcV iiow== X-Gm-Message-State: AOAM5328cvUP01agvfhuQiXVUErb2g8cYx2qrpI+ZKcVNkwHoh613aQr wKPoI2cVGACqk1f2If6ruuTo4Bf8F36FxrGEK61d X-Google-Smtp-Source: ABdhPJxv9ljf/EC5lZy/k36EEbz/l4zL+9SOFpsYjODOPjAFI0K0Ji1ACIURRzkOqqw0PrvW8Opqs3fti74vNH7Daus= X-Received: by 2002:a17:906:43c9:: with SMTP id j9mr4383574ejn.542.1598038620869; Fri, 21 Aug 2020 12:37:00 -0700 (PDT) MIME-Version: 1.0 References: <20200729200545.5apwc7fashwsnglj@madcap2.tricolour.ca> In-Reply-To: <20200729200545.5apwc7fashwsnglj@madcap2.tricolour.ca> From: Paul Moore Date: Fri, 21 Aug 2020 15:36:49 -0400 Message-ID: Subject: Re: [PATCH ghak90 V9 02/13] audit: add container id To: Richard Guy Briggs Cc: containers@lists.linux-foundation.org, linux-api@vger.kernel.org, Linux-Audit Mailing List , linux-fsdevel@vger.kernel.org, LKML , netdev@vger.kernel.org, netfilter-devel@vger.kernel.org, sgrubb@redhat.com, Ondrej Mosnacek , dhowells@redhat.com, simo@redhat.com, Eric Paris , Serge Hallyn , ebiederm@xmission.com, nhorman@tuxdriver.com, Dan Walsh , mpatel@redhat.com 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 Wed, Jul 29, 2020 at 4:06 PM Richard Guy Briggs wrote: > On 2020-07-05 11:09, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote: ... > > > @@ -212,6 +219,33 @@ void __init audit_task_init(void) > > > 0, SLAB_PANIC, NULL); > > > } > > > > > > +/* rcu_read_lock must be held by caller unless new */ > > > +static struct audit_contobj *_audit_contobj_hold(struct audit_contobj *cont) > > > +{ > > > + if (cont) > > > + refcount_inc(&cont->refcount); > > > + return cont; > > > +} > > > + > > > +static struct audit_contobj *_audit_contobj_get(struct task_struct *tsk) > > > +{ > > > + if (!tsk->audit) > > > + return NULL; > > > + return _audit_contobj_hold(tsk->audit->cont); > > > +} > > > + > > > +/* rcu_read_lock must be held by caller */ > > > +static void _audit_contobj_put(struct audit_contobj *cont) > > > +{ > > > + if (!cont) > > > + return; > > > + if (refcount_dec_and_test(&cont->refcount)) { > > > + put_task_struct(cont->owner); > > > + list_del_rcu(&cont->list); > > > > You should check your locking; I'm used to seeing exclusive locks > > (e.g. the spinlock) around list adds/removes, it just reads/traversals > > that can be done with just the RCU lock held. > > Ok, I've redone the locking yet again. I knew this on one level but > that didn't translate consistently to code... > > > > + kfree_rcu(cont, rcu); > > > + } > > > +} > > > > Another nitpick, but it might be nice to have similar arguments to the > > _get() and _put() functions, e.g. struct audit_contobj, but that is > > some serious bikeshedding (basically rename _hold() to _get() and > > rename _hold to audit_task_contid_hold() or similar). > > I have some idea what you are trying to say, but I think you misspoke. > Did you mean rename _hold to _get, rename _get to > audit_task_contobj_hold()? It reads okay to me, but I know what I'm intending here :) I agree it could be a bit confusing. Let me try to put my suggestion into some quick pseudo-code function prototypes to make things a bit more concrete. The _audit_contobj_hold() function would become: struct audit_contobj *_audit_contobj_hold(struct task_struct *tsk); The _audit_contobj_get() function would become: struct audit_contobj *_audit_contobj_get(struct audit_contobj *cont); The _audit_contobj_put() function would become: void _audit_contobj_put(struct audit_contobj *cont); Basically swap the _get() and _hold() function names so that the arguments are the same for both the _get() and _set() functions. Does this make more sense? > > > /** > > > * audit_alloc - allocate an audit info block for a task > > > * @tsk: task > > > @@ -232,6 +266,9 @@ int audit_alloc(struct task_struct *tsk) > > > } > > > info->loginuid = audit_get_loginuid(current); > > > info->sessionid = audit_get_sessionid(current); > > > + rcu_read_lock(); > > > + info->cont = _audit_contobj_get(current); > > > + rcu_read_unlock(); > > > > The RCU locks aren't strictly necessary here, are they? In fact I > > suppose we could probably just replace the _get() call with a > > refcount_set(1) just as we do in audit_set_contid(), yes? > > I don't understand what you are getting at here. It needs a *contobj, > along with bumping up the refcount of the existing contobj. Sorry, you can disregard. My mental definition for audit_alloc() is permanently messed up; I usually double check myself before commenting on related code, but I must have forgotten here. -- paul moore www.paul-moore.com 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=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 0240EC433DF for ; Fri, 21 Aug 2020 19:37:20 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9D43C2076E for ; Fri, 21 Aug 2020 19:37:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9D43C2076E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=paul-moore.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linux-audit-bounces@redhat.com Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-319-jBDYb8dANuuzZwxmWllvxA-1; Fri, 21 Aug 2020 15:37:16 -0400 X-MC-Unique: jBDYb8dANuuzZwxmWllvxA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 21EA71005E5E; Fri, 21 Aug 2020 19:37:13 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 61D6C5C1D7; Fri, 21 Aug 2020 19:37:12 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id CA1D8662BD; Fri, 21 Aug 2020 19:37:10 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 07LJb6RB013770 for ; Fri, 21 Aug 2020 15:37:07 -0400 Received: by smtp.corp.redhat.com (Postfix) id DC13E2022795; Fri, 21 Aug 2020 19:37:06 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast06.extmail.prod.ext.rdu2.redhat.com [10.11.55.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D795820110C9 for ; Fri, 21 Aug 2020 19:37:04 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 892DC1815DC4 for ; Fri, 21 Aug 2020 19:37:04 +0000 (UTC) Received: from mail-ej1-f68.google.com (mail-ej1-f68.google.com [209.85.218.68]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-141-BOKQiZ3SNbW39XW9UVG6kQ-1; Fri, 21 Aug 2020 15:37:02 -0400 X-MC-Unique: BOKQiZ3SNbW39XW9UVG6kQ-1 Received: by mail-ej1-f68.google.com with SMTP id m22so3668880eje.10 for ; Fri, 21 Aug 2020 12:37:01 -0700 (PDT) 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=jQURKxDjRWMiMTxRUVV2iQZ2ZeobDW7AH50DwoJj7f8=; b=MStiPkGrodYMl2QM8uckGl+gm3urgAqaCbBl/dMqtxzT7c8Hn/zpY8v1tfZERAIVPp IG7TxO3ovEQMaY5m68poaHyfXw9UdLaXecEjkxYllz67d11VPCwhz91nsJmgNlvV3mIp hjAZRsH7vtRHNXep+J6rTVie5/IPhVc6LAYLskMpBEZiSkJZYFedUNqEFQlmZCu/Hiay u0lcg8cO1wRPisA178ee/kZoSkmNzJjBc9fIp+iCJSAeC6YV530tZQldwU04q9BQHb4h 3Uufdu1dLOc2/5K2vV2ej8VYmkOm1MdPaWyTV/lRqsgEI2K/qTq67wEkQJzxiF315svK +1tg== X-Gm-Message-State: AOAM533A7Xex8m8kVO5E+r0bMN0UgM79f/7z4t2wHdMLp4VRdN+fak1f 2yElf9367PChB9L5ezKvUtdRm/JTC9iSnR9cOTAp X-Google-Smtp-Source: ABdhPJxv9ljf/EC5lZy/k36EEbz/l4zL+9SOFpsYjODOPjAFI0K0Ji1ACIURRzkOqqw0PrvW8Opqs3fti74vNH7Daus= X-Received: by 2002:a17:906:43c9:: with SMTP id j9mr4383574ejn.542.1598038620869; Fri, 21 Aug 2020 12:37:00 -0700 (PDT) MIME-Version: 1.0 References: <20200729200545.5apwc7fashwsnglj@madcap2.tricolour.ca> In-Reply-To: <20200729200545.5apwc7fashwsnglj@madcap2.tricolour.ca> From: Paul Moore Date: Fri, 21 Aug 2020 15:36:49 -0400 Message-ID: Subject: Re: [PATCH ghak90 V9 02/13] audit: add container id To: Richard Guy Briggs X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false; X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-loop: linux-audit@redhat.com Cc: nhorman@tuxdriver.com, linux-api@vger.kernel.org, containers@lists.linux-foundation.org, LKML , dhowells@redhat.com, Linux-Audit Mailing List , netfilter-devel@vger.kernel.org, ebiederm@xmission.com, simo@redhat.com, netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org, Eric Paris , mpatel@redhat.com, Serge Hallyn X-BeenThere: linux-audit@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Linux Audit Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=linux-audit-bounces@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Wed, Jul 29, 2020 at 4:06 PM Richard Guy Briggs wrote: > On 2020-07-05 11:09, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote: ... > > > @@ -212,6 +219,33 @@ void __init audit_task_init(void) > > > 0, SLAB_PANIC, NULL); > > > } > > > > > > +/* rcu_read_lock must be held by caller unless new */ > > > +static struct audit_contobj *_audit_contobj_hold(struct audit_contobj *cont) > > > +{ > > > + if (cont) > > > + refcount_inc(&cont->refcount); > > > + return cont; > > > +} > > > + > > > +static struct audit_contobj *_audit_contobj_get(struct task_struct *tsk) > > > +{ > > > + if (!tsk->audit) > > > + return NULL; > > > + return _audit_contobj_hold(tsk->audit->cont); > > > +} > > > + > > > +/* rcu_read_lock must be held by caller */ > > > +static void _audit_contobj_put(struct audit_contobj *cont) > > > +{ > > > + if (!cont) > > > + return; > > > + if (refcount_dec_and_test(&cont->refcount)) { > > > + put_task_struct(cont->owner); > > > + list_del_rcu(&cont->list); > > > > You should check your locking; I'm used to seeing exclusive locks > > (e.g. the spinlock) around list adds/removes, it just reads/traversals > > that can be done with just the RCU lock held. > > Ok, I've redone the locking yet again. I knew this on one level but > that didn't translate consistently to code... > > > > + kfree_rcu(cont, rcu); > > > + } > > > +} > > > > Another nitpick, but it might be nice to have similar arguments to the > > _get() and _put() functions, e.g. struct audit_contobj, but that is > > some serious bikeshedding (basically rename _hold() to _get() and > > rename _hold to audit_task_contid_hold() or similar). > > I have some idea what you are trying to say, but I think you misspoke. > Did you mean rename _hold to _get, rename _get to > audit_task_contobj_hold()? It reads okay to me, but I know what I'm intending here :) I agree it could be a bit confusing. Let me try to put my suggestion into some quick pseudo-code function prototypes to make things a bit more concrete. The _audit_contobj_hold() function would become: struct audit_contobj *_audit_contobj_hold(struct task_struct *tsk); The _audit_contobj_get() function would become: struct audit_contobj *_audit_contobj_get(struct audit_contobj *cont); The _audit_contobj_put() function would become: void _audit_contobj_put(struct audit_contobj *cont); Basically swap the _get() and _hold() function names so that the arguments are the same for both the _get() and _set() functions. Does this make more sense? > > > /** > > > * audit_alloc - allocate an audit info block for a task > > > * @tsk: task > > > @@ -232,6 +266,9 @@ int audit_alloc(struct task_struct *tsk) > > > } > > > info->loginuid = audit_get_loginuid(current); > > > info->sessionid = audit_get_sessionid(current); > > > + rcu_read_lock(); > > > + info->cont = _audit_contobj_get(current); > > > + rcu_read_unlock(); > > > > The RCU locks aren't strictly necessary here, are they? In fact I > > suppose we could probably just replace the _get() call with a > > refcount_set(1) just as we do in audit_set_contid(), yes? > > I don't understand what you are getting at here. It needs a *contobj, > along with bumping up the refcount of the existing contobj. Sorry, you can disregard. My mental definition for audit_alloc() is permanently messed up; I usually double check myself before commenting on related code, but I must have forgotten here. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit