Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 16 Jun 2020 07:28:35 +0200
From:   Salvatore Bonaccorso <carnil@debian.org>
To:     "J. Bruce Fields" <bfields@redhat.com>
Cc:     Elliott Mitchell <ehem+debian@m5p.com>, 962254@bugs.debian.org,
        linux-nfs@vger.kernel.org, agruenba@redhat.com
Subject: Re: Umask ignored when mounting NFSv4.2 share of an exported
 Filesystem with noacl (was: Re: Bug#962254: NFS(v4) broken at 4.19.118-2)
Message-ID: <20200616052835.GA19246@lorien.valinor.li>
References: <20200605064426.GA1538868@eldamar.local>
 <20200605051607.GA34405@mattapan.m5p.com>
 <20200605174349.GA40135@mattapan.m5p.com>
 <20200605183631.GA1720057@eldamar.local>
 <20200611223711.GA37917@mattapan.m5p.com>
 <20200613125431.GA349352@eldamar.local>
 <20200613184527.GA54221@mattapan.m5p.com>
 <20200615145035.GA214986@pick.fieldses.org>
 <20200615185311.GA702681@eldamar.local>
 <20200616023820.GB214986@pick.fieldses.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200616023820.GB214986@pick.fieldses.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Bruce,

On Mon, Jun 15, 2020 at 10:38:20PM -0400, J. Bruce Fields wrote:
> Thanks for the detailed reproducer.
> 
> It's weird, as the server is basically just setting the transmitted
> umask and then calling into the vfs to handle the rest, so it's not much
> different from any other user.  But the same reproducer run just on the
> ext4 filesystem does give the right permissions....
> 
> Oh, but looking at the system call, fs_namei.c:do_mkdirat(), it does:
> 
> 	if (!IS_POSIXACL(path.dentry->d_inode))
> 		mode &= ~current_umask();
> 	error = security_path_mkdir(&path, dentry, mode);
> 	if (!error)
> 		error = vfs_mkdir(path.dentry->d_inode, dentry, mode);
> 
> whereas nfsd just calls into vfs_mkdir().
> 
> And that IS_POSIXACL() check is exactly a check whether the filesystem
> supports ACLs.  So I guess it's the responsibility of the caller of
> vfs_mkdir() to handle that case.
> 
> So the obvious fix is something like (untested!)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 0aa02eb18bd3..dabdcca58969 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1234,6 +1234,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			nfsd_check_ignore_resizing(iap);
>  		break;
>  	case S_IFDIR:
> +		if (!IS_POSIXACL(dirp))
> +			iap->ia_mode &= ~current_umask();
>  		host_err = vfs_mkdir(dirp, dchild, iap->ia_mode);
>  		if (!host_err && unlikely(d_unhashed(dchild))) {
>  			struct dentry *d;

Thank you!

Tested your patch on top, and it would solve the directory case, but
the underlying problem is more general (and as you said proably needs
further checking in other places):

root@nfs-test:~# mount -t nfs 192.168.122.150:/srv/data /mnt
root@nfs-test:~# mkdir /mnt/foo && ls -ld /mnt/foo && rmdir /mnt/foo
drwxr-xr-x 2 root root 4096 Jun 16 07:24 /mnt/foo
root@nfs-test:~# touch /mnt/foo && ls -ld /mnt/foo && rm /mnt/foo
-rw-rw-rw- 1 root root 0 Jun 16 07:25 /mnt/foo
root@nfs-test:~# umount /mnt
root@nfs-test:~#

So when creating files the umask is still ignored in the noacl mounted
case.

Regards,
Salvatore