On Thu, Mar 20, 2025 at 1:32 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> On 3/20/25 12:29 PM, Olga Kornievskaia wrote:
> > On Thu, Mar 20, 2025 at 9:58 AM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >>
> >> Hi Olga -
> >>
> >> Thanks for taking a stab at this. Comments below.
> >>
> >>
> >> On 3/19/25 5:46 PM, Olga Kornievskaia wrote:
> >>> Since commit 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK")
> >>> for export policies with "sec=krb5:..." or "xprtsec=tls:.." NLM
> >>> locking calls on v3 mounts fail. And for "sec=krb5" NLM calls it
> >>> also leads to out-of-bounds reference while in check_nfsd_access().
> >>>
> >>> This patch does 3 fixes.
> >>
> >> That suggests to me this should ideally be three patches. That's a nit,
> >> really, but 3 patches might be better for subsequent bisection.
> >
> > I can break it into 3 patches but all would have "Fixes" for the same
> > patch, right?
>
> Yes.
>
>
> >>> 2 problems are related to sec=krb5.
> >>> First is fixing what "access" content is being passed into
> >>> the inode_permission(). Prior to 4cc9b9f2bf4df, the code would
> >>> explicitly set access to be read/ownership. And after is passes
> >>> access that's set in nlm_fopen but it's lacking read access.
> >>> Second is because previously for NLM check_nfsd_access() was
> >>> never called and thus nfsd4_spo_must_allow() function wasn't
> >>> called. After the patch, this lead to NLM call which has no
> >>> compound state structure created trying to dereference it.
> >>> This patch instead moves the call to after may_bypass_gss
> >>> check which implies NLM and would return there and would
> >>> never get to calling nfsd4_spo_must_allow().
> >>>
> >>> The last problem is related to TLS export policy. NLM dont
> >>> come over TLS and thus dont pass the TLS checks in
> >>> check_nfsd_access() leading to access being denied. Instead
> >>> rely on may_bypass_gss to indicate NLM and allow access
> >>> checking to continue.
> >>
> >> NFSD doesn't check that NLM is TLS protected because:
> >>
> >> a. the current Linux NFS client doesn't use TLS, and
> >> b. NFSD doesn't support NLM over krb5, IIRC.
> >>
> >> But that doesn't mean NLM will /never/ be protected by TLS.
> >
> > But if the future NFSD would support NLM with TLS wouldn't it be a new
> > feature with possible new controls. We'd still need existing code to
> > all interoperability with clients that don't support it (and thus
> > having a configuration that would allow NLM without TLS when xprtsec
> > is TLS-enabled?
> >
> >> I'm thinking aloud about whether the reorganization here might make
> >> adding TLS for NLM easier or more difficult. No conclusions yet.
> >
> > Do we need that complexity now vs if and when such support is added?
>
> The question is would we need to revert some or all of your patch
> to make that work in the future in order to support NLM with TLS.
> But perhaps that is an unanswerable question today.
>
>
> >>> Fixes: 4cc9b9f2bf4df ("nfsd: refine and rename NFSD_MAY_LOCK")
> >>> Signed-off-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
> >>>
> >>> ---
> >>> fs/nfsd/export.c | 23 +++++++++++++----------
> >>> fs/nfsd/vfs.c | 4 ++++
> >>> 2 files changed, 17 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> >>> index 0363720280d4..4cbf617efa7c 100644
> >>> --- a/fs/nfsd/export.c
> >>> +++ b/fs/nfsd/export.c
> >>> @@ -1124,7 +1124,8 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >>> test_bit(XPT_PEER_AUTH, &xprt->xpt_flags))
> >>> goto ok;
> >>> }
> >>> - goto denied;
> >>> + if (!may_bypass_gss)
> >>> + goto denied;
> >>>
> >>> ok:
> >>> /* legacy gss-only clients are always OK: */
> >>> @@ -1142,15 +1143,6 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >>> return nfs_ok;
> >>> }
> >>>
> >>> - /* If the compound op contains a spo_must_allowed op,
> >>> - * it will be sent with integrity/protection which
> >>> - * will have to be expressly allowed on mounts that
> >>> - * don't support it
> >>> - */
> >>> -
> >>> - if (nfsd4_spo_must_allow(rqstp))
> >>> - return nfs_ok;
> >>> -
> >>> /* Some calls may be processed without authentication
> >>> * on GSS exports. For example NFS2/3 calls on root
> >>> * directory, see section 2.3.2 of rfc 2623.
> >>> @@ -1168,6 +1160,17 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp,
> >>> }
> >>> }
> >>>
> >>> + /* If the compound op contains a spo_must_allowed op,
> >>> + * it will be sent with integrity/protection which
> >>> + * will have to be expressly allowed on mounts that
> >>> + * don't support it
> >>> + */
> >>> + /* This call must be done after the may_bypass_gss check.
> >>> + * may_bypass_gss implies NLM(/MOUNT) and not 4.1
> >>> + */
> >>> + if (nfsd4_spo_must_allow(rqstp))
> >>> + return nfs_ok;
> >>> +
> >>
> >> Comment about future work: This is technical debt (that's the 21st
> >> century term for spaghetti), logic that has accrued over time and is
> >> now therefore difficult to reason about. Would be nice to break
> >> check_nfsd_access() apart into "for NLM", "for NFS-legacy", and "for NFS
> >> with COMPOUND". For another day, perhaps.
> >>
> >>
> >>> denied:
> >>> return nfserr_wrongsec;
> >>> }
> >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >>> index 4021b047eb18..95d973136079 100644
> >>> --- a/fs/nfsd/vfs.c
> >>> +++ b/fs/nfsd/vfs.c
> >>> @@ -2600,6 +2600,10 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
> >>
> >> More context shows there is an NFSD_MAY_OWNER_OVERRIDE check just
> >> before the new logic added by this patch:
> >>
> >> if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
> >>
> >>> uid_eq(inode->i_uid, current_fsuid()))
> >>> return 0;
> >>>
> >>> + /* If this is NLM, require read or ownership permissions */
> >>> + if (acc & NFSD_MAY_NLM)
> >>> + acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
> >>> +
> >>
> >> So I'm wondering if this new test needs to come /before/ the existing
> >> MAY_OWNER_OVERRIDE check here? If not, the comment you add here might
> >> explain why it is correct to place the new logic afterwards.
> >
> > Why would you think this check should go before?
>
> Because this code is confusing.
>
> So, for NLM, MAY_OWNER_OVERRIDE is already set for the uid_eq check.
> The order of the new code is not consequential. But it still catches
> the eye.
I just re-checked the original code and the re(set)ing of acc was done
before the MAY_OWNER_OVERRIDE check. So changing my patch as you
suggest would be consistent with how things worked before. I will do
that.
> > NFS_MAY_OWNER_OVERRIDE is actually already set by nlm_fopen() when it
> > arrives in check_nfsd_access() .
>
> This code is /clearing/ the other flags too. That's a little subtle.
>
> The new comment should explain why only these two flags are needed for
> the subsequent code.
inode_permission() only care about READ, WRITE (EXEC)? NFSD_MAY_READ
is supposed to be the same as MAY_READ etc. Thus I believe passing
other values is of no consequence so to be honest I don't understand
why NFSD_MAY_OWNER_OVERRIDE is needed here. But what is of consequence
is (not) passing a NFSD_MAY_WRITE (which is what gets passed in by
nlm_fopen()).
> That is, explain not only why NFSD_MAY_READ is
> getting set, but also why NFSD_MAY_NLM and NFSD_MAY_BYPASS_GSS are
> getting cleared.
>
> Or, if clearing those flags was unintentional, make the code read:
>
> acc |= NFSD_MAY_READ;
>
> I don't understand this code well enough to figure out why nlm_fopen()
> shouldn't just set NFSD_MAY_READ. Therefore: better code comment needed.
My comment came from the comment that was removed by commit
4cc9b9f2bf4df. I don't have a good understanding of the code to
provide a "better" comment.
nlm_fopen assigns the access based on the requested lock type. an
exclusive lock i'm assuming gets translated to the "write" =
NFSD_MAY_WRITE. But the the user might not have write access to the
file but still be allowed to request an exclusive lock on it, right?
@@ -2531,16 +2531,6 @@ nfsd_permission(struct svc_cred *cred, struct
svc_export *exp,
if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
return nfserr_perm;
- if (acc & NFSD_MAY_LOCK) {
- /* If we cannot rely on authentication in NLM requests,
- * just allow locks, otherwise require read permission, or
- * ownership
- */
- if (exp->ex_flags & NFSEXP_NOAUTHNLM)
- return 0;
- else
- acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
- }
/*
* The file owner always gets access permission for accesses that
* would normally be checked at open time. This is to make
>
>
> > Prior to the 4cc9b9f2bf4df inode_permission() would get NFSD_MAY_READ
> > | NFSD_MAY_OWNER_OVERRIDE in access argument. After, it gets
> > NFSD_MAY_WRITE | NFSD_MAY_NLM | NFSD_MAY_OWNER_OVERRIDE |
> > NFSD_MAY_BYPASS_GSS. It made inode_permission() fail the call. Isn't
> > NLM asking for write permissions on the file for locking?
> >
> > My attempt was to return the code to previous functionality which
> > worked (and was only explicitly asking for read permissions on the
> > file).
>
> So the patch is reverting part of 4cc9b9f2bf4df; perhaps that should be
> called out in the patch description (up to you). However, the proposed
> fix doesn't make this code easier to understand, and that's probably my
> main quibble.
>
>
> > Unless you think more is needed here: I would resubmit with 3 patches
> > for each of the chunks and corresponding problems.
>
> Agreed, and I don't think I have any substantial change requests for the
> first two fixes.
>
>
> >>> /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
> >>> err = inode_permission(&nop_mnt_idmap, inode,
> >>> acc & (MAY_READ | MAY_WRITE | MAY_EXEC));
> >>
> >>
> >> --
> >> Chuck Lever
> >>
>
>
> --
> Chuck Lever
>
