On Thu, 10 Oct 2024, Chuck Lever wrote:
> On Thu, Oct 10, 2024 at 07:14:07AM +1100, NeilBrown wrote:
> > On Thu, 10 Oct 2024, Chuck Lever wrote:
> > > On Tue, Oct 08, 2024 at 05:47:55PM -0400, NeilBrown wrote:
> > > > And NFSD_MAY_LOCK should be discarded, and nlm_fopen() should set
> > > > NFSD_MAY_BYPASS_SEC.
> > >
> > > 366 /*
> > > 367 * pseudoflavor restrictions are not enforced on NLM,
> > >
> > > Wrt the mention of "NLM", nfsd4_lock() also sets NFSD_MAY_LOCK.
> >
> > True, but it shouldn't. NFSD_MAY_LOCK is only used to bypass the GSS
> > requirement. It must have been copied into nfsd4_lock() without a full
> > understanding of its purpose.
>
> nfsd4_lock()'s use of MAY_LOCK goes back before the git era, so it's
> difficult to say with certainty.
>
> I would like to keep such subtle changes bisectable. To me, it seems
> like it would be a basic first step to change the fh_verify() call
> in nfsd4_lock() to use (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE)
> instead of NFSD_MAY_LOCK, as a separate patch.
Yes, that is sensible ... though lockd used NFSD_MAY_WRITE for write
locks.
So if a process doesn't have read access to a file but does have write
access, and isn't the owner, then NLM would grant a write lock, but
NFSv4 would not. check_fmode_for_setlk() makes the same choice, so a
local user could also get the lock. Only NFSv4 would reject it.
>
>
> > > 368 * which clients virtually always use auth_sys for,
> > > 369 * even while using RPCSEC_GSS for NFS.
> > > 370 */
> > > 371 if (access & NFSD_MAY_LOCK)
> > > 372 goto skip_pseudoflavor_check;
> > > 373 if (access & NFSD_MAY_BYPASS_GSS)
> > > 374 may_bypass_gss = true;
> > > 375 /*
> > > 376 * Clients may expect to be able to use auth_sys during mount,
> > > 377 * even if they use gss for everything else; see section 2.3.2
> > > 378 * of rfc 2623.
> > > 379 */
> > > 380 if (access & NFSD_MAY_BYPASS_GSS_ON_ROOT
> > > 381 && exp->ex_path.dentry == dentry)
> > > 382 may_bypass_gss = true;
> > > 383
> > > 384 error = check_nfsd_access(exp, rqstp, may_bypass_gss);
> > > 385 if (error)
> > > 386 goto out;
> > > 387
> > > 388 skip_pseudoflavor_check:
> > > 389 /* Finally, check access permissions. */
> > > 390 error = nfsd_permission(cred, exp, dentry, access);
> > >
> > > MAY_LOCK is checked in nfsd_permission() and __fh_verify().
> > >
> > > But MAY_BYPASS_GSS is set in loads of places that use those two
> > > functions. How can we be certain that the two flags are equivalent?
> >
> > We can be certain by looking at the effect. Before a recent patch they
> > both did "goto skip_pseudoflavor_check" and nothing else.
>
> I'm still not convinced MAY_LOCK and MAY_BYPASS_GSS are 100%
> equivalent. nfsd_permission() checks for MAY_LOCK, but does not
> check for MAY_BYPASS_GSS:
>
> 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 only consumer of MAY_BYPASS_GSS seems to be OP_PUTFH, now that
> I'm looking closely for it. But I don't think we want the
> no_auth_nlm export option to modify the way PUTFH behaves.
Thanks for fact-checking my claim! I had forgotten about noauthnlm.
I'll suggest a patch which might make it all a bit clearer.
Thanks,
NeilBrown
>
>
> --
> Chuck Lever
>
