From: Christian Brauner <brauner@kernel.org>
To: Steve French <stfrench@microsoft.com>,
Christoph Hellwig <hch@infradead.org>,
Namjae Jeon <namjae.jeon@samsung.com>,
linux-cifs@vger.kernel.org
Cc: Hyunchul Lee <hyc.lee@gmail.com>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
Christian Brauner <christian.brauner@ubuntu.com>,
Colin Ian King <colin.king@canonical.com>,
David Sterba <dsterba@suse.com>
Subject: [PATCH 01/11] ksmbd: fix lookup on idmapped mounts
Date: Mon, 23 Aug 2021 17:13:47 +0200 [thread overview]
Message-ID: <20210823151357.471691-2-brauner@kernel.org> (raw)
In-Reply-To: <20210823151357.471691-1-brauner@kernel.org>
From: Christian Brauner <christian.brauner@ubuntu.com>
It's great that the new in-kernel ksmbd server will support idmapped
mounts out of the box! However, lookup is currently broken. Lookup
helpers such as lookup_one_len() call inode_permission() internally to
ensure that the caller is privileged over the inode of the base dentry
they are trying to lookup under. So the permission checking here is
currently wrong.
Linux v5.15 will gain a new lookup helper lookup_one() that does take
idmappings into account. I've added it as part of my patch series to
make btrfs support idmapped mounts. The new helper is in linux-next as
part of David's (Sterba) btrfs for-next branch as commit
c972214c133b ("namei: add mapping aware lookup helper").
I've said it before during one of my first reviews: I would very much
recommend adding fstests to [1]. It already seems to have very
rudimentary cifs support. There is a completely generic idmapped mount
testsuite that supports idmapped mounts.
[1]: https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/
Cc: Colin Ian King <colin.king@canonical.com>
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: David Sterba <dsterba@suse.com>
Cc: linux-cifs@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Colin Ian King <colin.king@canonical.com>:
- Fix uninitialized variable warning.
---
fs/ksmbd/smb2pdu.c | 18 +++++++++++-------
fs/ksmbd/vfs.c | 43 ++++++++++++++++++++++++-------------------
fs/ksmbd/vfs.h | 3 ++-
3 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index d329ea49fa14..a400dd292af1 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -3543,9 +3543,9 @@ static int process_query_dir_entries(struct smb2_query_dir_private *priv)
return -EINVAL;
lock_dir(priv->dir_fp);
- dent = lookup_one_len(priv->d_info->name,
- priv->dir_fp->filp->f_path.dentry,
- priv->d_info->name_len);
+ dent = lookup_one(user_ns, priv->d_info->name,
+ priv->dir_fp->filp->f_path.dentry,
+ priv->d_info->name_len);
unlock_dir(priv->dir_fp);
if (IS_ERR(dent)) {
@@ -5246,7 +5246,9 @@ int smb2_echo(struct ksmbd_work *work)
return 0;
}
-static int smb2_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
+static int smb2_rename(struct ksmbd_work *work,
+ struct ksmbd_file *fp,
+ struct user_namespace *user_ns,
struct smb2_file_rename_info *file_info,
struct nls_table *local_nls)
{
@@ -5310,7 +5312,7 @@ static int smb2_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
if (rc)
goto out;
- rc = ksmbd_vfs_setxattr(file_mnt_user_ns(fp->filp),
+ rc = ksmbd_vfs_setxattr(user_ns,
fp->filp->f_path.dentry,
xattr_stream_name,
NULL, 0, 0);
@@ -5624,6 +5626,7 @@ static int set_end_of_file_info(struct ksmbd_work *work, struct ksmbd_file *fp,
static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
char *buf)
{
+ struct user_namespace *user_ns;
struct ksmbd_file *parent_fp;
struct dentry *parent;
struct dentry *dentry = fp->filp->f_path.dentry;
@@ -5634,11 +5637,12 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
return -EACCES;
}
+ user_ns = file_mnt_user_ns(fp->filp);
if (ksmbd_stream_fd(fp))
goto next;
parent = dget_parent(dentry);
- ret = ksmbd_vfs_lock_parent(parent, dentry);
+ ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);
if (ret) {
dput(parent);
return ret;
@@ -5655,7 +5659,7 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
}
}
next:
- return smb2_rename(work, fp,
+ return smb2_rename(work, fp, user_ns,
(struct smb2_file_rename_info *)buf,
work->sess->conn->local_nls);
}
diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
index aee28ee6b19c..2bb506d1fb32 100644
--- a/fs/ksmbd/vfs.c
+++ b/fs/ksmbd/vfs.c
@@ -69,14 +69,15 @@ static void ksmbd_vfs_inherit_owner(struct ksmbd_work *work,
*
* the reference count of @parent isn't incremented.
*/
-int ksmbd_vfs_lock_parent(struct dentry *parent, struct dentry *child)
+int ksmbd_vfs_lock_parent(struct user_namespace *user_ns, struct dentry *parent,
+ struct dentry *child)
{
struct dentry *dentry;
int ret = 0;
inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
- dentry = lookup_one_len(child->d_name.name, parent,
- child->d_name.len);
+ dentry = lookup_one(user_ns, child->d_name.name, parent,
+ child->d_name.len);
if (IS_ERR(dentry)) {
ret = PTR_ERR(dentry);
goto out_err;
@@ -102,7 +103,7 @@ int ksmbd_vfs_may_delete(struct user_namespace *user_ns,
int ret;
parent = dget_parent(dentry);
- ret = ksmbd_vfs_lock_parent(parent, dentry);
+ ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);
if (ret) {
dput(parent);
return ret;
@@ -137,7 +138,7 @@ int ksmbd_vfs_query_maximal_access(struct user_namespace *user_ns,
*daccess |= FILE_EXECUTE_LE;
parent = dget_parent(dentry);
- ret = ksmbd_vfs_lock_parent(parent, dentry);
+ ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);
if (ret) {
dput(parent);
return ret;
@@ -197,6 +198,7 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
*/
int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
{
+ struct user_namespace *user_ns;
struct path path;
struct dentry *dentry;
int err;
@@ -210,16 +212,16 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
return err;
}
+ user_ns = mnt_user_ns(path.mnt);
mode |= S_IFDIR;
- err = vfs_mkdir(mnt_user_ns(path.mnt), d_inode(path.dentry),
- dentry, mode);
+ err = vfs_mkdir(user_ns, d_inode(path.dentry), dentry, mode);
if (err) {
goto out;
} else if (d_unhashed(dentry)) {
struct dentry *d;
- d = lookup_one_len(dentry->d_name.name, dentry->d_parent,
- dentry->d_name.len);
+ d = lookup_one(user_ns, dentry->d_name.name, dentry->d_parent,
+ dentry->d_name.len);
if (IS_ERR(d)) {
err = PTR_ERR(d);
goto out;
@@ -582,6 +584,7 @@ int ksmbd_vfs_fsync(struct ksmbd_work *work, u64 fid, u64 p_id)
*/
int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
{
+ struct user_namespace *user_ns;
struct path path;
struct dentry *parent;
int err;
@@ -601,8 +604,9 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
return err;
}
+ user_ns = mnt_user_ns(path.mnt);
parent = dget_parent(path.dentry);
- err = ksmbd_vfs_lock_parent(parent, path.dentry);
+ err = ksmbd_vfs_lock_parent(user_ns, parent, path.dentry);
if (err) {
dput(parent);
path_put(&path);
@@ -616,14 +620,12 @@ int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name)
}
if (S_ISDIR(d_inode(path.dentry)->i_mode)) {
- err = vfs_rmdir(mnt_user_ns(path.mnt), d_inode(parent),
- path.dentry);
+ err = vfs_rmdir(user_ns, d_inode(parent), path.dentry);
if (err && err != -ENOTEMPTY)
ksmbd_debug(VFS, "%s: rmdir failed, err %d\n", name,
err);
} else {
- err = vfs_unlink(mnt_user_ns(path.mnt), d_inode(parent),
- path.dentry, NULL);
+ err = vfs_unlink(user_ns, d_inode(parent), path.dentry, NULL);
if (err)
ksmbd_debug(VFS, "%s: unlink failed, err %d\n", name,
err);
@@ -748,7 +750,8 @@ static int __ksmbd_vfs_rename(struct ksmbd_work *work,
if (ksmbd_override_fsids(work))
return -ENOMEM;
- dst_dent = lookup_one_len(dst_name, dst_dent_parent, strlen(dst_name));
+ dst_dent = lookup_one(dst_user_ns, dst_name, dst_dent_parent,
+ strlen(dst_name));
err = PTR_ERR(dst_dent);
if (IS_ERR(dst_dent)) {
pr_err("lookup failed %s [%d]\n", dst_name, err);
@@ -779,6 +782,7 @@ static int __ksmbd_vfs_rename(struct ksmbd_work *work,
int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
char *newname)
{
+ struct user_namespace *user_ns;
struct path dst_path;
struct dentry *src_dent_parent, *dst_dent_parent;
struct dentry *src_dent, *trap_dent, *src_child;
@@ -808,8 +812,9 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
trap_dent = lock_rename(src_dent_parent, dst_dent_parent);
dget(src_dent);
dget(dst_dent_parent);
- src_child = lookup_one_len(src_dent->d_name.name, src_dent_parent,
- src_dent->d_name.len);
+ user_ns = file_mnt_user_ns(fp->filp);
+ src_child = lookup_one(user_ns, src_dent->d_name.name, src_dent_parent,
+ src_dent->d_name.len);
if (IS_ERR(src_child)) {
err = PTR_ERR(src_child);
goto out_lock;
@@ -823,7 +828,7 @@ int ksmbd_vfs_fp_rename(struct ksmbd_work *work, struct ksmbd_file *fp,
dput(src_child);
err = __ksmbd_vfs_rename(work,
- file_mnt_user_ns(fp->filp),
+ user_ns,
src_dent_parent,
src_dent,
mnt_user_ns(dst_path.mnt),
@@ -1109,7 +1114,7 @@ int ksmbd_vfs_unlink(struct user_namespace *user_ns,
{
int err = 0;
- err = ksmbd_vfs_lock_parent(dir, dentry);
+ err = ksmbd_vfs_lock_parent(user_ns, dir, dentry);
if (err)
return err;
dget(dentry);
diff --git a/fs/ksmbd/vfs.h b/fs/ksmbd/vfs.h
index cb0cba0d5d07..85db50abdb24 100644
--- a/fs/ksmbd/vfs.h
+++ b/fs/ksmbd/vfs.h
@@ -107,7 +107,8 @@ struct ksmbd_kstat {
__le32 file_attributes;
};
-int ksmbd_vfs_lock_parent(struct dentry *parent, struct dentry *child);
+int ksmbd_vfs_lock_parent(struct user_namespace *user_ns, struct dentry *parent,
+ struct dentry *child);
int ksmbd_vfs_may_delete(struct user_namespace *user_ns, struct dentry *dentry);
int ksmbd_vfs_query_maximal_access(struct user_namespace *user_ns,
struct dentry *dentry, __le32 *daccess);
--
2.30.2
next prev parent reply other threads:[~2021-08-23 15:14 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20210823030840epcas1p24b226d445a683012925efd81a72ecb6d@epcas1p2.samsung.com>
2021-08-23 2:58 ` [PATCH v8 00/13] ksmbd: introduce new SMB3 kernel server Namjae Jeon
[not found] ` <CGME20210823030841epcas1p1a811d4a6aec75c09581a9b0fb575d23e@epcas1p1.samsung.com>
2021-08-23 2:58 ` [PATCH v8 01/13] ksmbd: add document Namjae Jeon
[not found] ` <CGME20210823030842epcas1p27cdeb782776f6659826110cd9a3524d4@epcas1p2.samsung.com>
2021-08-23 2:58 ` [PATCH v8 02/13] ksmbd: add server handler Namjae Jeon
[not found] ` <CGME20210823030843epcas1p4502dad130066a74f08745c849b981112@epcas1p4.samsung.com>
2021-08-23 2:58 ` [PATCH v8 03/13] ksmbd: add tcp transport layer Namjae Jeon
[not found] ` <CGME20210823030844epcas1p2a9dc2c02d32df86e9eb3c2af975c7d81@epcas1p2.samsung.com>
2021-08-23 2:58 ` [PATCH v8 04/13] ksmbd: add ipc " Namjae Jeon
[not found] ` <CGME20210823030845epcas1p3ff50078868ed215c43898356c9248d24@epcas1p3.samsung.com>
2021-08-23 2:58 ` [PATCH v8 05/13] ksmbd: add rdma " Namjae Jeon
[not found] ` <CGME20210823030845epcas1p2c72588cd470ca46463fd46b42b7b9603@epcas1p2.samsung.com>
2021-08-23 2:58 ` [PATCH v8 06/13] ksmbd: add a utility code that tracks (and caches) sessions data Namjae Jeon
[not found] ` <CGME20210823030846epcas1p35bd3c665d8afd6205c617840e709afc7@epcas1p3.samsung.com>
2021-08-23 2:58 ` [PATCH v8 07/13] ksmbd: add authentication Namjae Jeon
[not found] ` <CGME20210823030849epcas1p39035b8f9ec5cdc87dc7beca86590932c@epcas1p3.samsung.com>
2021-08-23 2:58 ` [PATCH v8 10/13] ksmbd: add oplock/lease cache mechanism Namjae Jeon
[not found] ` <CGME20210823030850epcas1p1eea7803d1ca2e854a0199f4c83cd8190@epcas1p1.samsung.com>
2021-08-23 2:58 ` [PATCH v8 11/13] ksmbd: add file operations Namjae Jeon
[not found] ` <CGME20210823030851epcas1p2d141386b64cd9039121a9f6a5074a362@epcas1p2.samsung.com>
2021-08-23 2:58 ` [PATCH v8 12/13] ksmbd: add Kconfig and Makefile Namjae Jeon
[not found] ` <CGME20210823030851epcas1p3df6319948e331e2e0225adba4e81e660@epcas1p3.samsung.com>
2021-08-23 2:58 ` [PATCH v8 13/13] MAINTAINERS: add ksmbd kernel server Namjae Jeon
2021-08-23 15:13 ` [PATCH 00/11] ksmbd: various fixes Christian Brauner
2021-08-23 15:13 ` Christian Brauner [this message]
2021-08-23 15:13 ` [PATCH 02/11] ksmbd: fix translation in smb2_populate_readdir_entry() Christian Brauner
2021-08-23 15:13 ` [PATCH 03/11] ksmbd: fix translation in create_posix_rsp_buf() Christian Brauner
2021-08-23 15:13 ` [PATCH 04/11] smb2pdu: fix translation in ksmbd_acls_fattr() Christian Brauner
2021-08-23 15:13 ` [PATCH 05/11] ksmbd: fix translation in acl entries Christian Brauner
2021-08-23 15:13 ` [PATCH 06/11] ksmbd: fix subauth 0 handling in sid_to_id() Christian Brauner
2021-08-24 8:13 ` Namjae Jeon
2021-08-24 11:37 ` Christian Brauner
2021-08-23 15:13 ` [PATCH 07/11] ksmbd: fix translation " Christian Brauner
2021-08-23 15:13 ` [PATCH 08/11] ndr: fix translation in ndr_encode_posix_acl() Christian Brauner
2021-08-23 15:13 ` [PATCH 09/11] ksmbd: ensure error is surfaced in set_file_basic_info() Christian Brauner
2021-08-23 15:13 ` [PATCH 10/11] ksmbd: remove setattr preparations " Christian Brauner
2021-09-01 12:47 ` Namjae Jeon
2021-09-02 13:43 ` Christian Brauner
2021-10-01 18:51 ` Marios Makassikis
2021-10-02 0:41 ` Namjae Jeon
2021-10-02 19:29 ` Marios Makassikis
2021-10-03 0:12 ` Namjae Jeon
2021-08-23 15:13 ` [PATCH 11/11] ksmbd: defer notify_change() call Christian Brauner
2021-08-24 8:20 ` Namjae Jeon
2021-08-24 11:36 ` Christian Brauner
2021-09-01 12:53 ` Namjae Jeon
2021-09-02 13:42 ` Christian Brauner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210823151357.471691-2-brauner@kernel.org \
--to=brauner@kernel.org \
--cc=christian.brauner@ubuntu.com \
--cc=colin.king@canonical.com \
--cc=dsterba@suse.com \
--cc=hch@infradead.org \
--cc=hyc.lee@gmail.com \
--cc=linux-cifs@vger.kernel.org \
--cc=namjae.jeon@samsung.com \
--cc=senozhatsky@chromium.org \
--cc=stfrench@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).