| Message ID | 20210823151357.471691-12-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: various fixes | expand |
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 > >
On Tue, Aug 24, 2021 at 05:20:48PM +0900, Namjae Jeon wrote: > 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. Sounds good. Thanks! Christian
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) > 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); Can we move this above the ksmbd_vfs_set_sd_xattr call ? Current change cause hash diff error from smbtorture test. The hash value generated by using uid/gid/mode is stored into xattr. Could you please review the below change ? https://github.com/namjaejeon/smb3-kernel/commit/b53492ae8f16458dbc95424a5ebd7bb68c661900 Thanks! > + > out: > posix_acl_release(fattr.cf_acls); > posix_acl_release(fattr.cf_dacls); > -- > 2.30.2 > >
On Wed, Sep 01, 2021 at 09:53:55PM +0900, Namjae Jeon wrote: > 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) > > 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); > Can we move this above the ksmbd_vfs_set_sd_xattr call ? Current > change cause hash diff error from smbtorture test. The hash value > generated by using uid/gid/mode is stored into xattr. Could you please > review the below change ? > > https://github.com/namjaejeon/smb3-kernel/commit/b53492ae8f16458dbc95424a5ebd7bb68c661900 Yes, looks good. Thank you! Christian
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) 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);