Re: [PATCH v2] fs: Fix the default values of i_uid/i_gid on /proc/sys inodes.

[Luis Chamberlain <mcgrof@kernel.org>]

Please re-state the main fix in the commit log, not just the subject.

Also, this does not explain why the current values are and the impact to
systems / users. This would help in determine and evaluating if this
deserves to be a stable fix.

On Fri, Jul 05, 2019 at 06:30:21PM +0200, Radoslaw Burny wrote:
> This also fixes a problem where, in a user namespace without root user
> mapping, it is not possible to write to /proc/sys/kernel/shmmax.

This does not explain why that should be possible and what impact this
limitation has.

> The problem was introduced by the combination of the two commits:
> * 81754357770ebd900801231e7bc8d151ddc00498: fs: Update
>   i_[ug]id_(read|write) to translate relative to s_user_ns
>     - this caused the kernel to write INVALID_[UG]ID to i_uid/i_gid
>     members of /proc/sys inodes if a containing userns does not have
>     entries for root in the uid/gid_map.
This is 2014 commit merged as of v4.8.

> * 0bd23d09b874e53bd1a2fe2296030aa2720d7b08: vfs: Don't modify inodes
>   with a uid or gid unknown to the vfs
>     - changed the kernel to prevent opens for write if the i_uid/i_gid
>     field in the inode is invalid

This is a 2016 commit merged as of v4.8 as well...

So regardless of the dates of the commits, are you saying this is a
regression you can confirm did not exist prior to v4.8? Did you test
v4.7 to confirm?

> This commit fixes the issue by defaulting i_uid/i_gid to
> GLOBAL_ROOT_UID/GID.

Why is this right?

> Note that these values are not used for /proc/sys
> access checks, so the change does not otherwise affect /proc semantics.
> 
> Tested: Used a repro program that creates a user namespace without any
> mapping and stat'ed /proc/$PID/root/proc/sys/kernel/shmmax from outside.
> Before the change, it shows the overflow uid, with the change it's 0.

Why is the overflow uid bad for user experience? Did you test prior to
v4.8, ie on v4.7 to confirm this is indeed a regression?

You'd want then to also ammend in the commit log a Fixes:  tag with both
commits listed. If this is a stable fix (criteria yet to be determined),
then we'd need a stable tag.

  Luis

> Signed-off-by: Radoslaw Burny <rburny@google.com>
> ---
> Changelog since v1:
> - Updated the commit title and description.
> 
>  fs/proc/proc_sysctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index c74570736b24..36ad1b0d6259 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -499,6 +499,10 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>  
>  	if (root->set_ownership)
>  		root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
> +	else {
> +		inode->i_uid = GLOBAL_ROOT_UID;
> +		inode->i_gid = GLOBAL_ROOT_GID;
> +	}
>  
>  	return inode;
>  }
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
>