Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Rich Felker' <dalias@libc.org>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>
CC:     Alexander Viro <viro@zeniv.linux.org.uk>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] vfs: add fchmodat2 syscall
Thread-Topic: [PATCH] vfs: add fchmodat2 syscall
Thread-Index: AQHWh4NogO4zJqm73EG5GUWCltp+Valh+9/A
Date:   Thu, 10 Sep 2020 15:23:40 +0000
Message-ID: <1111806ca0344527a8855616e46346c5@AcuMS.aculab.com>
References: <20200910142335.GG3265@brightrain.aerifal.cx>
In-Reply-To: <20200910142335.GG3265@brightrain.aerifal.cx>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

From: Rich Felker
> Sent: 10 September 2020 15:24
...
> index 9af548fb841b..570a21f4d81e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -610,15 +610,30 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)
>  	return err;
>  }
> 
> -static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
> +static int do_fchmodat(int dfd, const char __user *filename, umode_t mode, int flags)
>  {
>  	struct path path;
>  	int error;
>  	unsigned int lookup_flags = LOOKUP_FOLLOW;
> +
> +	if (flags & AT_SYMLINK_NOFOLLOW)
> +		lookup_flags &= ~LOOKUP_FOLLOW;
> +	if (flags & ~AT_SYMLINK_NOFOLLOW)
> +		return -EINVAL;

I think I'd swap over those two tests.
So unsupported flags are clearly errored.

>  retry:
>  	error = user_path_at(dfd, filename, lookup_flags, &path);
>  	if (!error) {
> -		error = chmod_common(&path, mode);
> +		/* Block chmod from getting to fs layer. Ideally the
> +		 * fs would either allow it or fail with EOPNOTSUPP,
> +		 * but some are buggy and return an error but change
> +		 * the mode, which is non-conforming and wrong.
> +		 * Userspace emulation of AT_SYMLINK_NOFOLLOW in
> +		 * glibc and musl blocked it too, for same reason. */
> +		if (S_ISLNK(path.dentry->d_inode->i_mode)
> +		    && (flags & AT_SYMLINK_NOFOLLOW))
> +			error = -EOPNOTSUPP;

Again swap the order of the tests. I think it reads better as:
		if ((flags & AT_SYMLINK_NOFOLLOW)
		    && S_ISLNK(path.dentry->d_inode->i_mode))
			error = -EOPNOTSUPP;
As well as saving a few clock cycles.

> +		else
> +			error = chmod_common(&path, mode);
>  		path_put(&path);
>  		if (retry_estale(error, lookup_flags)) {
>  			lookup_flags |= LOOKUP_REVAL;
...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)