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

[Chuck Lever <cel@kernel.org>]

On 10/16/25 2:02 PM, Eric Biggers wrote:
> 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?

Agreed, that's possible. My concern right now is that there are enough
testing gaps (simply because Kconfig makes the test matrix exponentially
large) that I don't have confidence that we can come up with a good fix
and get it broadly tested before v6.18 final.

I am, however, happy to continue discussing ways to improve the menu
and the settings it selects.


-- 
Chuck Lever