Received-SPF: pass (google.com: domain of linux-crypto+bounces-2206-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 20 Feb 2024 18:03:02 +0000
Message-Id: <CZA3R5R9CVYD.1HH1S662FW2RX@seitikki>
Cc: <keyrings@vger.kernel.org>, <linux-crypto@vger.kernel.org>, "Andy
 Shevchenko" <andriy.shevchenko@linux.intel.com>, "Peter Zijlstra"
 <peterz@infradead.org>, "Dan Williams" <dan.j.williams@intel.com>, "Ard
 Biesheuvel" <ardb@kernel.org>, "Nick Desaulniers"
 <ndesaulniers@google.com>, "Nathan Chancellor" <nathan@kernel.org>
Subject: Re: [PATCH v3] X.509: Introduce scope-based x509_certificate
 allocation
From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Jarkko Sakkinen" <jarkko@kernel.org>, "Lukas Wunner" <lukas@wunner.de>,
 "David Howells" <dhowells@redhat.com>, "Herbert Xu"
 <herbert@gondor.apana.org.au>, "David S. Miller" <davem@davemloft.net>,
 "Jonathan Cameron" <Jonathan.Cameron@huawei.com>
References: <63cc7ab17a5064756e26e50bc605e3ff8914f05a.1708439875.git.lukas@wunner.de> <CZA3PCY3U4YU.3R05ZC4X16EX0@seitikki>
In-Reply-To: <CZA3PCY3U4YU.3R05ZC4X16EX0@seitikki>

On Tue Feb 20, 2024 at 6:00 PM UTC, Jarkko Sakkinen wrote:
> On Tue Feb 20, 2024 at 3:10 PM UTC, Lukas Wunner wrote:
> > Add a DEFINE_FREE() clause for x509_certificate structs and use it in
> > x509_cert_parse() and x509_key_preparse().  These are the only function=
s
> > where scope-based x509_certificate allocation currently makes sense.
> > A third user will be introduced with the forthcoming SPDM library
> > (Security Protocol and Data Model) for PCI device authentication.
>
> I think you are adding scope-based memory management and not
> DEFINE_FREE(). Otherwise, this would be one-liner patch.
>
> I'm not sure if the last sentence adds more than clutter as this
> patch has nothing to do with SPDM changes per se.
>
> > Unlike most other DEFINE_FREE() clauses, this one checks for IS_ERR()
> > instead of NULL before calling x509_free_certificate() at end of scope.
> > That's because the "constructor" of x509_certificate structs,
> > x509_cert_parse(), returns a valid pointer or an ERR_PTR(), but never
> > NULL.
> >
> > I've compared the Assembler output before/after and they are identical,
> > save for the fact that gcc-12 always generates two return paths when
> > __cleanup() is used, one for the success case and one for the error cas=
e.
>
> Use passive as commit message is not a personal letter.
>
> >
> > In x509_cert_parse(), add a hint for the compiler that kzalloc() never
> > returns an ERR_PTR().  Otherwise the compiler adds a gratuitous IS_ERR(=
)
> > check on return.  Introduce a handy assume() macro for this which can b=
e
> > re-used elsewhere in the kernel to provide hints for the compiler.
>
> Does not explain why it is "handy".
>
> I don't see a story here but instead I see bunch of disordered tecnical
> terms.
>
> We have the code diff for detailed technical stuff. The commit message
> should simply explain why we want this and what it does for us. And we
> zero care about PCI changes in the scope of this patch, especially since
> this is not part of such patch set.

I mean think it this way.

What is the most important function of a commit message? Well, it comes
when the commit is in the mainline. It reminds of the *reasons* why a
change was made and this commit message does not really serve well in
that role.

BR, Jarkko