Re: [PATCH 17/18] nfsd: use vfs_lookup_open() for non-creating open requests too.

On Sun, May 31, 2026, at 11:38 PM, NeilBrown wrote:

> @@ -429,45 +438,35 @@ do_open_lookup(struct svc_rqst *rqstp, struct 
> nfsd4_compound_state *cstate, stru
>  	fh_init(*resfh, NFS4_FHSIZE);
>  	open->op_truncate = false;
> 
> -	status = fh_fill_pre_attrs(current_fh);
> -	if (status)
> -		goto out;
> -	if (open->op_create) {
> -		/* FIXME: check session persistence and pnfs flags.
> -		 * The nfsv4.1 spec requires the following semantics:
> -		 *
> -		 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
> -		 * Reply Cache  | server |                 |
> -		 * -------------+--------+-----------------+--------------------
> -		 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
> -		 *              |        |                 | (SHOULD)
> -		 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
> -		 *              |        |                 | (SHOULD NOT)
> -		 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
> -		 * yes          | no     | GUARDED4        | GUARDED4
> -		 * yes          | yes    | GUARDED4        | GUARDED4
> -		 */
> +	/* FIXME: check session persistence and pnfs flags.
> +	 * The nfsv4.1 spec requires the following semantics:
> +	 *
> +	 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
> +	 * Reply Cache  | server |                 |
> +	 * -------------+--------+-----------------+--------------------
> +	 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
> +	 *              |        |                 | (SHOULD)
> +	 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
> +	 *              |        |                 | (SHOULD NOT)
> +	 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
> +	 * yes          | no     | GUARDED4        | GUARDED4
> +	 * yes          | yes    | GUARDED4        | GUARDED4
> +	 */
> 
> -		current->fs->umask = open->op_umask;
> -		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
> -		current->fs->umask = 0;
> +	current->fs->umask = open->op_umask;
> +	status = nfsd4_open_file(rqstp, current_fh, *resfh, open);
> +	current->fs->umask = 0;
> +
> +	/*
> +	 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
> +	 * use the returned bitmask to indicate which attributes
> +	 * we used to store the verifier:
> +	 */
> +	if (open->op_create && status == 0 &&
> +	    nfsd4_create_is_exclusive(open->op_createmode))
> +		open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> +				      FATTR4_WORD1_TIME_MODIFY);
> 
> -		/*
> -		 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
> -		 * use the returned bitmask to indicate which attributes
> -		 * we used to store the verifier:
> -		 */
> -		if (nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
> -			open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
> -						FATTR4_WORD1_TIME_MODIFY);
> -	} else {
> -		status = nfsd_lookup(rqstp, current_fh,
> -				     open->op_fname, open->op_fnamelen, *resfh);
> -		/* NFSv4 protocol requires change attributes even though
> -		 * no change happened.
> -		 */
> -		fh_fill_post_noop(current_fh);
> -	}
>  	if (status)
>  		goto out;
>  	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);

Logic bug: Pre-series, the non-create case takes do_open_lookup()'s
else branch and succeeds; post-series, do_open_lookup() calls
nfsd4_open_file() unconditionally. Every CLAIM_NULL open of an
existing file fails with NFS4ERR_EXIST.


-- 
Chuck Lever