Re: [GIT PULL] NFSD changes for v6.18

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Oct 16, 2025 at 11:19:24AM -0400, Trond Myklebust wrote:
> On Thu, 2025-10-16 at 11:04 -0400, Chuck Lever wrote:
> > On 10/16/25 10:36 AM, Geert Uytterhoeven wrote:
> > > Hi Jeff,
> > > 
> > > On Thu, 16 Oct 2025 at 16:31, Jeff Layton <jlayton@kernel.org>
> > > wrote:
> > > > On Mon, 2025-10-13 at 15:37 -0400, Chuck Lever wrote:
> > > > > On 10/13/25 3:21 PM, Eric Biggers wrote:
> > > > > > On Mon, Oct 13, 2025 at 12:15:52PM +0200, Geert Uytterhoeven
> > > > > > wrote:
> > > > > > > Hi Chuck, Eric,
> > > > > > > 
> > > > > > > On Wed, 8 Oct 2025 at 00:05, Chuck Lever <cel@kernel.org>
> > > > > > > wrote:
> > > > > > > > Eric Biggers (4):
> > > > > > > >       SUNRPC: Make RPCSEC_GSS_KRB5 select CRYPTO instead
> > > > > > > > of depending on it
> > > > > > > 
> > > > > > > This is now commit d8e97cc476e33037 ("SUNRPC: Make
> > > > > > > RPCSEC_GSS_KRB5
> > > > > > > select CRYPTO instead of depending on it") in v6.18-rc1.
> > > > > > > As RPCSEC_GSS_KRB5 defaults to "y", CRYPTO is now auto-
> > > > > > > enabled in
> > > > > > > defconfigs that didn't enable it before.
> > > > > > 
> > > > > > Now the config is:
> > > > > > 
> > > > > >     config RPCSEC_GSS_KRB5
> > > > > >         tristate "Secure RPC: Kerberos V mechanism"
> > > > > >         depends on SUNRPC
> > > > > >         default y
> > > > > >         select SUNRPC_GSS
> > > > > >         select CRYPTO
> > > > > >         select CRYPTO_SKCIPHER
> > > > > >         select CRYPTO_HASH
> > > > > > 
> > > > > > Perhaps the 'default y' should be removed?
> > > > > > 
> > > > > > Chuck, do you know why it's there?
> > > > > The "default y" was added by 2010 commit df486a25900f ("NFS:
> > > > > Fix the
> > > > > selection of security flavours in Kconfig"), then modified
> > > > > again by
> > > > > commit e3b2854faabd ("SUNRPC: Fix the SUNRPC Kerberos V
> > > > > RPCSEC_GSS
> > > > > module dependencies") in 2011.
> > > > > 
> > > > > Copying Trond, the author of both of those patches.
> > > > 
> > > > Looking at this a bit closer, maybe a patch like this is what we
> > > > want?
> > > > This should make it so that we only enable RPCSEC_GSS_KRB5 if
> > > > CRYPTO is
> > > > already enabled:
> > > > 
> > > > diff --git a/net/sunrpc/Kconfig b/net/sunrpc/Kconfig
> > > > index 984e0cf9bf8a..d433626c7917 100644
> > > > --- a/net/sunrpc/Kconfig
> > > > +++ b/net/sunrpc/Kconfig
> > > > @@ -19,9 +19,8 @@ config SUNRPC_SWAP
> > > >  config RPCSEC_GSS_KRB5
> > > >         tristate "Secure RPC: Kerberos V mechanism"
> > > >         depends on SUNRPC
> > > > -       default y
> > > > +       default y if CRYPTO
> > > 
> > > This merely controls the default, the user can still override it.
> > > Implementing your suggestion above would mean re-adding "depends on
> > > CRYPTO", i.e. reverting commit d8e97cc476e33037.
> > > 
> > > >         select SUNRPC_GSS
> > > > -       select CRYPTO
> > > >         select CRYPTO_SKCIPHER
> > > >         select CRYPTO_HASH
> > > >         help
> > > 
> > > Gr{oetje,eeting}s,
> > > 
> > >                         Geert
> > > 
> > 
> > The graph of dependencies and selects between NFS, NFSD, and SUNRPC
> > is
> > brittle, unfortunately. I suggest reverting d8e97cc476e33037 for now
> > while a proper solution is worked out and then tested.
> > 
> 
> Yes. The reason why I went for the weaker 'default y if ...' and
> 'depends on ...' is precisely because 'select' is so brittle, and at
> the time others advised against using it for more complicated
> situations such as this. The crypto code has a number of dependencies,
> and those have been known to change both over time and across hardware
> platforms.

CRYPTO doesn't have any dependencies.  As I documented in the commit
itself, CRYPTO is normally selected rather than depended on.  Similar to
how e.g. this option (RPCSEC_GSS_KRB5) already selected CRYPTO_SKCIPHER
and CRYPTO_HASH rather than depending on them.  It doesn't really make
sense to handle these options differently.

The real issue is RPCSEC_GSS_KRB5 being 'default y'.  The nfs folks
should make a decision about whether they want that or not.

I'll also that NFSD_V4 already selects RPCSEC_GSS_KRB5.  Perhaps that
already achieves what the 'default y' may have been intended to achieve?

- Eric