Re: [PATCH 11/11] ksmbd: defer notify_change() call

[Namjae Jeon <linkinjeon@kernel.org>]

2021-08-24 0:13 GMT+09:00, Christian Brauner <brauner@kernel.org>:
> From: Christian Brauner <christian.brauner@ubuntu.com>
>
> When ownership is changed we might in certain scenarios loose the
> ability to alter the inode after we changed ownership. This can e.g.
> happen when we are on an idmapped mount where uid 0 is mapped to uid
> 1000 and uid 1000 is mapped to uid 0.
> A caller with fs*id 1000 will be able to create files as *id 1000 on
> disk. They will also be able to change ownership of files owned by *id 0
> to *id 1000 but they won't be able to change ownership in the other
> direction. This means acl operations following notify_change() would
> fail. Move the notify_change() call after the acls have been updated.
> This guarantees that we don't end up with spurious "hash value diff"
> warnings later on because we managed to change ownership but didn't
> manage to alter acls.
>
> Cc: Steve French <stfrench@microsoft.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Namjae Jeon <namjae.jeon@samsung.com>
> Cc: Hyunchul Lee <hyc.lee@gmail.com>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: linux-cifs@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  fs/ksmbd/smbacl.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
> index ef5896297607..8457a3c27c12 100644
> --- a/fs/ksmbd/smbacl.c
> +++ b/fs/ksmbd/smbacl.c
> @@ -1334,25 +1334,27 @@ int set_info_sec(struct ksmbd_conn *conn, struct
> ksmbd_tree_connect *tcon,
>  	newattrs.ia_valid |= ATTR_MODE;
>  	newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
>
> -	inode_lock(inode);
> -	rc = notify_change(user_ns, path->dentry, &newattrs, NULL);
> -	inode_unlock(inode);
> -	if (rc)
> -		goto out;
> -
>  	ksmbd_vfs_remove_acl_xattrs(user_ns, path->dentry);
>  	/* Update posix acls */
>  	if (IS_ENABLED(CONFIG_FS_POSIX_ACL) && fattr.cf_dacls) {
>  		rc = set_posix_acl(user_ns, inode,
>  				   ACL_TYPE_ACCESS, fattr.cf_acls);
> +		if (rc < 0)
> +			ksmbd_debug(SMB,
> +				    "Set posix acl(ACL_TYPE_ACCESS) failed, rc : %d\n",
> +				    rc);
>  		if (S_ISDIR(inode->i_mode) && fattr.cf_dacls)
You seem to have missed adding braces after adding a debug print.
I will directly update it if you don't mind.

Thanks!
>  			rc = set_posix_acl(user_ns, inode,
>  					   ACL_TYPE_DEFAULT, fattr.cf_dacls);
> +		if (rc)
> +			ksmbd_debug(SMB,
> +				    "Set posix acl(ACL_TYPE_DEFAULT) failed, rc : %d\n",
> +				    rc);
>  	}
>
>  	/* Check it only calling from SD BUFFER context */
>  	if (type_check && !(le16_to_cpu(pntsd->type) & DACL_PRESENT))
> -		goto out;
> +		goto out_change;
>
>  	if (test_share_config_flag(tcon->share_conf, KSMBD_SHARE_FLAG_ACL_XATTR))
> {
>  		/* Update WinACL in xattr */
> @@ -1361,6 +1363,11 @@ int set_info_sec(struct ksmbd_conn *conn, struct
> ksmbd_tree_connect *tcon,
>  				       path->dentry, pntsd, ntsd_len);
>  	}
>
> +out_change:
> +	inode_lock(inode);
> +	rc = notify_change(user_ns, path->dentry, &newattrs, NULL);
> +	inode_unlock(inode);
> +
>  out:
>  	posix_acl_release(fattr.cf_acls);
>  	posix_acl_release(fattr.cf_dacls);
> --
> 2.30.2
>
>