[GIT PULL] NFSD changes for v6.18

[GIT PULL] NFSD changes for v6.18

[Chuck Lever]

Hi Linus-

One potential merge conflict has been reported for nfsd-6.18.
See the following URLs for recommended resolution:

https://lore.kernel.org/lkml/aN5dMYUPfFly6eUO@sirena.org.uk/

or

https://lore.kernel.org/linux-nfs/aNxL88GmEzJ5hsHl@kernel.org/


--- cut here ---

The following changes since commit 07e27ad16399afcd693be20211b0dfae63e0615f:

  Linux 6.17-rc7 (2025-09-21 15:08:52 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git tags/nfsd-6.18

for you to fetch changes up to 73cc6ec1a89a6c443a77b9b93ddcea63b7cea223:

  nfsd: discard nfserr_dropit (2025-10-01 15:54:01 -0400)

----------------------------------------------------------------
NFSD 6.18 Release Notes

Mike Snitzer has prototyped a mechanism for disabling I/O caching in
NFSD. This is introduced in v6.18 as an experimental feature. This
enables scaling NFSD in /both/ directions:

- NFS service can be supported on systems with small memory
  footprints, such as low-cost cloud instances
- Large NFS workloads will be less likely to force the eviction of
  server-local activity, helping it avoid thrashing

Jeff Layton contributed a number of fixes to the new attribute
delegation implementation (based on a pending Internet RFC) that we
hope will make attribute delegation reliable enough to enable by
default, as it is on the Linux NFS client.

The remaining patches in this pull request are clean-ups and minor
optimizations. Many thanks to the contributors, reviewers, testers,
and bug reporters who participated during the v6.18 NFSD development
cycle.

----------------------------------------------------------------
Chuck Lever (7):
      NFSD: Relocate the fh_want_write() and fh_drop_write() helpers
      NFSD: Move the fh_getattr() helper
      NFS: Remove rpcbind cleanup for NFSv4.0 callback
      SUNRPC: Move the svc_rpcb_cleanup() call sites
      NFSD: Delay adding new entries to LRU
      NFSD: Reduce DRC bucket size
      NFSD: Do the grace period check in ->proc_layoutget

Colin Ian King (1):
      lockd: Remove space before newline

Dan Carpenter (1):
      nfsd: delete unnecessary NULL check in __fh_verify()

Eric Biggers (4):
      nfsd: Replace open-coded conversion of bytes to hex
      nfsd: Eliminate an allocation in nfs4_make_rec_clidname()
      nfsd: Don't force CRYPTO_LIB_SHA256 to be built-in
      SUNRPC: Make RPCSEC_GSS_KRB5 select CRYPTO instead of depending on it

Jeff Layton (11):
      sunrpc: delay pc_release callback until after the reply is sent
      nfsd: fix assignment of ia_ctime.tv_nsec on delegated mtime update
      nfsd: ignore ATTR_DELEG when checking ia_valid before notify_change()
      vfs: add ATTR_CTIME_SET flag
      nfsd: use ATTR_CTIME_SET for delegated ctime updates
      nfsd: track original timestamps in nfs4_delegation
      nfsd: fix SETATTR updates for delegated timestamps
      nfsd: fix timestamp updates in CB_GETATTR
      nfsd: freeze c/mtime updates with outstanding WRITE_ATTRS delegation
      sunrpc: fix pr_notice in svc_tcp_sendto() to show correct length
      sunrpc: eliminate return pointer in svc_tcp_sendmsg()

Lei Lu (1):
      sunrpc: fix null pointer dereference on zero-length checksum

Mike Snitzer (1):
      NFSD: Add io_cache_{read,write} controls to debugfs

NeilBrown (2):
      nfsd: discard nfsd_file_get_local()
      nfsd: discard nfserr_dropit

Olga Kornievskaia (2):
      nfsd: unregister with rpcbind when deleting a transport
      nfsd: nfserr_jukebox in nlm_fopen should lead to a retry

Scott Mayhew (1):
      nfsd: decouple the xprtsec policy check from check_nfsd_access()

Sergey Bashirov (8):
      sunrpc: Change ret code of xdr_stream_decode_opaque_fixed
      NFSD: Rework encoding and decoding of nfsd4_deviceid
      NFSD: Minor cleanup in layoutcommit processing
      NFSD: Minor cleanup in layoutcommit decoding
      NFSD: Implement large extent array support in pNFS
      NFSD: Fix last write offset handling in layoutcommit
      NFSD: Disallow layoutget during grace period
      NFSD: Allow layoutcommit during grace period

Thorsten Blum (1):
      NFSD: Fix destination buffer size in nfsd4_ssc_setup_dul()

Xichao Zhao (2):
      NFSD: Drop redundant conversion to bool
      sunrpc: fix "occurence"->"occurrence"

 fs/attr.c                                          |  44 +++----
 fs/lockd/svc.c                                     |   6 +-
 fs/lockd/svclock.c                                 |   2 +-
 fs/nfs/callback.c                                  |  10 +-
 fs/nfsd/Kconfig                                    |   2 +-
 fs/nfsd/blocklayout.c                              |  32 +++---
 fs/nfsd/blocklayoutxdr.c                           |  86 +++++++++-----
 fs/nfsd/blocklayoutxdr.h                           |   4 +-
 fs/nfsd/debugfs.c                                  |  95 ++++++++++++++-
 fs/nfsd/export.c                                   |  82 +++++++++----
 fs/nfsd/export.h                                   |   3 +
 fs/nfsd/filecache.c                                |  21 ----
 fs/nfsd/filecache.h                                |   1 -
 fs/nfsd/flexfilelayout.c                           |   4 +-
 fs/nfsd/flexfilelayoutxdr.c                        |   3 +-
 fs/nfsd/localio.c                                  |   1 -
 fs/nfsd/lockd.c                                    |  15 ++-
 fs/nfsd/nfs4layouts.c                              |   1 -
 fs/nfsd/nfs4proc.c                                 | 127 +++++++++++++++------
 fs/nfsd/nfs4recover.c                              |  31 +----
 fs/nfsd/nfs4state.c                                |  86 ++++++++++----
 fs/nfsd/nfs4xdr.c                                  |  32 ++----
 fs/nfsd/nfscache.c                                 |  15 +--
 fs/nfsd/nfsctl.c                                   |   2 +-
 fs/nfsd/nfsd.h                                     |  17 +--
 fs/nfsd/nfsfh.c                                    |  51 ++++++++-
 fs/nfsd/nfsfh.h                                    |  38 ++++++
 fs/nfsd/nfssvc.c                                   |   7 +-
 fs/nfsd/pnfs.h                                     |   5 +-
 fs/nfsd/state.h                                    |  16 ++-
 fs/nfsd/vfs.c                                      |  23 +++-
 fs/nfsd/vfs.h                                      |  33 ------
 fs/nfsd/xdr4.h                                     |  39 ++++++-
 include/linux/fs.h                                 |   1 +
 include/linux/nfslocalio.h                         |   1 -
 include/linux/sunrpc/svc_xprt.h                    |   6 +-
 include/linux/sunrpc/xdr.h                         |   4 +-
 net/sunrpc/Kconfig                                 |   3 +-
 net/sunrpc/auth_gss/svcauth_gss.c                  |   2 +-
 net/sunrpc/svc.c                                   |  18 ++-
 net/sunrpc/svc_xprt.c                              |  20 +++-
 net/sunrpc/svcsock.c                               |  25 ++--
 net/sunrpc/sysfs.c                                 |   2 +-
 .../C/typedef/decoder/fixed_length_opaque.j2       |   2 +-
 44 files changed, 678 insertions(+), 340 deletions(-)

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

[Linus Torvalds]

On Mon, 6 Oct 2025 at 06:50, Chuck Lever <cel@kernel.org> wrote:
>
> One potential merge conflict has been reported for nfsd-6.18.

No problem, this is the simple kind of explicit conflict (famous last
words before I mess one of those things up).

Anyway, the reason I'm replying is actually that I notice that you
added that ATTR_CTIME_SET flag in <linux/fs.h> in commit afc5b36e29b9
("vfs: add ATTR_CTIME_SET flag").

No complaints about it, but it looks a bit odd with ATTR_{A,M}TIME_SET
in bits 7 and 8, and then the new ATTR_CTIME_SET is in bit 10 with the
entirely unrelated ATTR_FORCE in between them all.

So I'm thinking it would look cleaner if we just swapped
ATTR_CTIME_SET and ATTR_FORCE around - these are all just our own
kernel-internal bits (and the reason bit 10 was unused is that it used
to contain the odd ATTR_ATTR_FLAG that was never used).

Danger Will Robinson: hostfs has odd duplicate copies of all these, including a

   #define HOSTFS_ATTR_ATTR_FLAG   1024

of that no-longer existing flag.

But hostfs doesn't use ATTR_FORCE (aka HOSTFS_ATTR_FORCE), so
switching those two bits around wouldn't affect it either, even if you
were to have a version mismatch between the client and host when doing
UML (which I don't know

Adding Christian to the participants list, because I did *not* do that
cleanup thing myself, because I'm slightly worried that I'm missing
something. But it would seem to be a good thing to do just to have the
numbering make more sense, and Christian is probably the right person.

And adding Johannes Berg due to the UML connection, just to see that I
haven't misread that odd hostfs situation.

Comments?

            Linus

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

[Chuck Lever]

On 10/6/25 4:51 PM, Linus Torvalds wrote:
> On Mon, 6 Oct 2025 at 06:50, Chuck Lever <cel@kernel.org> wrote:
>>
>> One potential merge conflict has been reported for nfsd-6.18.
> 
> No problem, this is the simple kind of explicit conflict (famous last
> words before I mess one of those things up).
> 
> Anyway, the reason I'm replying is actually that I notice that you
> added that ATTR_CTIME_SET flag in <linux/fs.h> in commit afc5b36e29b9
> ("vfs: add ATTR_CTIME_SET flag").
> 
> No complaints about it, but it looks a bit odd with ATTR_{A,M}TIME_SET
> in bits 7 and 8, and then the new ATTR_CTIME_SET is in bit 10 with the
> entirely unrelated ATTR_FORCE in between them all.

Oof. We should have gotten Acks for "vfs: add ATTR_CTIME_SET flag". My
bad.


> So I'm thinking it would look cleaner if we just swapped
> ATTR_CTIME_SET and ATTR_FORCE around - these are all just our own
> kernel-internal bits (and the reason bit 10 was unused is that it used
> to contain the odd ATTR_ATTR_FLAG that was never used).
> 
> Danger Will Robinson: hostfs has odd duplicate copies of all these, including a
> 
>    #define HOSTFS_ATTR_ATTR_FLAG   1024
> 
> of that no-longer existing flag.
> 
> But hostfs doesn't use ATTR_FORCE (aka HOSTFS_ATTR_FORCE), so
> switching those two bits around wouldn't affect it either, even if you
> were to have a version mismatch between the client and host when doing
> UML (which I don't know
> 
> Adding Christian to the participants list, because I did *not* do that
> cleanup thing myself, because I'm slightly worried that I'm missing
> something. But it would seem to be a good thing to do just to have the
> numbering make more sense, and Christian is probably the right person.
> 
> And adding Johannes Berg due to the UML connection, just to see that I
> haven't misread that odd hostfs situation.
> 
> Comments?
> 
>             Linus


-- 
Chuck Lever

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

[pr-tracker-bot]

The pull request you sent on Mon,  6 Oct 2025 09:50:10 -0400:

> https://lore.kernel.org/lkml/aN5dMYUPfFly6eUO@sirena.org.uk/ or

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/81538c8e42806eed71ce125723877a7c2307370c

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[Johannes Berg]

On Mon, 2025-10-06 at 13:51 -0700, Linus Torvalds wrote:
> Danger Will Robinson: hostfs has odd duplicate copies of all these, including a
> 
>    #define HOSTFS_ATTR_ATTR_FLAG   1024
> 
> of that no-longer existing flag.
> 
> But hostfs doesn't use ATTR_FORCE (aka HOSTFS_ATTR_FORCE), so
> switching those two bits around wouldn't affect it either, even if you
> were to have a version mismatch between the client and host when doing
> UML (which I don't know
> 
> Adding Christian to the participants list, because I did *not* do that
> cleanup thing myself, because I'm slightly worried that I'm missing
> something. But it would seem to be a good thing to do just to have the
> numbering make more sense, and Christian is probably the right person.

That indeed looks messy in hostfs; I'm not really all _that_ familiar
with all the details of it, but the stated reason is that it needs to
have the defines in kernel and user code.

That doesn't mean it's between the host and guest kernels, it's just
between code built for "userspace" and code built for "kernel", both of
which go into the UML linux "guest" binary. IOW, it's just for
communication between hostfs_kern.c and hostfs_user.c, which nobody else
needs to care about.

And as long as hostfs doesn't propagate CTIME_SET from the host to the
guest, it doesn't need a HOSTFS_ATTR_CTIME_SET. No idea why
HOSTFS_ATTR_ATTR_FLAG was even defined, it's ancient anyway.

However ... it looks like hostfs_kern.c is using ATTR_* in some places,
and hostfs_user.c is using HOSTFS_ATTR_*, so it looks like right now
these *do* need to match. Given that, we should just generate them via
asm-offsets.h, like we do for other constants with the property of being
needed on both sides but defined in places that cannot be included into
user-side code, like this:


From: Johannes Berg <johannes.berg@intel.com>
Date: Mon, 6 Oct 2025 23:14:36 +0200
Subject: [PATCH] um/hostfs: define HOSTFS_ATTR_* via asm-offsets

The HOSTFS_ATTR_* values were meant to be standalone for
communication between hostfs's kernel and user code parts.
However, it's easy to forget that HOSTFS_ATTR_* should be
used even on the kernel side, and that wasn't consistently
done. As a result, the values need to match ATTR_* values,
which is not useful to maintain by hand. Instead, generate
them via asm-offsets like other constants that UML needs
in user-side code that aren't otherwise available in any
header files that can be included there.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/um/include/shared/common-offsets.h    | 10 +++++++
 arch/x86/um/shared/sysdep/kernel-offsets.h |  1 +
 fs/hostfs/hostfs.h                         | 34 +---------------------
 3 files changed, 12 insertions(+), 33 deletions(-)

diff --git a/arch/um/include/shared/common-offsets.h b/arch/um/include/shared/common-offsets.h
index 8ca66a1918c3..fcec75a93e0c 100644
--- a/arch/um/include/shared/common-offsets.h
+++ b/arch/um/include/shared/common-offsets.h
@@ -18,3 +18,13 @@ DEFINE(UM_NSEC_PER_USEC, NSEC_PER_USEC);
 DEFINE(UM_KERN_GDT_ENTRY_TLS_ENTRIES, GDT_ENTRY_TLS_ENTRIES);
 
 DEFINE(UM_SECCOMP_ARCH_NATIVE, SECCOMP_ARCH_NATIVE);
+
+DEFINE(HOSTFS_ATTR_MODE, ATTR_MODE);
+DEFINE(HOSTFS_ATTR_UID, ATTR_UID);
+DEFINE(HOSTFS_ATTR_GID, ATTR_GID);
+DEFINE(HOSTFS_ATTR_SIZE, ATTR_SIZE);
+DEFINE(HOSTFS_ATTR_ATIME, ATTR_ATIME);
+DEFINE(HOSTFS_ATTR_MTIME, ATTR_MTIME);
+DEFINE(HOSTFS_ATTR_CTIME, ATTR_CTIME);
+DEFINE(HOSTFS_ATTR_ATIME_SET, ATTR_ATIME_SET);
+DEFINE(HOSTFS_ATTR_MTIME_SET, ATTR_MTIME_SET);
diff --git a/arch/x86/um/shared/sysdep/kernel-offsets.h b/arch/x86/um/shared/sysdep/kernel-offsets.h
index 6fd1ed400399..ee6b44ef2217 100644
--- a/arch/x86/um/shared/sysdep/kernel-offsets.h
+++ b/arch/x86/um/shared/sysdep/kernel-offsets.h
@@ -5,6 +5,7 @@
 #include <linux/crypto.h>
 #include <linux/kbuild.h>
 #include <linux/audit.h>
+#include <linux/fs.h>
 #include <asm/mman.h>
 #include <asm/seccomp.h>
 
diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
index 15b2f094d36e..aa02599b770f 100644
--- a/fs/hostfs/hostfs.h
+++ b/fs/hostfs/hostfs.h
@@ -3,40 +3,8 @@
 #define __UM_FS_HOSTFS
 
 #include <os.h>
+#include <generated/asm-offsets.h>
 
-/*
- * These are exactly the same definitions as in fs.h, but the names are
- * changed so that this file can be included in both kernel and user files.
- */
-
-#define HOSTFS_ATTR_MODE	1
-#define HOSTFS_ATTR_UID 	2
-#define HOSTFS_ATTR_GID 	4
-#define HOSTFS_ATTR_SIZE	8
-#define HOSTFS_ATTR_ATIME	16
-#define HOSTFS_ATTR_MTIME	32
-#define HOSTFS_ATTR_CTIME	64
-#define HOSTFS_ATTR_ATIME_SET	128
-#define HOSTFS_ATTR_MTIME_SET	256
-
-/* This one is unused by hostfs. */
-#define HOSTFS_ATTR_FORCE	512	/* Not a change, but a change it */
-#define HOSTFS_ATTR_ATTR_FLAG	1024
-
-/*
- * If you are very careful, you'll notice that these two are missing:
- *
- * #define ATTR_KILL_SUID	2048
- * #define ATTR_KILL_SGID	4096
- *
- * and this is because they were added in 2.5 development.
- * Actually, they are not needed by most ->setattr() methods - they are set by
- * callers of notify_change() to notify that the setuid/setgid bits must be
- * dropped.
- * notify_change() will delete those flags, make sure attr->ia_valid & ATTR_MODE
- * is on, and remove the appropriate bits from attr->ia_mode (attr is a
- * "struct iattr *"). -BlaisorBlade
- */
 struct hostfs_timespec {
 	long long tv_sec;
 	long long tv_nsec;
-- 
2.51.0



(that passes my usual tests, if you want you can apply it as is, or I
can resend it as a real patch, or I can also put it into uml-next for
later...)

johannes

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

[Linus Torvalds]

On Mon, 6 Oct 2025 at 14:20, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> That doesn't mean it's between the host and guest kernels, it's just
> between code built for "userspace" and code built for "kernel", both of
> which go into the UML linux "guest" binary. IOW, it's just for
> communication between hostfs_kern.c and hostfs_user.c, which nobody else
> needs to care about.

I was worried about people having version mismatches between those two parts.

But if that can't happen then that certainly simplifies things.

> However ... it looks like hostfs_kern.c is using ATTR_* in some places,
> and hostfs_user.c is using HOSTFS_ATTR_*, so it looks like right now
> these *do* need to match. Given that, we should just generate them via
> asm-offsets.h, like we do for other constants with the property of being
> needed on both sides but defined in places that cannot be included into
> user-side code, like this:

Sounds good.

> (that passes my usual tests, if you want you can apply it as is, or I
> can resend it as a real patch, or I can also put it into uml-next for
> later...)

None of this is the least critical - the bits I actually reacted to
aren't even used by hostfs so we also don't need to synchronize these
(potential) updates with each other.

So it does look like a good idea to remove the hardcoded "these bits
must match" thing, but clearly this hasn't been causing huge problems
in the many years they've been that way.

IOW - I'll wait for a later pull request from you.

            Linus

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

[Christian Brauner]

On Mon, Oct 06, 2025 at 04:58:22PM -0400, Chuck Lever wrote:
> On 10/6/25 4:51 PM, Linus Torvalds wrote:
> > On Mon, 6 Oct 2025 at 06:50, Chuck Lever <cel@kernel.org> wrote:
> >>
> >> One potential merge conflict has been reported for nfsd-6.18.
> > 
> > No problem, this is the simple kind of explicit conflict (famous last
> > words before I mess one of those things up).
> > 
> > Anyway, the reason I'm replying is actually that I notice that you
> > added that ATTR_CTIME_SET flag in <linux/fs.h> in commit afc5b36e29b9
> > ("vfs: add ATTR_CTIME_SET flag").
> > 
> > No complaints about it, but it looks a bit odd with ATTR_{A,M}TIME_SET
> > in bits 7 and 8, and then the new ATTR_CTIME_SET is in bit 10 with the
> > entirely unrelated ATTR_FORCE in between them all.
> 
> Oof. We should have gotten Acks for "vfs: add ATTR_CTIME_SET flag". My
> bad.

Yes, indeed. I wondered why I hadn't seen this patch.

> 
> 
> > So I'm thinking it would look cleaner if we just swapped
> > ATTR_CTIME_SET and ATTR_FORCE around - these are all just our own
> > kernel-internal bits (and the reason bit 10 was unused is that it used
> > to contain the odd ATTR_ATTR_FLAG that was never used).
> > 
> > Danger Will Robinson: hostfs has odd duplicate copies of all these, including a
> > 
> >    #define HOSTFS_ATTR_ATTR_FLAG   1024
> > 
> > of that no-longer existing flag.
> > 
> > But hostfs doesn't use ATTR_FORCE (aka HOSTFS_ATTR_FORCE), so
> > switching those two bits around wouldn't affect it either, even if you
> > were to have a version mismatch between the client and host when doing
> > UML (which I don't know
> > 
> > Adding Christian to the participants list, because I did *not* do that
> > cleanup thing myself, because I'm slightly worried that I'm missing
> > something. But it would seem to be a good thing to do just to have the
> > numbering make more sense, and Christian is probably the right person.
> > 
> > And adding Johannes Berg due to the UML connection, just to see that I
> > haven't misread that odd hostfs situation.
> > 
> > Comments?
> > 
> >             Linus
> 
> 
> -- 
> Chuck Lever

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

[Jeff Layton]

On Tue, 2025-10-07 at 13:26 +0200, Christian Brauner wrote:
> On Mon, Oct 06, 2025 at 04:58:22PM -0400, Chuck Lever wrote:
> > On 10/6/25 4:51 PM, Linus Torvalds wrote:
> > > On Mon, 6 Oct 2025 at 06:50, Chuck Lever <cel@kernel.org> wrote:
> > > > 
> > > > One potential merge conflict has been reported for nfsd-6.18.
> > > 
> > > No problem, this is the simple kind of explicit conflict (famous last
> > > words before I mess one of those things up).
> > > 
> > > Anyway, the reason I'm replying is actually that I notice that you
> > > added that ATTR_CTIME_SET flag in <linux/fs.h> in commit afc5b36e29b9
> > > ("vfs: add ATTR_CTIME_SET flag").
> > > 
> > > No complaints about it, but it looks a bit odd with ATTR_{A,M}TIME_SET
> > > in bits 7 and 8, and then the new ATTR_CTIME_SET is in bit 10 with the
> > > entirely unrelated ATTR_FORCE in between them all.
> > 
> > Oof. We should have gotten Acks for "vfs: add ATTR_CTIME_SET flag". My
> > bad.
> 
> Yes, indeed. I wondered why I hadn't seen this patch.
> 

I did send it to fsdevel, but you may have missed it in the deluge. Mea
culpa from me too -- I should have noticed that you guys hadn't acked
this yet. Any objection?
-- 
Jeff Layton <jlayton@kernel.org>

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

[Christian Brauner]

On Tue, Oct 07, 2025 at 07:47:39AM -0400, Jeff Layton wrote:
> On Tue, 2025-10-07 at 13:26 +0200, Christian Brauner wrote:
> > On Mon, Oct 06, 2025 at 04:58:22PM -0400, Chuck Lever wrote:
> > > On 10/6/25 4:51 PM, Linus Torvalds wrote:
> > > > On Mon, 6 Oct 2025 at 06:50, Chuck Lever <cel@kernel.org> wrote:
> > > > > 
> > > > > One potential merge conflict has been reported for nfsd-6.18.
> > > > 
> > > > No problem, this is the simple kind of explicit conflict (famous last
> > > > words before I mess one of those things up).
> > > > 
> > > > Anyway, the reason I'm replying is actually that I notice that you
> > > > added that ATTR_CTIME_SET flag in <linux/fs.h> in commit afc5b36e29b9
> > > > ("vfs: add ATTR_CTIME_SET flag").
> > > > 
> > > > No complaints about it, but it looks a bit odd with ATTR_{A,M}TIME_SET
> > > > in bits 7 and 8, and then the new ATTR_CTIME_SET is in bit 10 with the
> > > > entirely unrelated ATTR_FORCE in between them all.
> > > 
> > > Oof. We should have gotten Acks for "vfs: add ATTR_CTIME_SET flag". My
> > > bad.
> > 
> > Yes, indeed. I wondered why I hadn't seen this patch.
> > 
> 
> I did send it to fsdevel, but you may have missed it in the deluge. Mea
> culpa from me too -- I should have noticed that you guys hadn't acked
> this yet. Any objection?

No, it looks sane overally.

I think we should renumber. Frankly, I would also prefer for stuff like
this to be enums. It makes debugging for stuff like drgn that people use
easier and imho also looks nicer in the code. But that's a matter of
taste. And the renumbering might be the bigger win as Linus suggested.

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

[Geert Uytterhoeven]

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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Eric Biggers]

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.
> 
> Gr{oetje,eeting}s,
> 

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?

- Eric

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

[Chuck Lever]

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.
>>
>> Gr{oetje,eeting}s,
>>
> 
> 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.


-- 
Chuck Lever

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

[Jeff Layton]

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.
> > > 
> > > Gr{oetje,eeting}s,
> > > 
> > 
> > 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
        select SUNRPC_GSS
-       select CRYPTO
        select CRYPTO_SKCIPHER
        select CRYPTO_HASH
        help



-- 
Jeff Layton <jlayton@kernel.org>

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Chuck Lever]

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.


-- 
Chuck Lever

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

[Trond Myklebust]

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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com

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

[Eric Biggers]

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

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

[Chuck Lever]

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