MIME-Version: 1.0
References: <20211223141113.1240679-1-Jason@zx2c4.com>
In-Reply-To: <20211223141113.1240679-1-Jason@zx2c4.com>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Sat, 25 Dec 2021 18:26:51 +0900
Message-ID: <CAK7LNARjb4=9JOVDdfUg1sxLG4OH-Eko44iEewbr+0Wd+CNf1Q@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] lib/crypto: blake2s: include as built-in
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Theodore Ts'o" <tytso@mit.edu>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 23, 2021 at 11:11 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> In preparation for using blake2s in the RNG, we change the way that it
> is wired-in to the build system. Instead of kconfig mazes and ifdefs, we
> use weak symbols, so that an arch version can override the generic
> version. Then we include the generic version in lib-y, so that it can be
> removed from the image if the arch version doesn't fallback to it (as is
> the case on arm though not x86).


As I replied in another email, this does not work like that.

Since 7273ad2b08f8ac9563579d16a3cf528857b26f49,
libs-y are all linked when CONFIG_MODULES=y.


So, what this patch is doing are:

 - Add __weak to the generic function
 - Make modules into built-in.


Both generic functions and ARM-specific ones
will remain in vmlinux.

__weak makes it difficult to track which function is
actually used.
Using #ifdef CONFIG_* (as the current code does)
is better.


>
> diff --git a/Makefile b/Makefile
> index d85f1ff79f5c..892ea632ea63 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -668,7 +668,7 @@ drivers-y   := drivers/ sound/
>  drivers-$(CONFIG_SAMPLES) += samples/
>  drivers-$(CONFIG_NET) += net/
>  drivers-y      += virt/
> -libs-y         := lib/
> +libs-y         := lib/ lib/crypto/


If this is merged, someone will try to
add random patterns.
libs-y         := lib/ lib/crypto/ lib/foo/bar/ lib/foo/ba/baz


lib-y and libs-y are a bad idea in the first place
and should not be extended any more.

Since this patch is not working as the commit description
claims, and it is going in the bad direction, so

NACK


-- 
Best Regards
Masahiro Yamada