[PATCH] ksmbd: fix lookup on idmapped mounts

[PATCH] ksmbd: fix lookup on idmapped mounts

[Christian Brauner]

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: 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>
---
I merged David's for-next tree into cifsd-next to test this. I did only
compile test this. If someone gives me a clear set of instructions how
to test ksmbd on my local machine I can at least try to cut some time
out of my week to do more reviews. (I'd especially like to see acl
behavior with ksmbd.)

One more thing, the tree for ksmbd was very hard to find. I had to do a
lot archeology to end up at:

git://git.samba.org/ksmbd.git

Would be appreciated if this tree could be reflected in MAINTAINERS or
somewhere else. The github repos with the broken out patches/module
aren't really that helpful.

Thanks!
Christian
---
 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 636570ecfa31..9a1a9a024714 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -3539,9 +3539,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)) {
@@ -5242,7 +5242,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)
 {
@@ -5306,7 +5308,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);
@@ -5620,6 +5622,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;
@@ -5633,8 +5636,9 @@ static int set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
 	if (ksmbd_stream_fd(fp))
 		goto next;
 
+	user_ns = file_mnt_user_ns(fp->filp);
 	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;
@@ -5651,7 +5655,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 612c52d7a01b..8000ea3ade10 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);

base-commit: 456af438ad490bac7ed954cb929bcec1df7f0c82
-- 
2.30.2

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4734 bytes --]

Hi Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on 456af438ad490bac7ed954cb929bcec1df7f0c82]

url:    https://github.com/0day-ci/linux/commits/Christian-Brauner/ksmbd-fix-lookup-on-idmapped-mounts/20210816-200054
base:   456af438ad490bac7ed954cb929bcec1df7f0c82
config: s390-allyesconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ebbc41b9d3f7b7f6c558cb5d5f12ecb8134c9c2e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christian-Brauner/ksmbd-fix-lookup-on-idmapped-mounts/20210816-200054
        git checkout ebbc41b9d3f7b7f6c558cb5d5f12ecb8134c9c2e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/ksmbd/vfs.c: In function 'ksmbd_vfs_lock_parent':
   fs/ksmbd/vfs.c:79:18: error: implicit declaration of function 'lookup_one'; did you mean 'lookup_bdev'? [-Werror=implicit-function-declaration]
      79 |         dentry = lookup_one(user_ns, child->d_name.name, parent,
         |                  ^~~~~~~~~~
         |                  lookup_bdev
>> fs/ksmbd/vfs.c:79:16: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      79 |         dentry = lookup_one(user_ns, child->d_name.name, parent,
         |                ^
   fs/ksmbd/vfs.c: In function 'ksmbd_vfs_mkdir':
   fs/ksmbd/vfs.c:223:19: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     223 |                 d = lookup_one(user_ns, dentry->d_name.name, dentry->d_parent,
         |                   ^
   fs/ksmbd/vfs.c: In function '__ksmbd_vfs_rename':
   fs/ksmbd/vfs.c:753:18: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     753 |         dst_dent = lookup_one(dst_user_ns, dst_name, dst_dent_parent,
         |                  ^
   fs/ksmbd/vfs.c: In function 'ksmbd_vfs_fp_rename':
   fs/ksmbd/vfs.c:816:19: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     816 |         src_child = lookup_one(user_ns, src_dent->d_name.name, src_dent_parent,
         |                   ^
   cc1: some warnings being treated as errors
--
   fs/ksmbd/smb2pdu.c: In function 'process_query_dir_entries':
   fs/ksmbd/smb2pdu.c:3542:24: error: implicit declaration of function 'lookup_one'; did you mean 'lookup_bdev'? [-Werror=implicit-function-declaration]
    3542 |                 dent = lookup_one(user_ns, priv->d_info->name,
         |                        ^~~~~~~~~~
         |                        lookup_bdev
>> fs/ksmbd/smb2pdu.c:3542:22: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    3542 |                 dent = lookup_one(user_ns, priv->d_info->name,
         |                      ^
   cc1: some warnings being treated as errors


vim +79 fs/ksmbd/vfs.c

    62	
    63	/**
    64	 * ksmbd_vfs_lock_parent() - lock parent dentry if it is stable
    65	 *
    66	 * the parent dentry got by dget_parent or @parent could be
    67	 * unstable, we try to lock a parent inode and lookup the
    68	 * child dentry again.
    69	 *
    70	 * the reference count of @parent isn't incremented.
    71	 */
    72	int ksmbd_vfs_lock_parent(struct user_namespace *user_ns, struct dentry *parent,
    73				  struct dentry *child)
    74	{
    75		struct dentry *dentry;
    76		int ret = 0;
    77	
    78		inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
  > 79		dentry = lookup_one(user_ns, child->d_name.name, parent,
    80				    child->d_name.len);
    81		if (IS_ERR(dentry)) {
    82			ret = PTR_ERR(dentry);
    83			goto out_err;
    84		}
    85	
    86		if (dentry != child) {
    87			ret = -ESTALE;
    88			dput(dentry);
    89			goto out_err;
    90		}
    91	
    92		dput(dentry);
    93		return 0;
    94	out_err:
    95		inode_unlock(d_inode(parent));
    96		return ret;
    97	}
    98	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 66437 bytes --]

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On Mon, Aug 16, 2021 at 10:47:49PM +0800, kernel test robot wrote:
> Hi Christian,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on 456af438ad490bac7ed954cb929bcec1df7f0c82]
> 
> url:    https://github.com/0day-ci/linux/commits/Christian-Brauner/ksmbd-fix-lookup-on-idmapped-mounts/20210816-200054
> base:   456af438ad490bac7ed954cb929bcec1df7f0c82
> config: s390-allyesconfig (attached as .config)
> compiler: s390-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/ebbc41b9d3f7b7f6c558cb5d5f12ecb8134c9c2e
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Christian-Brauner/ksmbd-fix-lookup-on-idmapped-mounts/20210816-200054
>         git checkout ebbc41b9d3f7b7f6c558cb5d5f12ecb8134c9c2e
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=s390 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>

Note, this failure is expected as lookup_one() is in another tree. This
is mentioned in the commit message which obviously the automatic tool
can't be expected to figure out. But just an fyi, that there's nothing
to do right now. :)

Christian

RE: [PATCH] ksmbd: fix lookup on idmapped mounts

[Namjae Jeon]

> 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: 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>
> ---
Hi Christian,

> I merged David's for-next tree into cifsd-next to test this. I did only compile test this. If someone
> gives me a clear set of instructions how to test ksmbd on my local machine I can at least try to cut
> some time out of my week to do more reviews. (I'd especially like to see acl behavior with ksmbd.)

There is "How to run ksmbd" section in patch cover letter.
 https://lkml.org/lkml/2021/8/5/54

Let me know if it doesn't work well even if you try to run it with this step.
And We will also check whether your patch work fine.

> 
> One more thing, the tree for ksmbd was very hard to find. I had to do a lot archeology to end up at:
> 
> git://git.samba.org/ksmbd.git
This is also in the patch cover letter. See "Mailing list and repositories" section.
I think that you can use :
    https://github.com/namjaejeon/smb3-kernel/tree/ksmbd-v7-series

> 
> Would be appreciated if this tree could be reflected in MAINTAINERS or somewhere else. The github
> repos with the broken out patches/module aren't really that helpful.
Okay, I will add git address of ksmbd in MAINTAINERS on next spin.

> 
> Thanks!
> Christian
Really thanks for your review and I will apply this patch after checking it.

> ---
>  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 636570ecfa31..9a1a9a024714 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -3539,9 +3539,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)) {
> @@ -5242,7 +5242,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)
>  {
> @@ -5306,7 +5308,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);
> @@ -5620,6 +5622,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; @@ -5633,8 +5636,9 @@ static int
> set_rename_info(struct ksmbd_work *work, struct ksmbd_file *fp,
>  	if (ksmbd_stream_fd(fp))
>  		goto next;
> 
> +	user_ns = file_mnt_user_ns(fp->filp);
>  	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;
> @@ -5651,7 +5655,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 612c52d7a01b..8000ea3ade10 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);
> 
> base-commit: 456af438ad490bac7ed954cb929bcec1df7f0c82
> --
> 2.30.2

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Christian Brauner]

On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
> > 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: 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>
> > ---
> Hi Christian,
> 
> > I merged David's for-next tree into cifsd-next to test this. I did only compile test this. If someone
> > gives me a clear set of instructions how to test ksmbd on my local machine I can at least try to cut
> > some time out of my week to do more reviews. (I'd especially like to see acl behavior with ksmbd.)
> 
> There is "How to run ksmbd" section in patch cover letter.
>  https://lkml.org/lkml/2021/8/5/54
> 
> Let me know if it doesn't work well even if you try to run it with this step.
> And We will also check whether your patch work fine.
> 
> > 
> > One more thing, the tree for ksmbd was very hard to find. I had to do a lot archeology to end up at:
> > 
> > git://git.samba.org/ksmbd.git
> This is also in the patch cover letter. See "Mailing list and repositories" section.
> I think that you can use :
>     https://github.com/namjaejeon/smb3-kernel/tree/ksmbd-v7-series
> 
> > 
> > Would be appreciated if this tree could be reflected in MAINTAINERS or somewhere else. The github
> > repos with the broken out patches/module aren't really that helpful.
> Okay, I will add git address of ksmbd in MAINTAINERS on next spin.
> 
> > 
> > Thanks!
> > Christian
> Really thanks for your review and I will apply this patch after checking it.

Thank your for the pointers.

Ok, so I've been taking the time to look into cifs and ksmbd today. My
mental model was wrong. There are two things to consider here:

1. server: idmapped mounts with ksmbd
2. client: idmapped mounts with cifs

Your patchset adds support for 1.
Let's say I have the following ksmbd config:

root@f2-vm:~# cat /etc/ksmbd/smb.conf
[global]
        netbios name = SMBD
        server max protocol = SMB3
[test]
        path = /opt
        writeable = yes
        comment = TEST
        read only = no

So /opt can be an idmapped mount and ksmb would know how to deal with
that correctly, i.e. you could do:

mount-idmapped --map-mount=b:1000:0:1 /opt /opt

ksmbd.mountd

and ksmbd would take the idmapping of /opt into account.

That however is different from 2. which is cifs itself being idmappable.
Whether or not that makes sense or is needed will need some thinking.

In any case, this has consequences for xfstests and now I understand
your earlier confusion. In another mail you pointed out that cifs
reports that idmapped mounts are not supported. That is correct insofar
as it means 2. is not supported, i.e. you can't do:

mount -t cifs -o username=foo,password=bar //server/files /mnt

and then

mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt

but that's also not what you want in order to test for ksmbd. What you
want is to test 1.

So your test setup would require you to setup an idmapped mount and have
ksmbd use that which can then be mounted by a client.

With your instructions I was at least able to get a ksmb instance
running and be able to mount a client with -t cifs. All on the same
machine, i.e. my server is localhost.

However, I need to dig a bit into the semantics to make better
assertions about what's going on.

Are unix extension supported with ksmb? Everytime I try to use "posix"
as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I get
"uid=0" and "gid=0" and "noposix". I do set "unix extensions = yes" in
both the samba and ksmbd smb.conf.

Christian

RE: [PATCH] ksmbd: fix lookup on idmapped mounts

[Namjae Jeon]

> On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
> > > 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: 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>
> > > ---
> > Hi Christian,
> >
> > > I merged David's for-next tree into cifsd-next to test this. I did
> > > only compile test this. If someone gives me a clear set of
> > > instructions how to test ksmbd on my local machine I can at least
> > > try to cut some time out of my week to do more reviews. (I'd
> > > especially like to see acl behavior with ksmbd.)
> >
> > There is "How to run ksmbd" section in patch cover letter.
> >
> > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
> > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
> >
> > Let me know if it doesn't work well even if you try to run it with this step.
> > And We will also check whether your patch work fine.
> >
> > >
> > > One more thing, the tree for ksmbd was very hard to find. I had to do a lot archeology to end up
> at:
> > >
> > > git://git.samba.org/ksmbd.git
> > This is also in the patch cover letter. See "Mailing list and repositories" section.
> > I think that you can use :
> >
> > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
> > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
> > series
> >
> > >
> > > Would be appreciated if this tree could be reflected in MAINTAINERS
> > > or somewhere else. The github repos with the broken out patches/module aren't really that helpful.
> > Okay, I will add git address of ksmbd in MAINTAINERS on next spin.
> >
> > >
> > > Thanks!
> > > Christian
> > Really thanks for your review and I will apply this patch after checking it.
> 
> Thank your for the pointers.
> 
> Ok, so I've been taking the time to look into cifs and ksmbd today. My mental model was wrong. There
> are two things to consider here:
> 
> 1. server: idmapped mounts with ksmbd
> 2. client: idmapped mounts with cifs
> 
> Your patchset adds support for 1.
Right.

> Let's say I have the following ksmbd config:
> 
> root@f2-vm:~# cat /etc/ksmbd/smb.conf
> [global]
>         netbios name = SMBD
>         server max protocol = SMB3
> [test]
>         path = /opt
>         writeable = yes
>         comment = TEST
>         read only = no
> 
> So /opt can be an idmapped mount and ksmb would know how to deal with that correctly, i.e. you could
> do:
> 
> mount-idmapped --map-mount=b:1000:0:1 /opt /opt
> 
> ksmbd.mountd
> 
> and ksmbd would take the idmapping of /opt into account.
Right.

> 
> That however is different from 2. which is cifs itself being idmappable.
Right.

> Whether or not that makes sense or is needed will need some thinking.
> 
> In any case, this has consequences for xfstests and now I understand your earlier confusion. In
> another mail you pointed out that cifs reports that idmapped mounts are not supported. That is correct
> insofar as it means 2. is not supported, i.e. you can't do:
Right.

> 
> mount -t cifs -o username=foo,password=bar //server/files /mnt
> 
> and then
> 
> mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
> 
> but that's also not what you want in order to test for ksmbd. What you want is to test 1.
Right. So we have manually tested it, not xfstests.

> 
> So your test setup would require you to setup an idmapped mount and have ksmbd use that which can then
> be mounted by a client.
> 
> With your instructions I was at least able to get a ksmb instance running and be able to mount a
> client with -t cifs. All on the same machine, i.e. my server is localhost.
Okay.

> 
> However, I need to dig a bit into the semantics to make better assertions about what's going on.
Okay. And I have applied your patch to ksmbd.

> 
> Are unix extension supported with ksmb? Everytime I try to use "posix"
> as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I get "uid=0" and "gid=0" and "noposix".
> I do set "unix extensions = yes" in both the samba and ksmbd smb.conf.
With posix mount option, It should work. It worked well before but it is strange now.

I'm not sure this is the correct fix, But could you please try to mount with the below change ?

diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index eed59bc1d913..5fd0b0ddcc57 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
        case Opt_unix:
                if (result.negated)
                        ctx->linux_ext = 0;
-               else
+               else {
+                       ctx->linux_ext = 1;
                        ctx->no_linux_ext = 1;
+               }
                break;
        case Opt_nocase:
                ctx->nocase = 1;

Thanks for your work!
> 
> Christian

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Christian Brauner]

On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
> > > > 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: 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>
> > > > ---
> > > Hi Christian,
> > >
> > > > I merged David's for-next tree into cifsd-next to test this. I did
> > > > only compile test this. If someone gives me a clear set of
> > > > instructions how to test ksmbd on my local machine I can at least
> > > > try to cut some time out of my week to do more reviews. (I'd
> > > > especially like to see acl behavior with ksmbd.)
> > >
> > > There is "How to run ksmbd" section in patch cover letter.
> > >
> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
> > >
> > > Let me know if it doesn't work well even if you try to run it with this step.
> > > And We will also check whether your patch work fine.
> > >
> > > >
> > > > One more thing, the tree for ksmbd was very hard to find. I had to do a lot archeology to end up
> > at:
> > > >
> > > > git://git.samba.org/ksmbd.git
> > > This is also in the patch cover letter. See "Mailing list and repositories" section.
> > > I think that you can use :
> > >
> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
> > > series
> > >
> > > >
> > > > Would be appreciated if this tree could be reflected in MAINTAINERS
> > > > or somewhere else. The github repos with the broken out patches/module aren't really that helpful.
> > > Okay, I will add git address of ksmbd in MAINTAINERS on next spin.
> > >
> > > >
> > > > Thanks!
> > > > Christian
> > > Really thanks for your review and I will apply this patch after checking it.
> > 
> > Thank your for the pointers.
> > 
> > Ok, so I've been taking the time to look into cifs and ksmbd today. My mental model was wrong. There
> > are two things to consider here:
> > 
> > 1. server: idmapped mounts with ksmbd
> > 2. client: idmapped mounts with cifs
> > 
> > Your patchset adds support for 1.
> Right.
> 
> > Let's say I have the following ksmbd config:
> > 
> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
> > [global]
> >         netbios name = SMBD
> >         server max protocol = SMB3
> > [test]
> >         path = /opt
> >         writeable = yes
> >         comment = TEST
> >         read only = no
> > 
> > So /opt can be an idmapped mount and ksmb would know how to deal with that correctly, i.e. you could
> > do:
> > 
> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
> > 
> > ksmbd.mountd
> > 
> > and ksmbd would take the idmapping of /opt into account.
> Right.
> 
> > 
> > That however is different from 2. which is cifs itself being idmappable.
> Right.
> 
> > Whether or not that makes sense or is needed will need some thinking.
> > 
> > In any case, this has consequences for xfstests and now I understand your earlier confusion. In
> > another mail you pointed out that cifs reports that idmapped mounts are not supported. That is correct
> > insofar as it means 2. is not supported, i.e. you can't do:
> Right.
> 
> > 
> > mount -t cifs -o username=foo,password=bar //server/files /mnt
> > 
> > and then
> > 
> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
> > 
> > but that's also not what you want in order to test for ksmbd. What you want is to test 1.
> Right. So we have manually tested it, not xfstests.
> 
> > 
> > So your test setup would require you to setup an idmapped mount and have ksmbd use that which can then
> > be mounted by a client.
> > 
> > With your instructions I was at least able to get a ksmb instance running and be able to mount a
> > client with -t cifs. All on the same machine, i.e. my server is localhost.
> Okay.
> 
> > 
> > However, I need to dig a bit into the semantics to make better assertions about what's going on.
> Okay. And I have applied your patch to ksmbd.
> 
> > 
> > Are unix extension supported with ksmb? Everytime I try to use "posix"
> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I get "uid=0" and "gid=0" and "noposix".
> > I do set "unix extensions = yes" in both the samba and ksmbd smb.conf.
> With posix mount option, It should work. It worked well before but it is strange now.
> 
> I'm not sure this is the correct fix, But could you please try to mount with the below change ?
> 
> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> index eed59bc1d913..5fd0b0ddcc57 100644
> --- a/fs/cifs/fs_context.c
> +++ b/fs/cifs/fs_context.c
> @@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>         case Opt_unix:
>                 if (result.negated)
>                         ctx->linux_ext = 0;
> -               else
> +               else {
> +                       ctx->linux_ext = 1;
>                         ctx->no_linux_ext = 1;
> +               }
>                 break;
>         case Opt_nocase:
>                 ctx->nocase = 1;

That stops the bleeding indeed. :)
I think a slightly nicer fix might be:

diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index eed59bc1d913..424b8dc2232e 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -1269,7 +1269,8 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
                if (result.negated)
                        ctx->linux_ext = 0;
                else
-                       ctx->no_linux_ext = 1;
+                       ctx->linux_ext = 1;
+               ctx->no_linux_ext = !ctx->linux_ext;
                break;
        case Opt_nocase:
                ctx->nocase = 1;

So with this patch applied I got unix permissions working after all. So
I was able to do some more testing.

Just a few questions (independent of idmapped mounts):

1. Are the "uid=" and "gid=" mount options ignored when "username=" is
   specified and "posix" is specified?

   It seems that "uid=" and "gid=" have are silently ignored when
   "posix' is set. They are neither used to report file ownership under
   the cifs mountpoint nor are they relevant when determining ownership
   on disk?

   As an example, assume I have added a user "foo" with uid 1000 to
   ksmbd via:

           ksmbd.adduser -a foo

   And I mounted a share via:

           mount -t cifs -o username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid //127.0.0.1/test /mnt

   i) Ownership in /mnt appears posix-correct, i.e. "uid=" and "gid=" have
      no effect on the reported ownership.

   ii) Assume I'm logged in as the root user with uid 0. If I create
       file or directory in /mnt it will be owned by user foo, i.e. uid
       1000, i.e., the "uid=1234" and "gid=1234" mount option have zero
       effect on the ownership of the files?

2. Are the "uid=" and "gid=" options ignored for permission checking
   when "posix" is specified?

3. Am I correct in assuming that there is no mount option that makes
   chown() or chmod() have an actual effect.

   cifs seems to have support for it but the ksmbd server doesn't seem
   to?

   Both with "perm" and "noperm" chown() and chmod() report success but
   without actually changing ownership or mode. That seems to be in line
   with what I can gather from various mangpages.

Christian

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Namjae Jeon]

2021-08-19 22:01 GMT+09:00, Christian Brauner <christian.brauner@ubuntu.com>:
> On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
>> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
>> > > > 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: 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>
>> > > > ---
>> > > Hi Christian,
>> > >
>> > > > I merged David's for-next tree into cifsd-next to test this. I did
>> > > > only compile test this. If someone gives me a clear set of
>> > > > instructions how to test ksmbd on my local machine I can at least
>> > > > try to cut some time out of my week to do more reviews. (I'd
>> > > > especially like to see acl behavior with ksmbd.)
>> > >
>> > > There is "How to run ksmbd" section in patch cover letter.
>> > >
>> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
>> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
>> > >
>> > > Let me know if it doesn't work well even if you try to run it with
>> > > this step.
>> > > And We will also check whether your patch work fine.
>> > >
>> > > >
>> > > > One more thing, the tree for ksmbd was very hard to find. I had to
>> > > > do a lot archeology to end up
>> > at:
>> > > >
>> > > > git://git.samba.org/ksmbd.git
>> > > This is also in the patch cover letter. See "Mailing list and
>> > > repositories" section.
>> > > I think that you can use :
>> > >
>> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
>> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
>> > > series
>> > >
>> > > >
>> > > > Would be appreciated if this tree could be reflected in MAINTAINERS
>> > > > or somewhere else. The github repos with the broken out
>> > > > patches/module aren't really that helpful.
>> > > Okay, I will add git address of ksmbd in MAINTAINERS on next spin.
>> > >
>> > > >
>> > > > Thanks!
>> > > > Christian
>> > > Really thanks for your review and I will apply this patch after
>> > > checking it.
>> >
>> > Thank your for the pointers.
>> >
>> > Ok, so I've been taking the time to look into cifs and ksmbd today. My
>> > mental model was wrong. There
>> > are two things to consider here:
>> >
>> > 1. server: idmapped mounts with ksmbd
>> > 2. client: idmapped mounts with cifs
>> >
>> > Your patchset adds support for 1.
>> Right.
>>
>> > Let's say I have the following ksmbd config:
>> >
>> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
>> > [global]
>> >         netbios name = SMBD
>> >         server max protocol = SMB3
>> > [test]
>> >         path = /opt
>> >         writeable = yes
>> >         comment = TEST
>> >         read only = no
>> >
>> > So /opt can be an idmapped mount and ksmb would know how to deal with
>> > that correctly, i.e. you could
>> > do:
>> >
>> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
>> >
>> > ksmbd.mountd
>> >
>> > and ksmbd would take the idmapping of /opt into account.
>> Right.
>>
>> >
>> > That however is different from 2. which is cifs itself being
>> > idmappable.
>> Right.
>>
>> > Whether or not that makes sense or is needed will need some thinking.
>> >
>> > In any case, this has consequences for xfstests and now I understand
>> > your earlier confusion. In
>> > another mail you pointed out that cifs reports that idmapped mounts are
>> > not supported. That is correct
>> > insofar as it means 2. is not supported, i.e. you can't do:
>> Right.
>>
>> >
>> > mount -t cifs -o username=foo,password=bar //server/files /mnt
>> >
>> > and then
>> >
>> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
>> >
>> > but that's also not what you want in order to test for ksmbd. What you
>> > want is to test 1.
>> Right. So we have manually tested it, not xfstests.
>>
>> >
>> > So your test setup would require you to setup an idmapped mount and have
>> > ksmbd use that which can then
>> > be mounted by a client.
>> >
>> > With your instructions I was at least able to get a ksmb instance
>> > running and be able to mount a
>> > client with -t cifs. All on the same machine, i.e. my server is
>> > localhost.
>> Okay.
>>
>> >
>> > However, I need to dig a bit into the semantics to make better
>> > assertions about what's going on.
>> Okay. And I have applied your patch to ksmbd.
>>
>> >
>> > Are unix extension supported with ksmb? Everytime I try to use "posix"
>> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I get
>> > "uid=0" and "gid=0" and "noposix".
>> > I do set "unix extensions = yes" in both the samba and ksmbd smb.conf.
>> With posix mount option, It should work. It worked well before but it is
>> strange now.
>>
>> I'm not sure this is the correct fix, But could you please try to mount
>> with the below change ?
>>
>> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
>> index eed59bc1d913..5fd0b0ddcc57 100644
>> --- a/fs/cifs/fs_context.c
>> +++ b/fs/cifs/fs_context.c
>> @@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct
>> fs_context *fc,
>>         case Opt_unix:
>>                 if (result.negated)
>>                         ctx->linux_ext = 0;
>> -               else
>> +               else {
>> +                       ctx->linux_ext = 1;
>>                         ctx->no_linux_ext = 1;
>> +               }
>>                 break;
>>         case Opt_nocase:
>>                 ctx->nocase = 1;
>
> That stops the bleeding indeed. :)
Okay, sorry for late response.
> I think a slightly nicer fix might be:
>
> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> index eed59bc1d913..424b8dc2232e 100644
> --- a/fs/cifs/fs_context.c
> +++ b/fs/cifs/fs_context.c
> @@ -1269,7 +1269,8 @@ static int smb3_fs_context_parse_param(struct
> fs_context *fc,
>                 if (result.negated)
>                         ctx->linux_ext = 0;
>                 else
> -                       ctx->no_linux_ext = 1;
> +                       ctx->linux_ext = 1;
> +               ctx->no_linux_ext = !ctx->linux_ext;
>                 break;
>         case Opt_nocase:
>                 ctx->nocase = 1;
>
> So with this patch applied I got unix permissions working after all. So
> I was able to do some more testing.
Okay.
>
> Just a few questions (independent of idmapped mounts):
>
> 1. Are the "uid=" and "gid=" mount options ignored when "username=" is
>    specified and "posix" is specified?
>
>    It seems that "uid=" and "gid=" have are silently ignored when
>    "posix' is set. They are neither used to report file ownership under
>    the cifs mountpoint nor are they relevant when determining ownership
>    on disk?
>
>    As an example, assume I have added a user "foo" with uid 1000 to
>    ksmbd via:
>
>            ksmbd.adduser -a foo
>
>    And I mounted a share via:
>
>            mount -t cifs -o
> username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid
> //127.0.0.1/test /mnt
>
>    i) Ownership in /mnt appears posix-correct, i.e. "uid=" and "gid=" have
>       no effect on the reported ownership.
>
>    ii) Assume I'm logged in as the root user with uid 0. If I create
>        file or directory in /mnt it will be owned by user foo, i.e. uid
>        1000, i.e., the "uid=1234" and "gid=1234" mount option have zero
>        effect on the ownership of the files?
>
> 2. Are the "uid=" and "gid=" options ignored for permission checking
>    when "posix" is specified?
>
> 3. Am I correct in assuming that there is no mount option that makes
>    chown() or chmod() have an actual effect.
That will be an answer for 1,2, 3 question. There are mount options for this.
 1. modefromsid
 2. idsfromsid

You can use this mount option and please check it.
>
>    cifs seems to have support for it but the ksmbd server doesn't seem
>    to?
No, you need to use mount options for this as I said.
ksmbd have supported it but I found an issue related to chown and have fixed.

Could you please check the below git branch (pulled David'tree + ksmbd fixes) ?

  git clone --branch=for-christian https://github.com/namjaejeon/smb3-kernel

Thanks!
>
>    Both with "perm" and "noperm" chown() and chmod() report success but
>    without actually changing ownership or mode. That seems to be in line
>    with what I can gather from various mangpages.
>
> Christian
>

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Christian Brauner]

On Sat, Aug 21, 2021 at 02:59:21PM +0900, Namjae Jeon wrote:
> 2021-08-19 22:01 GMT+09:00, Christian Brauner <christian.brauner@ubuntu.com>:
> > On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
> >> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
> >> > > > 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: 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>
> >> > > > ---
> >> > > Hi Christian,
> >> > >
> >> > > > I merged David's for-next tree into cifsd-next to test this. I did
> >> > > > only compile test this. If someone gives me a clear set of
> >> > > > instructions how to test ksmbd on my local machine I can at least
> >> > > > try to cut some time out of my week to do more reviews. (I'd
> >> > > > especially like to see acl behavior with ksmbd.)
> >> > >
> >> > > There is "How to run ksmbd" section in patch cover letter.
> >> > >
> >> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
> >> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> >> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
> >> > >
> >> > > Let me know if it doesn't work well even if you try to run it with
> >> > > this step.
> >> > > And We will also check whether your patch work fine.
> >> > >
> >> > > >
> >> > > > One more thing, the tree for ksmbd was very hard to find. I had to
> >> > > > do a lot archeology to end up
> >> > at:
> >> > > >
> >> > > > git://git.samba.org/ksmbd.git
> >> > > This is also in the patch cover letter. See "Mailing list and
> >> > > repositories" section.
> >> > > I think that you can use :
> >> > >
> >> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
> >> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> >> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
> >> > > series
> >> > >
> >> > > >
> >> > > > Would be appreciated if this tree could be reflected in MAINTAINERS
> >> > > > or somewhere else. The github repos with the broken out
> >> > > > patches/module aren't really that helpful.
> >> > > Okay, I will add git address of ksmbd in MAINTAINERS on next spin.
> >> > >
> >> > > >
> >> > > > Thanks!
> >> > > > Christian
> >> > > Really thanks for your review and I will apply this patch after
> >> > > checking it.
> >> >
> >> > Thank your for the pointers.
> >> >
> >> > Ok, so I've been taking the time to look into cifs and ksmbd today. My
> >> > mental model was wrong. There
> >> > are two things to consider here:
> >> >
> >> > 1. server: idmapped mounts with ksmbd
> >> > 2. client: idmapped mounts with cifs
> >> >
> >> > Your patchset adds support for 1.
> >> Right.
> >>
> >> > Let's say I have the following ksmbd config:
> >> >
> >> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
> >> > [global]
> >> >         netbios name = SMBD
> >> >         server max protocol = SMB3
> >> > [test]
> >> >         path = /opt
> >> >         writeable = yes
> >> >         comment = TEST
> >> >         read only = no
> >> >
> >> > So /opt can be an idmapped mount and ksmb would know how to deal with
> >> > that correctly, i.e. you could
> >> > do:
> >> >
> >> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
> >> >
> >> > ksmbd.mountd
> >> >
> >> > and ksmbd would take the idmapping of /opt into account.
> >> Right.
> >>
> >> >
> >> > That however is different from 2. which is cifs itself being
> >> > idmappable.
> >> Right.
> >>
> >> > Whether or not that makes sense or is needed will need some thinking.
> >> >
> >> > In any case, this has consequences for xfstests and now I understand
> >> > your earlier confusion. In
> >> > another mail you pointed out that cifs reports that idmapped mounts are
> >> > not supported. That is correct
> >> > insofar as it means 2. is not supported, i.e. you can't do:
> >> Right.
> >>
> >> >
> >> > mount -t cifs -o username=foo,password=bar //server/files /mnt
> >> >
> >> > and then
> >> >
> >> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
> >> >
> >> > but that's also not what you want in order to test for ksmbd. What you
> >> > want is to test 1.
> >> Right. So we have manually tested it, not xfstests.
> >>
> >> >
> >> > So your test setup would require you to setup an idmapped mount and have
> >> > ksmbd use that which can then
> >> > be mounted by a client.
> >> >
> >> > With your instructions I was at least able to get a ksmb instance
> >> > running and be able to mount a
> >> > client with -t cifs. All on the same machine, i.e. my server is
> >> > localhost.
> >> Okay.
> >>
> >> >
> >> > However, I need to dig a bit into the semantics to make better
> >> > assertions about what's going on.
> >> Okay. And I have applied your patch to ksmbd.
> >>
> >> >
> >> > Are unix extension supported with ksmb? Everytime I try to use "posix"
> >> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I get
> >> > "uid=0" and "gid=0" and "noposix".
> >> > I do set "unix extensions = yes" in both the samba and ksmbd smb.conf.
> >> With posix mount option, It should work. It worked well before but it is
> >> strange now.
> >>
> >> I'm not sure this is the correct fix, But could you please try to mount
> >> with the below change ?
> >>
> >> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> >> index eed59bc1d913..5fd0b0ddcc57 100644
> >> --- a/fs/cifs/fs_context.c
> >> +++ b/fs/cifs/fs_context.c
> >> @@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct
> >> fs_context *fc,
> >>         case Opt_unix:
> >>                 if (result.negated)
> >>                         ctx->linux_ext = 0;
> >> -               else
> >> +               else {
> >> +                       ctx->linux_ext = 1;
> >>                         ctx->no_linux_ext = 1;
> >> +               }
> >>                 break;
> >>         case Opt_nocase:
> >>                 ctx->nocase = 1;
> >
> > That stops the bleeding indeed. :)
> Okay, sorry for late response.
> > I think a slightly nicer fix might be:
> >
> > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> > index eed59bc1d913..424b8dc2232e 100644
> > --- a/fs/cifs/fs_context.c
> > +++ b/fs/cifs/fs_context.c
> > @@ -1269,7 +1269,8 @@ static int smb3_fs_context_parse_param(struct
> > fs_context *fc,
> >                 if (result.negated)
> >                         ctx->linux_ext = 0;
> >                 else
> > -                       ctx->no_linux_ext = 1;
> > +                       ctx->linux_ext = 1;
> > +               ctx->no_linux_ext = !ctx->linux_ext;
> >                 break;
> >         case Opt_nocase:
> >                 ctx->nocase = 1;
> >
> > So with this patch applied I got unix permissions working after all. So
> > I was able to do some more testing.
> Okay.
> >
> > Just a few questions (independent of idmapped mounts):
> >
> > 1. Are the "uid=" and "gid=" mount options ignored when "username=" is
> >    specified and "posix" is specified?
> >
> >    It seems that "uid=" and "gid=" have are silently ignored when
> >    "posix' is set. They are neither used to report file ownership under
> >    the cifs mountpoint nor are they relevant when determining ownership
> >    on disk?
> >
> >    As an example, assume I have added a user "foo" with uid 1000 to
> >    ksmbd via:
> >
> >            ksmbd.adduser -a foo
> >
> >    And I mounted a share via:
> >
> >            mount -t cifs -o
> > username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid
> > //127.0.0.1/test /mnt
> >
> >    i) Ownership in /mnt appears posix-correct, i.e. "uid=" and "gid=" have
> >       no effect on the reported ownership.
> >
> >    ii) Assume I'm logged in as the root user with uid 0. If I create
> >        file or directory in /mnt it will be owned by user foo, i.e. uid
> >        1000, i.e., the "uid=1234" and "gid=1234" mount option have zero
> >        effect on the ownership of the files?
> >
> > 2. Are the "uid=" and "gid=" options ignored for permission checking
> >    when "posix" is specified?
> >
> > 3. Am I correct in assuming that there is no mount option that makes
> >    chown() or chmod() have an actual effect.
> That will be an answer for 1,2, 3 question. There are mount options for this.
>  1. modefromsid
>  2. idsfromsid
> 
> You can use this mount option and please check it.

Thank you! This works and finally I can hit some codepaths I wasn't able
to until now.

> >
> >    cifs seems to have support for it but the ksmbd server doesn't seem
> >    to?
> No, you need to use mount options for this as I said.
> ksmbd have supported it but I found an issue related to chown and have fixed.
> 
> Could you please check the below git branch (pulled David'tree + ksmbd fixes) ?
> 
>   git clone --branch=for-christian https://github.com/namjaejeon/smb3-kernel

Thanks, I've pulled that branch.

I have a some patches for ksmb that I'll be sending out next week. I
just need to test the changes and verify that it all makes sense.

Christian

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Christian Brauner]

On Sat, Aug 21, 2021 at 01:11:17PM +0200, Christian Brauner wrote:
> On Sat, Aug 21, 2021 at 02:59:21PM +0900, Namjae Jeon wrote:
> > 2021-08-19 22:01 GMT+09:00, Christian Brauner <christian.brauner@ubuntu.com>:
> > > On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
> > >> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
> > >> > > > 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: 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>
> > >> > > > ---
> > >> > > Hi Christian,
> > >> > >
> > >> > > > I merged David's for-next tree into cifsd-next to test this. I did
> > >> > > > only compile test this. If someone gives me a clear set of
> > >> > > > instructions how to test ksmbd on my local machine I can at least
> > >> > > > try to cut some time out of my week to do more reviews. (I'd
> > >> > > > especially like to see acl behavior with ksmbd.)
> > >> > >
> > >> > > There is "How to run ksmbd" section in patch cover letter.
> > >> > >
> > >> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
> > >> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> > >> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
> > >> > >
> > >> > > Let me know if it doesn't work well even if you try to run it with
> > >> > > this step.
> > >> > > And We will also check whether your patch work fine.
> > >> > >
> > >> > > >
> > >> > > > One more thing, the tree for ksmbd was very hard to find. I had to
> > >> > > > do a lot archeology to end up
> > >> > at:
> > >> > > >
> > >> > > > git://git.samba.org/ksmbd.git
> > >> > > This is also in the patch cover letter. See "Mailing list and
> > >> > > repositories" section.
> > >> > > I think that you can use :
> > >> > >
> > >> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
> > >> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> > >> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
> > >> > > series
> > >> > >
> > >> > > >
> > >> > > > Would be appreciated if this tree could be reflected in MAINTAINERS
> > >> > > > or somewhere else. The github repos with the broken out
> > >> > > > patches/module aren't really that helpful.
> > >> > > Okay, I will add git address of ksmbd in MAINTAINERS on next spin.
> > >> > >
> > >> > > >
> > >> > > > Thanks!
> > >> > > > Christian
> > >> > > Really thanks for your review and I will apply this patch after
> > >> > > checking it.
> > >> >
> > >> > Thank your for the pointers.
> > >> >
> > >> > Ok, so I've been taking the time to look into cifs and ksmbd today. My
> > >> > mental model was wrong. There
> > >> > are two things to consider here:
> > >> >
> > >> > 1. server: idmapped mounts with ksmbd
> > >> > 2. client: idmapped mounts with cifs
> > >> >
> > >> > Your patchset adds support for 1.
> > >> Right.
> > >>
> > >> > Let's say I have the following ksmbd config:
> > >> >
> > >> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
> > >> > [global]
> > >> >         netbios name = SMBD
> > >> >         server max protocol = SMB3
> > >> > [test]
> > >> >         path = /opt
> > >> >         writeable = yes
> > >> >         comment = TEST
> > >> >         read only = no
> > >> >
> > >> > So /opt can be an idmapped mount and ksmb would know how to deal with
> > >> > that correctly, i.e. you could
> > >> > do:
> > >> >
> > >> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
> > >> >
> > >> > ksmbd.mountd
> > >> >
> > >> > and ksmbd would take the idmapping of /opt into account.
> > >> Right.
> > >>
> > >> >
> > >> > That however is different from 2. which is cifs itself being
> > >> > idmappable.
> > >> Right.
> > >>
> > >> > Whether or not that makes sense or is needed will need some thinking.
> > >> >
> > >> > In any case, this has consequences for xfstests and now I understand
> > >> > your earlier confusion. In
> > >> > another mail you pointed out that cifs reports that idmapped mounts are
> > >> > not supported. That is correct
> > >> > insofar as it means 2. is not supported, i.e. you can't do:
> > >> Right.
> > >>
> > >> >
> > >> > mount -t cifs -o username=foo,password=bar //server/files /mnt
> > >> >
> > >> > and then
> > >> >
> > >> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
> > >> >
> > >> > but that's also not what you want in order to test for ksmbd. What you
> > >> > want is to test 1.
> > >> Right. So we have manually tested it, not xfstests.
> > >>
> > >> >
> > >> > So your test setup would require you to setup an idmapped mount and have
> > >> > ksmbd use that which can then
> > >> > be mounted by a client.
> > >> >
> > >> > With your instructions I was at least able to get a ksmb instance
> > >> > running and be able to mount a
> > >> > client with -t cifs. All on the same machine, i.e. my server is
> > >> > localhost.
> > >> Okay.
> > >>
> > >> >
> > >> > However, I need to dig a bit into the semantics to make better
> > >> > assertions about what's going on.
> > >> Okay. And I have applied your patch to ksmbd.
> > >>
> > >> >
> > >> > Are unix extension supported with ksmb? Everytime I try to use "posix"
> > >> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I get
> > >> > "uid=0" and "gid=0" and "noposix".
> > >> > I do set "unix extensions = yes" in both the samba and ksmbd smb.conf.
> > >> With posix mount option, It should work. It worked well before but it is
> > >> strange now.
> > >>
> > >> I'm not sure this is the correct fix, But could you please try to mount
> > >> with the below change ?
> > >>
> > >> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> > >> index eed59bc1d913..5fd0b0ddcc57 100644
> > >> --- a/fs/cifs/fs_context.c
> > >> +++ b/fs/cifs/fs_context.c
> > >> @@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct
> > >> fs_context *fc,
> > >>         case Opt_unix:
> > >>                 if (result.negated)
> > >>                         ctx->linux_ext = 0;
> > >> -               else
> > >> +               else {
> > >> +                       ctx->linux_ext = 1;
> > >>                         ctx->no_linux_ext = 1;
> > >> +               }
> > >>                 break;
> > >>         case Opt_nocase:
> > >>                 ctx->nocase = 1;
> > >
> > > That stops the bleeding indeed. :)
> > Okay, sorry for late response.
> > > I think a slightly nicer fix might be:
> > >
> > > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> > > index eed59bc1d913..424b8dc2232e 100644
> > > --- a/fs/cifs/fs_context.c
> > > +++ b/fs/cifs/fs_context.c
> > > @@ -1269,7 +1269,8 @@ static int smb3_fs_context_parse_param(struct
> > > fs_context *fc,
> > >                 if (result.negated)
> > >                         ctx->linux_ext = 0;
> > >                 else
> > > -                       ctx->no_linux_ext = 1;
> > > +                       ctx->linux_ext = 1;
> > > +               ctx->no_linux_ext = !ctx->linux_ext;
> > >                 break;
> > >         case Opt_nocase:
> > >                 ctx->nocase = 1;
> > >
> > > So with this patch applied I got unix permissions working after all. So
> > > I was able to do some more testing.
> > Okay.
> > >
> > > Just a few questions (independent of idmapped mounts):
> > >
> > > 1. Are the "uid=" and "gid=" mount options ignored when "username=" is
> > >    specified and "posix" is specified?
> > >
> > >    It seems that "uid=" and "gid=" have are silently ignored when
> > >    "posix' is set. They are neither used to report file ownership under
> > >    the cifs mountpoint nor are they relevant when determining ownership
> > >    on disk?
> > >
> > >    As an example, assume I have added a user "foo" with uid 1000 to
> > >    ksmbd via:
> > >
> > >            ksmbd.adduser -a foo
> > >
> > >    And I mounted a share via:
> > >
> > >            mount -t cifs -o
> > > username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid
> > > //127.0.0.1/test /mnt
> > >
> > >    i) Ownership in /mnt appears posix-correct, i.e. "uid=" and "gid=" have
> > >       no effect on the reported ownership.
> > >
> > >    ii) Assume I'm logged in as the root user with uid 0. If I create
> > >        file or directory in /mnt it will be owned by user foo, i.e. uid
> > >        1000, i.e., the "uid=1234" and "gid=1234" mount option have zero
> > >        effect on the ownership of the files?
> > >
> > > 2. Are the "uid=" and "gid=" options ignored for permission checking
> > >    when "posix" is specified?
> > >
> > > 3. Am I correct in assuming that there is no mount option that makes
> > >    chown() or chmod() have an actual effect.
> > That will be an answer for 1,2, 3 question. There are mount options for this.
> >  1. modefromsid
> >  2. idsfromsid
> > 
> > You can use this mount option and please check it.
> 
> Thank you! This works and finally I can hit some codepaths I wasn't able
> to until now.
> 
> > >
> > >    cifs seems to have support for it but the ksmbd server doesn't seem
> > >    to?
> > No, you need to use mount options for this as I said.
> > ksmbd have supported it but I found an issue related to chown and have fixed.
> > 
> > Could you please check the below git branch (pulled David'tree + ksmbd fixes) ?
> > 
> >   git clone --branch=for-christian https://github.com/namjaejeon/smb3-kernel
> 
> Thanks, I've pulled that branch.

There's two problems in this patch. I'll post and point out here
quickly, if you don't mind:

commit fd7d13c387798cbc3abd68ecc07b2c868c6d96cb
Author: Namjae Jeon <namjae.jeon@samsung.com>
Date:   Sat Aug 21 14:39:43 2021 +0900

    ksmbd: fix permission check issue on chown and chmod

    When commanding chmod and chown on cifs&ksmbd, ksmbd allows it without file
    permissions check. There is code to check it in settattr_prepare.
    Instead of setting the inode directly, update the mode and uid/gid
    through notify_change.

    Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
    Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 0131997c2177..d329ea49fa14 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -5861,10 +5861,15 @@ int smb2_set_info(struct ksmbd_work *work)
                break;
        case SMB2_O_INFO_SECURITY:
                ksmbd_debug(SMB, "GOT SMB2_O_INFO_SECURITY\n");
+               if (ksmbd_override_fsids(work)) {
+                       rc = -ENOMEM;
+                       goto err_out;
+               }
                rc = smb2_set_info_sec(fp,
                                       le32_to_cpu(req->AdditionalInformation),
                                       req->Buffer,
                                       le32_to_cpu(req->BufferLength));
+               ksmbd_revert_fsids(work);
                break;
        default:
                rc = -EOPNOTSUPP;
diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
index 20455d810523..f28af33c0bd5 100644
--- a/fs/ksmbd/smbacl.c
+++ b/fs/ksmbd/smbacl.c
@@ -1300,6 +1300,7 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
        struct smb_fattr fattr = {{0}};
        struct inode *inode = d_inode(path->dentry);
        struct user_namespace *user_ns = mnt_user_ns(path->mnt);
+       struct iattr newattrs;

        fattr.cf_uid = INVALID_UID;
        fattr.cf_gid = INVALID_GID;
@@ -1309,12 +1310,28 @@ int set_info_sec(struct ksmbd_conn *conn, struct ksmbd_tree_connect *tcon,
        if (rc)
                goto out;

-       inode->i_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
-       if (!uid_eq(fattr.cf_uid, INVALID_UID))
-               inode->i_uid = fattr.cf_uid;
-       if (!gid_eq(fattr.cf_gid, INVALID_GID))
+       newattrs.ia_valid = ATTR_CTIME;
+       if (!uid_eq(fattr.cf_uid, INVALID_UID)) {
+               newattrs.ia_valid |= ATTR_UID;
+               newattrs.ia_uid = fattr.cf_uid;
+       }
+       if (!gid_eq(fattr.cf_gid, INVALID_GID)) {
                inode->i_gid = fattr.cf_gid;

This needs to be removed. If setattr_prepare() fails you will still end
up with inode->i_gid changed, i.e. you're changing group ownership even
if you fail the subsequent setattr_prepare().

-       mark_inode_dirty(inode);
+               newattrs.ia_valid |= ATTR_GID;
+               newattrs.ia_gid = fattr.cf_gid;
+       }
+       newattrs.ia_valid |= ATTR_MODE;
+       newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
+       rc = setattr_prepare(user_ns, path->dentry, &newattrs);
+       if (rc)
+               goto out;
+
+       inode_lock(inode);
+       setattr_copy(user_ns, inode, &newattrs);

This needs to be removed. setattr_copy() will be called in the
i_io->setattr()
method of the underlying filesystem which notify_change() will all.

Right now, with the setattr_copy() will commit the changes to the inode.
If the following notify_change() call fails you end up with a change in
ownership that should've failed.

Christian

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Namjae Jeon]

2021-08-21 20:39 GMT+09:00, Christian Brauner <christian.brauner@ubuntu.com>:
> On Sat, Aug 21, 2021 at 01:11:17PM +0200, Christian Brauner wrote:
>> On Sat, Aug 21, 2021 at 02:59:21PM +0900, Namjae Jeon wrote:
>> > 2021-08-19 22:01 GMT+09:00, Christian Brauner
>> > <christian.brauner@ubuntu.com>:
>> > > On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
>> > >> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
>> > >> > > > 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: 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>
>> > >> > > > ---
>> > >> > > Hi Christian,
>> > >> > >
>> > >> > > > I merged David's for-next tree into cifsd-next to test this. I
>> > >> > > > did
>> > >> > > > only compile test this. If someone gives me a clear set of
>> > >> > > > instructions how to test ksmbd on my local machine I can at
>> > >> > > > least
>> > >> > > > try to cut some time out of my week to do more reviews. (I'd
>> > >> > > > especially like to see acl behavior with ksmbd.)
>> > >> > >
>> > >> > > There is "How to run ksmbd" section in patch cover letter.
>> > >> > >
>> > >> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
>> > >> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> > >> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
>> > >> > >
>> > >> > > Let me know if it doesn't work well even if you try to run it
>> > >> > > with
>> > >> > > this step.
>> > >> > > And We will also check whether your patch work fine.
>> > >> > >
>> > >> > > >
>> > >> > > > One more thing, the tree for ksmbd was very hard to find. I had
>> > >> > > > to
>> > >> > > > do a lot archeology to end up
>> > >> > at:
>> > >> > > >
>> > >> > > > git://git.samba.org/ksmbd.git
>> > >> > > This is also in the patch cover letter. See "Mailing list and
>> > >> > > repositories" section.
>> > >> > > I think that you can use :
>> > >> > >
>> > >> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
>> > >> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> > >> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
>> > >> > > series
>> > >> > >
>> > >> > > >
>> > >> > > > Would be appreciated if this tree could be reflected in
>> > >> > > > MAINTAINERS
>> > >> > > > or somewhere else. The github repos with the broken out
>> > >> > > > patches/module aren't really that helpful.
>> > >> > > Okay, I will add git address of ksmbd in MAINTAINERS on next
>> > >> > > spin.
>> > >> > >
>> > >> > > >
>> > >> > > > Thanks!
>> > >> > > > Christian
>> > >> > > Really thanks for your review and I will apply this patch after
>> > >> > > checking it.
>> > >> >
>> > >> > Thank your for the pointers.
>> > >> >
>> > >> > Ok, so I've been taking the time to look into cifs and ksmbd today.
>> > >> > My
>> > >> > mental model was wrong. There
>> > >> > are two things to consider here:
>> > >> >
>> > >> > 1. server: idmapped mounts with ksmbd
>> > >> > 2. client: idmapped mounts with cifs
>> > >> >
>> > >> > Your patchset adds support for 1.
>> > >> Right.
>> > >>
>> > >> > Let's say I have the following ksmbd config:
>> > >> >
>> > >> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
>> > >> > [global]
>> > >> >         netbios name = SMBD
>> > >> >         server max protocol = SMB3
>> > >> > [test]
>> > >> >         path = /opt
>> > >> >         writeable = yes
>> > >> >         comment = TEST
>> > >> >         read only = no
>> > >> >
>> > >> > So /opt can be an idmapped mount and ksmb would know how to deal
>> > >> > with
>> > >> > that correctly, i.e. you could
>> > >> > do:
>> > >> >
>> > >> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
>> > >> >
>> > >> > ksmbd.mountd
>> > >> >
>> > >> > and ksmbd would take the idmapping of /opt into account.
>> > >> Right.
>> > >>
>> > >> >
>> > >> > That however is different from 2. which is cifs itself being
>> > >> > idmappable.
>> > >> Right.
>> > >>
>> > >> > Whether or not that makes sense or is needed will need some
>> > >> > thinking.
>> > >> >
>> > >> > In any case, this has consequences for xfstests and now I
>> > >> > understand
>> > >> > your earlier confusion. In
>> > >> > another mail you pointed out that cifs reports that idmapped mounts
>> > >> > are
>> > >> > not supported. That is correct
>> > >> > insofar as it means 2. is not supported, i.e. you can't do:
>> > >> Right.
>> > >>
>> > >> >
>> > >> > mount -t cifs -o username=foo,password=bar //server/files /mnt
>> > >> >
>> > >> > and then
>> > >> >
>> > >> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
>> > >> >
>> > >> > but that's also not what you want in order to test for ksmbd. What
>> > >> > you
>> > >> > want is to test 1.
>> > >> Right. So we have manually tested it, not xfstests.
>> > >>
>> > >> >
>> > >> > So your test setup would require you to setup an idmapped mount and
>> > >> > have
>> > >> > ksmbd use that which can then
>> > >> > be mounted by a client.
>> > >> >
>> > >> > With your instructions I was at least able to get a ksmb instance
>> > >> > running and be able to mount a
>> > >> > client with -t cifs. All on the same machine, i.e. my server is
>> > >> > localhost.
>> > >> Okay.
>> > >>
>> > >> >
>> > >> > However, I need to dig a bit into the semantics to make better
>> > >> > assertions about what's going on.
>> > >> Okay. And I have applied your patch to ksmbd.
>> > >>
>> > >> >
>> > >> > Are unix extension supported with ksmb? Everytime I try to use
>> > >> > "posix"
>> > >> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I
>> > >> > get
>> > >> > "uid=0" and "gid=0" and "noposix".
>> > >> > I do set "unix extensions = yes" in both the samba and ksmbd
>> > >> > smb.conf.
>> > >> With posix mount option, It should work. It worked well before but it
>> > >> is
>> > >> strange now.
>> > >>
>> > >> I'm not sure this is the correct fix, But could you please try to
>> > >> mount
>> > >> with the below change ?
>> > >>
>> > >> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
>> > >> index eed59bc1d913..5fd0b0ddcc57 100644
>> > >> --- a/fs/cifs/fs_context.c
>> > >> +++ b/fs/cifs/fs_context.c
>> > >> @@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct
>> > >> fs_context *fc,
>> > >>         case Opt_unix:
>> > >>                 if (result.negated)
>> > >>                         ctx->linux_ext = 0;
>> > >> -               else
>> > >> +               else {
>> > >> +                       ctx->linux_ext = 1;
>> > >>                         ctx->no_linux_ext = 1;
>> > >> +               }
>> > >>                 break;
>> > >>         case Opt_nocase:
>> > >>                 ctx->nocase = 1;
>> > >
>> > > That stops the bleeding indeed. :)
>> > Okay, sorry for late response.
>> > > I think a slightly nicer fix might be:
>> > >
>> > > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
>> > > index eed59bc1d913..424b8dc2232e 100644
>> > > --- a/fs/cifs/fs_context.c
>> > > +++ b/fs/cifs/fs_context.c
>> > > @@ -1269,7 +1269,8 @@ static int smb3_fs_context_parse_param(struct
>> > > fs_context *fc,
>> > >                 if (result.negated)
>> > >                         ctx->linux_ext = 0;
>> > >                 else
>> > > -                       ctx->no_linux_ext = 1;
>> > > +                       ctx->linux_ext = 1;
>> > > +               ctx->no_linux_ext = !ctx->linux_ext;
>> > >                 break;
>> > >         case Opt_nocase:
>> > >                 ctx->nocase = 1;
>> > >
>> > > So with this patch applied I got unix permissions working after all.
>> > > So
>> > > I was able to do some more testing.
>> > Okay.
>> > >
>> > > Just a few questions (independent of idmapped mounts):
>> > >
>> > > 1. Are the "uid=" and "gid=" mount options ignored when "username="
>> > > is
>> > >    specified and "posix" is specified?
>> > >
>> > >    It seems that "uid=" and "gid=" have are silently ignored when
>> > >    "posix' is set. They are neither used to report file ownership
>> > > under
>> > >    the cifs mountpoint nor are they relevant when determining
>> > > ownership
>> > >    on disk?
>> > >
>> > >    As an example, assume I have added a user "foo" with uid 1000 to
>> > >    ksmbd via:
>> > >
>> > >            ksmbd.adduser -a foo
>> > >
>> > >    And I mounted a share via:
>> > >
>> > >            mount -t cifs -o
>> > > username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid
>> > > //127.0.0.1/test /mnt
>> > >
>> > >    i) Ownership in /mnt appears posix-correct, i.e. "uid=" and "gid="
>> > > have
>> > >       no effect on the reported ownership.
>> > >
>> > >    ii) Assume I'm logged in as the root user with uid 0. If I create
>> > >        file or directory in /mnt it will be owned by user foo, i.e.
>> > > uid
>> > >        1000, i.e., the "uid=1234" and "gid=1234" mount option have
>> > > zero
>> > >        effect on the ownership of the files?
>> > >
>> > > 2. Are the "uid=" and "gid=" options ignored for permission checking
>> > >    when "posix" is specified?
>> > >
>> > > 3. Am I correct in assuming that there is no mount option that makes
>> > >    chown() or chmod() have an actual effect.
>> > That will be an answer for 1,2, 3 question. There are mount options for
>> > this.
>> >  1. modefromsid
>> >  2. idsfromsid
>> >
>> > You can use this mount option and please check it.
>>
>> Thank you! This works and finally I can hit some codepaths I wasn't able
>> to until now.
>>
>> > >
>> > >    cifs seems to have support for it but the ksmbd server doesn't
>> > > seem
>> > >    to?
>> > No, you need to use mount options for this as I said.
>> > ksmbd have supported it but I found an issue related to chown and have
>> > fixed.
>> >
>> > Could you please check the below git branch (pulled David'tree + ksmbd
>> > fixes) ?
>> >
>> >   git clone --branch=for-christian
>> > https://github.com/namjaejeon/smb3-kernel
>>
>> Thanks, I've pulled that branch.
>
> There's two problems in this patch. I'll post and point out here
> quickly, if you don't mind:
Thanks for your reiview!
>
> commit fd7d13c387798cbc3abd68ecc07b2c868c6d96cb
> Author: Namjae Jeon <namjae.jeon@samsung.com>
> Date:   Sat Aug 21 14:39:43 2021 +0900
>
>     ksmbd: fix permission check issue on chown and chmod
>
>     When commanding chmod and chown on cifs&ksmbd, ksmbd allows it without
> file
>     permissions check. There is code to check it in settattr_prepare.
>     Instead of setting the inode directly, update the mode and uid/gid
>     through notify_change.
>
>     Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
>     Signed-off-by: Steve French <stfrench@microsoft.com>
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 0131997c2177..d329ea49fa14 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -5861,10 +5861,15 @@ int smb2_set_info(struct ksmbd_work *work)
>                 break;
>         case SMB2_O_INFO_SECURITY:
>                 ksmbd_debug(SMB, "GOT SMB2_O_INFO_SECURITY\n");
> +               if (ksmbd_override_fsids(work)) {
> +                       rc = -ENOMEM;
> +                       goto err_out;
> +               }
>                 rc = smb2_set_info_sec(fp,
>
> le32_to_cpu(req->AdditionalInformation),
>                                        req->Buffer,
>                                        le32_to_cpu(req->BufferLength));
> +               ksmbd_revert_fsids(work);
>                 break;
>         default:
>                 rc = -EOPNOTSUPP;
> diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
> index 20455d810523..f28af33c0bd5 100644
> --- a/fs/ksmbd/smbacl.c
> +++ b/fs/ksmbd/smbacl.c
> @@ -1300,6 +1300,7 @@ int set_info_sec(struct ksmbd_conn *conn, struct
> ksmbd_tree_connect *tcon,
>         struct smb_fattr fattr = {{0}};
>         struct inode *inode = d_inode(path->dentry);
>         struct user_namespace *user_ns = mnt_user_ns(path->mnt);
> +       struct iattr newattrs;
>
>         fattr.cf_uid = INVALID_UID;
>         fattr.cf_gid = INVALID_GID;
> @@ -1309,12 +1310,28 @@ int set_info_sec(struct ksmbd_conn *conn, struct
> ksmbd_tree_connect *tcon,
>         if (rc)
>                 goto out;
>
> -       inode->i_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
> -       if (!uid_eq(fattr.cf_uid, INVALID_UID))
> -               inode->i_uid = fattr.cf_uid;
> -       if (!gid_eq(fattr.cf_gid, INVALID_GID))
> +       newattrs.ia_valid = ATTR_CTIME;
> +       if (!uid_eq(fattr.cf_uid, INVALID_UID)) {
> +               newattrs.ia_valid |= ATTR_UID;
> +               newattrs.ia_uid = fattr.cf_uid;
> +       }
> +       if (!gid_eq(fattr.cf_gid, INVALID_GID)) {
>                 inode->i_gid = fattr.cf_gid;
>
> This needs to be removed. If setattr_prepare() fails you will still end
> up with inode->i_gid changed, i.e. you're changing group ownership even
> if you fail the subsequent setattr_prepare().
Oops, My mistake, I am missing remove this line.
>
> -       mark_inode_dirty(inode);
> +               newattrs.ia_valid |= ATTR_GID;
> +               newattrs.ia_gid = fattr.cf_gid;
> +       }
> +       newattrs.ia_valid |= ATTR_MODE;
> +       newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode &
> 0777);
> +       rc = setattr_prepare(user_ns, path->dentry, &newattrs);
> +       if (rc)
> +               goto out;
> +
> +       inode_lock(inode);
> +       setattr_copy(user_ns, inode, &newattrs);
>
> This needs to be removed. setattr_copy() will be called in the
> i_io->setattr()
> method of the underlying filesystem which notify_change() will all.
>
> Right now, with the setattr_copy() will commit the changes to the inode.
> If the following notify_change() call fails you end up with a change in
> ownership that should've failed.
Okay, I will remove it.

Thanks!
>
> Christian
>

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Christian Brauner]

On Sat, Aug 21, 2021 at 09:09:28PM +0900, Namjae Jeon wrote:
> 2021-08-21 20:39 GMT+09:00, Christian Brauner <christian.brauner@ubuntu.com>:
> > On Sat, Aug 21, 2021 at 01:11:17PM +0200, Christian Brauner wrote:
> >> On Sat, Aug 21, 2021 at 02:59:21PM +0900, Namjae Jeon wrote:
> >> > 2021-08-19 22:01 GMT+09:00, Christian Brauner
> >> > <christian.brauner@ubuntu.com>:
> >> > > On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
> >> > >> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
> >> > >> > > > 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: 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>
> >> > >> > > > ---
> >> > >> > > Hi Christian,
> >> > >> > >
> >> > >> > > > I merged David's for-next tree into cifsd-next to test this. I
> >> > >> > > > did
> >> > >> > > > only compile test this. If someone gives me a clear set of
> >> > >> > > > instructions how to test ksmbd on my local machine I can at
> >> > >> > > > least
> >> > >> > > > try to cut some time out of my week to do more reviews. (I'd
> >> > >> > > > especially like to see acl behavior with ksmbd.)
> >> > >> > >
> >> > >> > > There is "How to run ksmbd" section in patch cover letter.
> >> > >> > >
> >> > >> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
> >> > >> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> >> > >> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
> >> > >> > >
> >> > >> > > Let me know if it doesn't work well even if you try to run it
> >> > >> > > with
> >> > >> > > this step.
> >> > >> > > And We will also check whether your patch work fine.
> >> > >> > >
> >> > >> > > >
> >> > >> > > > One more thing, the tree for ksmbd was very hard to find. I had
> >> > >> > > > to
> >> > >> > > > do a lot archeology to end up
> >> > >> > at:
> >> > >> > > >
> >> > >> > > > git://git.samba.org/ksmbd.git
> >> > >> > > This is also in the patch cover letter. See "Mailing list and
> >> > >> > > repositories" section.
> >> > >> > > I think that you can use :
> >> > >> > >
> >> > >> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
> >> > >> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
> >> > >> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
> >> > >> > > series
> >> > >> > >
> >> > >> > > >
> >> > >> > > > Would be appreciated if this tree could be reflected in
> >> > >> > > > MAINTAINERS
> >> > >> > > > or somewhere else. The github repos with the broken out
> >> > >> > > > patches/module aren't really that helpful.
> >> > >> > > Okay, I will add git address of ksmbd in MAINTAINERS on next
> >> > >> > > spin.
> >> > >> > >
> >> > >> > > >
> >> > >> > > > Thanks!
> >> > >> > > > Christian
> >> > >> > > Really thanks for your review and I will apply this patch after
> >> > >> > > checking it.
> >> > >> >
> >> > >> > Thank your for the pointers.
> >> > >> >
> >> > >> > Ok, so I've been taking the time to look into cifs and ksmbd today.
> >> > >> > My
> >> > >> > mental model was wrong. There
> >> > >> > are two things to consider here:
> >> > >> >
> >> > >> > 1. server: idmapped mounts with ksmbd
> >> > >> > 2. client: idmapped mounts with cifs
> >> > >> >
> >> > >> > Your patchset adds support for 1.
> >> > >> Right.
> >> > >>
> >> > >> > Let's say I have the following ksmbd config:
> >> > >> >
> >> > >> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
> >> > >> > [global]
> >> > >> >         netbios name = SMBD
> >> > >> >         server max protocol = SMB3
> >> > >> > [test]
> >> > >> >         path = /opt
> >> > >> >         writeable = yes
> >> > >> >         comment = TEST
> >> > >> >         read only = no
> >> > >> >
> >> > >> > So /opt can be an idmapped mount and ksmb would know how to deal
> >> > >> > with
> >> > >> > that correctly, i.e. you could
> >> > >> > do:
> >> > >> >
> >> > >> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
> >> > >> >
> >> > >> > ksmbd.mountd
> >> > >> >
> >> > >> > and ksmbd would take the idmapping of /opt into account.
> >> > >> Right.
> >> > >>
> >> > >> >
> >> > >> > That however is different from 2. which is cifs itself being
> >> > >> > idmappable.
> >> > >> Right.
> >> > >>
> >> > >> > Whether or not that makes sense or is needed will need some
> >> > >> > thinking.
> >> > >> >
> >> > >> > In any case, this has consequences for xfstests and now I
> >> > >> > understand
> >> > >> > your earlier confusion. In
> >> > >> > another mail you pointed out that cifs reports that idmapped mounts
> >> > >> > are
> >> > >> > not supported. That is correct
> >> > >> > insofar as it means 2. is not supported, i.e. you can't do:
> >> > >> Right.
> >> > >>
> >> > >> >
> >> > >> > mount -t cifs -o username=foo,password=bar //server/files /mnt
> >> > >> >
> >> > >> > and then
> >> > >> >
> >> > >> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
> >> > >> >
> >> > >> > but that's also not what you want in order to test for ksmbd. What
> >> > >> > you
> >> > >> > want is to test 1.
> >> > >> Right. So we have manually tested it, not xfstests.
> >> > >>
> >> > >> >
> >> > >> > So your test setup would require you to setup an idmapped mount and
> >> > >> > have
> >> > >> > ksmbd use that which can then
> >> > >> > be mounted by a client.
> >> > >> >
> >> > >> > With your instructions I was at least able to get a ksmb instance
> >> > >> > running and be able to mount a
> >> > >> > client with -t cifs. All on the same machine, i.e. my server is
> >> > >> > localhost.
> >> > >> Okay.
> >> > >>
> >> > >> >
> >> > >> > However, I need to dig a bit into the semantics to make better
> >> > >> > assertions about what's going on.
> >> > >> Okay. And I have applied your patch to ksmbd.
> >> > >>
> >> > >> >
> >> > >> > Are unix extension supported with ksmb? Everytime I try to use
> >> > >> > "posix"
> >> > >> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I
> >> > >> > get
> >> > >> > "uid=0" and "gid=0" and "noposix".
> >> > >> > I do set "unix extensions = yes" in both the samba and ksmbd
> >> > >> > smb.conf.
> >> > >> With posix mount option, It should work. It worked well before but it
> >> > >> is
> >> > >> strange now.
> >> > >>
> >> > >> I'm not sure this is the correct fix, But could you please try to
> >> > >> mount
> >> > >> with the below change ?
> >> > >>
> >> > >> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> >> > >> index eed59bc1d913..5fd0b0ddcc57 100644
> >> > >> --- a/fs/cifs/fs_context.c
> >> > >> +++ b/fs/cifs/fs_context.c
> >> > >> @@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct
> >> > >> fs_context *fc,
> >> > >>         case Opt_unix:
> >> > >>                 if (result.negated)
> >> > >>                         ctx->linux_ext = 0;
> >> > >> -               else
> >> > >> +               else {
> >> > >> +                       ctx->linux_ext = 1;
> >> > >>                         ctx->no_linux_ext = 1;
> >> > >> +               }
> >> > >>                 break;
> >> > >>         case Opt_nocase:
> >> > >>                 ctx->nocase = 1;
> >> > >
> >> > > That stops the bleeding indeed. :)
> >> > Okay, sorry for late response.
> >> > > I think a slightly nicer fix might be:
> >> > >
> >> > > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
> >> > > index eed59bc1d913..424b8dc2232e 100644
> >> > > --- a/fs/cifs/fs_context.c
> >> > > +++ b/fs/cifs/fs_context.c
> >> > > @@ -1269,7 +1269,8 @@ static int smb3_fs_context_parse_param(struct
> >> > > fs_context *fc,
> >> > >                 if (result.negated)
> >> > >                         ctx->linux_ext = 0;
> >> > >                 else
> >> > > -                       ctx->no_linux_ext = 1;
> >> > > +                       ctx->linux_ext = 1;
> >> > > +               ctx->no_linux_ext = !ctx->linux_ext;
> >> > >                 break;
> >> > >         case Opt_nocase:
> >> > >                 ctx->nocase = 1;
> >> > >
> >> > > So with this patch applied I got unix permissions working after all.
> >> > > So
> >> > > I was able to do some more testing.
> >> > Okay.
> >> > >
> >> > > Just a few questions (independent of idmapped mounts):
> >> > >
> >> > > 1. Are the "uid=" and "gid=" mount options ignored when "username="
> >> > > is
> >> > >    specified and "posix" is specified?
> >> > >
> >> > >    It seems that "uid=" and "gid=" have are silently ignored when
> >> > >    "posix' is set. They are neither used to report file ownership
> >> > > under
> >> > >    the cifs mountpoint nor are they relevant when determining
> >> > > ownership
> >> > >    on disk?
> >> > >
> >> > >    As an example, assume I have added a user "foo" with uid 1000 to
> >> > >    ksmbd via:
> >> > >
> >> > >            ksmbd.adduser -a foo
> >> > >
> >> > >    And I mounted a share via:
> >> > >
> >> > >            mount -t cifs -o
> >> > > username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid
> >> > > //127.0.0.1/test /mnt
> >> > >
> >> > >    i) Ownership in /mnt appears posix-correct, i.e. "uid=" and "gid="
> >> > > have
> >> > >       no effect on the reported ownership.
> >> > >
> >> > >    ii) Assume I'm logged in as the root user with uid 0. If I create
> >> > >        file or directory in /mnt it will be owned by user foo, i.e.
> >> > > uid
> >> > >        1000, i.e., the "uid=1234" and "gid=1234" mount option have
> >> > > zero
> >> > >        effect on the ownership of the files?
> >> > >
> >> > > 2. Are the "uid=" and "gid=" options ignored for permission checking
> >> > >    when "posix" is specified?
> >> > >
> >> > > 3. Am I correct in assuming that there is no mount option that makes
> >> > >    chown() or chmod() have an actual effect.
> >> > That will be an answer for 1,2, 3 question. There are mount options for
> >> > this.
> >> >  1. modefromsid
> >> >  2. idsfromsid
> >> >
> >> > You can use this mount option and please check it.
> >>
> >> Thank you! This works and finally I can hit some codepaths I wasn't able
> >> to until now.
> >>
> >> > >
> >> > >    cifs seems to have support for it but the ksmbd server doesn't
> >> > > seem
> >> > >    to?
> >> > No, you need to use mount options for this as I said.
> >> > ksmbd have supported it but I found an issue related to chown and have
> >> > fixed.
> >> >
> >> > Could you please check the below git branch (pulled David'tree + ksmbd
> >> > fixes) ?
> >> >
> >> >   git clone --branch=for-christian
> >> > https://github.com/namjaejeon/smb3-kernel
> >>
> >> Thanks, I've pulled that branch.
> >
> > There's two problems in this patch. I'll post and point out here
> > quickly, if you don't mind:
> Thanks for your reiview!
> >
> > commit fd7d13c387798cbc3abd68ecc07b2c868c6d96cb
> > Author: Namjae Jeon <namjae.jeon@samsung.com>
> > Date:   Sat Aug 21 14:39:43 2021 +0900
> >
> >     ksmbd: fix permission check issue on chown and chmod
> >
> >     When commanding chmod and chown on cifs&ksmbd, ksmbd allows it without
> > file
> >     permissions check. There is code to check it in settattr_prepare.
> >     Instead of setting the inode directly, update the mode and uid/gid
> >     through notify_change.
> >
> >     Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> >     Signed-off-by: Steve French <stfrench@microsoft.com>
> >
> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> > index 0131997c2177..d329ea49fa14 100644
> > --- a/fs/ksmbd/smb2pdu.c
> > +++ b/fs/ksmbd/smb2pdu.c
> > @@ -5861,10 +5861,15 @@ int smb2_set_info(struct ksmbd_work *work)
> >                 break;
> >         case SMB2_O_INFO_SECURITY:
> >                 ksmbd_debug(SMB, "GOT SMB2_O_INFO_SECURITY\n");
> > +               if (ksmbd_override_fsids(work)) {
> > +                       rc = -ENOMEM;
> > +                       goto err_out;
> > +               }
> >                 rc = smb2_set_info_sec(fp,
> >
> > le32_to_cpu(req->AdditionalInformation),
> >                                        req->Buffer,
> >                                        le32_to_cpu(req->BufferLength));
> > +               ksmbd_revert_fsids(work);
> >                 break;
> >         default:
> >                 rc = -EOPNOTSUPP;
> > diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
> > index 20455d810523..f28af33c0bd5 100644
> > --- a/fs/ksmbd/smbacl.c
> > +++ b/fs/ksmbd/smbacl.c
> > @@ -1300,6 +1300,7 @@ int set_info_sec(struct ksmbd_conn *conn, struct
> > ksmbd_tree_connect *tcon,
> >         struct smb_fattr fattr = {{0}};
> >         struct inode *inode = d_inode(path->dentry);
> >         struct user_namespace *user_ns = mnt_user_ns(path->mnt);
> > +       struct iattr newattrs;
> >
> >         fattr.cf_uid = INVALID_UID;
> >         fattr.cf_gid = INVALID_GID;
> > @@ -1309,12 +1310,28 @@ int set_info_sec(struct ksmbd_conn *conn, struct
> > ksmbd_tree_connect *tcon,
> >         if (rc)
> >                 goto out;
> >
> > -       inode->i_mode = (inode->i_mode & ~0777) | (fattr.cf_mode & 0777);
> > -       if (!uid_eq(fattr.cf_uid, INVALID_UID))
> > -               inode->i_uid = fattr.cf_uid;
> > -       if (!gid_eq(fattr.cf_gid, INVALID_GID))
> > +       newattrs.ia_valid = ATTR_CTIME;
> > +       if (!uid_eq(fattr.cf_uid, INVALID_UID)) {
> > +               newattrs.ia_valid |= ATTR_UID;
> > +               newattrs.ia_uid = fattr.cf_uid;
> > +       }
> > +       if (!gid_eq(fattr.cf_gid, INVALID_GID)) {
> >                 inode->i_gid = fattr.cf_gid;
> >
> > This needs to be removed. If setattr_prepare() fails you will still end
> > up with inode->i_gid changed, i.e. you're changing group ownership even
> > if you fail the subsequent setattr_prepare().
> Oops, My mistake, I am missing remove this line.
> >
> > -       mark_inode_dirty(inode);
> > +               newattrs.ia_valid |= ATTR_GID;
> > +               newattrs.ia_gid = fattr.cf_gid;
> > +       }
> > +       newattrs.ia_valid |= ATTR_MODE;
> > +       newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode &
> > 0777);
> > +       rc = setattr_prepare(user_ns, path->dentry, &newattrs);

Sorry, I also forgot to mention that you need to remove this too. All of
this is handled in the underlying filesystem's ->setattr. So this just
needs a notify_change().

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Namjae Jeon]

2021-08-21 23:10 GMT+09:00, Christian Brauner <christian.brauner@ubuntu.com>:
> On Sat, Aug 21, 2021 at 09:09:28PM +0900, Namjae Jeon wrote:
>> 2021-08-21 20:39 GMT+09:00, Christian Brauner
>> <christian.brauner@ubuntu.com>:
>> > On Sat, Aug 21, 2021 at 01:11:17PM +0200, Christian Brauner wrote:
>> >> On Sat, Aug 21, 2021 at 02:59:21PM +0900, Namjae Jeon wrote:
>> >> > 2021-08-19 22:01 GMT+09:00, Christian Brauner
>> >> > <christian.brauner@ubuntu.com>:
>> >> > > On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
>> >> > >> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
>> >> > >> > > > 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: 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>
>> >> > >> > > > ---
>> >> > >> > > Hi Christian,
>> >> > >> > >
>> >> > >> > > > I merged David's for-next tree into cifsd-next to test this.
>> >> > >> > > > I
>> >> > >> > > > did
>> >> > >> > > > only compile test this. If someone gives me a clear set of
>> >> > >> > > > instructions how to test ksmbd on my local machine I can at
>> >> > >> > > > least
>> >> > >> > > > try to cut some time out of my week to do more reviews.
>> >> > >> > > > (I'd
>> >> > >> > > > especially like to see acl behavior with ksmbd.)
>> >> > >> > >
>> >> > >> > > There is "How to run ksmbd" section in patch cover letter.
>> >> > >> > >
>> >> > >> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
>> >> > >> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> >> > >> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
>> >> > >> > >
>> >> > >> > > Let me know if it doesn't work well even if you try to run it
>> >> > >> > > with
>> >> > >> > > this step.
>> >> > >> > > And We will also check whether your patch work fine.
>> >> > >> > >
>> >> > >> > > >
>> >> > >> > > > One more thing, the tree for ksmbd was very hard to find. I
>> >> > >> > > > had
>> >> > >> > > > to
>> >> > >> > > > do a lot archeology to end up
>> >> > >> > at:
>> >> > >> > > >
>> >> > >> > > > git://git.samba.org/ksmbd.git
>> >> > >> > > This is also in the patch cover letter. See "Mailing list and
>> >> > >> > > repositories" section.
>> >> > >> > > I think that you can use :
>> >> > >> > >
>> >> > >> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
>> >> > >> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> >> > >> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
>> >> > >> > > series
>> >> > >> > >
>> >> > >> > > >
>> >> > >> > > > Would be appreciated if this tree could be reflected in
>> >> > >> > > > MAINTAINERS
>> >> > >> > > > or somewhere else. The github repos with the broken out
>> >> > >> > > > patches/module aren't really that helpful.
>> >> > >> > > Okay, I will add git address of ksmbd in MAINTAINERS on next
>> >> > >> > > spin.
>> >> > >> > >
>> >> > >> > > >
>> >> > >> > > > Thanks!
>> >> > >> > > > Christian
>> >> > >> > > Really thanks for your review and I will apply this patch
>> >> > >> > > after
>> >> > >> > > checking it.
>> >> > >> >
>> >> > >> > Thank your for the pointers.
>> >> > >> >
>> >> > >> > Ok, so I've been taking the time to look into cifs and ksmbd
>> >> > >> > today.
>> >> > >> > My
>> >> > >> > mental model was wrong. There
>> >> > >> > are two things to consider here:
>> >> > >> >
>> >> > >> > 1. server: idmapped mounts with ksmbd
>> >> > >> > 2. client: idmapped mounts with cifs
>> >> > >> >
>> >> > >> > Your patchset adds support for 1.
>> >> > >> Right.
>> >> > >>
>> >> > >> > Let's say I have the following ksmbd config:
>> >> > >> >
>> >> > >> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
>> >> > >> > [global]
>> >> > >> >         netbios name = SMBD
>> >> > >> >         server max protocol = SMB3
>> >> > >> > [test]
>> >> > >> >         path = /opt
>> >> > >> >         writeable = yes
>> >> > >> >         comment = TEST
>> >> > >> >         read only = no
>> >> > >> >
>> >> > >> > So /opt can be an idmapped mount and ksmb would know how to
>> >> > >> > deal
>> >> > >> > with
>> >> > >> > that correctly, i.e. you could
>> >> > >> > do:
>> >> > >> >
>> >> > >> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
>> >> > >> >
>> >> > >> > ksmbd.mountd
>> >> > >> >
>> >> > >> > and ksmbd would take the idmapping of /opt into account.
>> >> > >> Right.
>> >> > >>
>> >> > >> >
>> >> > >> > That however is different from 2. which is cifs itself being
>> >> > >> > idmappable.
>> >> > >> Right.
>> >> > >>
>> >> > >> > Whether or not that makes sense or is needed will need some
>> >> > >> > thinking.
>> >> > >> >
>> >> > >> > In any case, this has consequences for xfstests and now I
>> >> > >> > understand
>> >> > >> > your earlier confusion. In
>> >> > >> > another mail you pointed out that cifs reports that idmapped
>> >> > >> > mounts
>> >> > >> > are
>> >> > >> > not supported. That is correct
>> >> > >> > insofar as it means 2. is not supported, i.e. you can't do:
>> >> > >> Right.
>> >> > >>
>> >> > >> >
>> >> > >> > mount -t cifs -o username=foo,password=bar //server/files /mnt
>> >> > >> >
>> >> > >> > and then
>> >> > >> >
>> >> > >> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
>> >> > >> >
>> >> > >> > but that's also not what you want in order to test for ksmbd.
>> >> > >> > What
>> >> > >> > you
>> >> > >> > want is to test 1.
>> >> > >> Right. So we have manually tested it, not xfstests.
>> >> > >>
>> >> > >> >
>> >> > >> > So your test setup would require you to setup an idmapped mount
>> >> > >> > and
>> >> > >> > have
>> >> > >> > ksmbd use that which can then
>> >> > >> > be mounted by a client.
>> >> > >> >
>> >> > >> > With your instructions I was at least able to get a ksmb
>> >> > >> > instance
>> >> > >> > running and be able to mount a
>> >> > >> > client with -t cifs. All on the same machine, i.e. my server is
>> >> > >> > localhost.
>> >> > >> Okay.
>> >> > >>
>> >> > >> >
>> >> > >> > However, I need to dig a bit into the semantics to make better
>> >> > >> > assertions about what's going on.
>> >> > >> Okay. And I have applied your patch to ksmbd.
>> >> > >>
>> >> > >> >
>> >> > >> > Are unix extension supported with ksmb? Everytime I try to use
>> >> > >> > "posix"
>> >> > >> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I
>> >> > >> > get
>> >> > >> > "uid=0" and "gid=0" and "noposix".
>> >> > >> > I do set "unix extensions = yes" in both the samba and ksmbd
>> >> > >> > smb.conf.
>> >> > >> With posix mount option, It should work. It worked well before but
>> >> > >> it
>> >> > >> is
>> >> > >> strange now.
>> >> > >>
>> >> > >> I'm not sure this is the correct fix, But could you please try to
>> >> > >> mount
>> >> > >> with the below change ?
>> >> > >>
>> >> > >> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
>> >> > >> index eed59bc1d913..5fd0b0ddcc57 100644
>> >> > >> --- a/fs/cifs/fs_context.c
>> >> > >> +++ b/fs/cifs/fs_context.c
>> >> > >> @@ -1268,8 +1268,10 @@ static int
>> >> > >> smb3_fs_context_parse_param(struct
>> >> > >> fs_context *fc,
>> >> > >>         case Opt_unix:
>> >> > >>                 if (result.negated)
>> >> > >>                         ctx->linux_ext = 0;
>> >> > >> -               else
>> >> > >> +               else {
>> >> > >> +                       ctx->linux_ext = 1;
>> >> > >>                         ctx->no_linux_ext = 1;
>> >> > >> +               }
>> >> > >>                 break;
>> >> > >>         case Opt_nocase:
>> >> > >>                 ctx->nocase = 1;
>> >> > >
>> >> > > That stops the bleeding indeed. :)
>> >> > Okay, sorry for late response.
>> >> > > I think a slightly nicer fix might be:
>> >> > >
>> >> > > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
>> >> > > index eed59bc1d913..424b8dc2232e 100644
>> >> > > --- a/fs/cifs/fs_context.c
>> >> > > +++ b/fs/cifs/fs_context.c
>> >> > > @@ -1269,7 +1269,8 @@ static int
>> >> > > smb3_fs_context_parse_param(struct
>> >> > > fs_context *fc,
>> >> > >                 if (result.negated)
>> >> > >                         ctx->linux_ext = 0;
>> >> > >                 else
>> >> > > -                       ctx->no_linux_ext = 1;
>> >> > > +                       ctx->linux_ext = 1;
>> >> > > +               ctx->no_linux_ext = !ctx->linux_ext;
>> >> > >                 break;
>> >> > >         case Opt_nocase:
>> >> > >                 ctx->nocase = 1;
>> >> > >
>> >> > > So with this patch applied I got unix permissions working after
>> >> > > all.
>> >> > > So
>> >> > > I was able to do some more testing.
>> >> > Okay.
>> >> > >
>> >> > > Just a few questions (independent of idmapped mounts):
>> >> > >
>> >> > > 1. Are the "uid=" and "gid=" mount options ignored when
>> >> > > "username="
>> >> > > is
>> >> > >    specified and "posix" is specified?
>> >> > >
>> >> > >    It seems that "uid=" and "gid=" have are silently ignored when
>> >> > >    "posix' is set. They are neither used to report file ownership
>> >> > > under
>> >> > >    the cifs mountpoint nor are they relevant when determining
>> >> > > ownership
>> >> > >    on disk?
>> >> > >
>> >> > >    As an example, assume I have added a user "foo" with uid 1000
>> >> > > to
>> >> > >    ksmbd via:
>> >> > >
>> >> > >            ksmbd.adduser -a foo
>> >> > >
>> >> > >    And I mounted a share via:
>> >> > >
>> >> > >            mount -t cifs -o
>> >> > > username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid
>> >> > > //127.0.0.1/test /mnt
>> >> > >
>> >> > >    i) Ownership in /mnt appears posix-correct, i.e. "uid=" and
>> >> > > "gid="
>> >> > > have
>> >> > >       no effect on the reported ownership.
>> >> > >
>> >> > >    ii) Assume I'm logged in as the root user with uid 0. If I
>> >> > > create
>> >> > >        file or directory in /mnt it will be owned by user foo,
>> >> > > i.e.
>> >> > > uid
>> >> > >        1000, i.e., the "uid=1234" and "gid=1234" mount option have
>> >> > > zero
>> >> > >        effect on the ownership of the files?
>> >> > >
>> >> > > 2. Are the "uid=" and "gid=" options ignored for permission
>> >> > > checking
>> >> > >    when "posix" is specified?
>> >> > >
>> >> > > 3. Am I correct in assuming that there is no mount option that
>> >> > > makes
>> >> > >    chown() or chmod() have an actual effect.
>> >> > That will be an answer for 1,2, 3 question. There are mount options
>> >> > for
>> >> > this.
>> >> >  1. modefromsid
>> >> >  2. idsfromsid
>> >> >
>> >> > You can use this mount option and please check it.
>> >>
>> >> Thank you! This works and finally I can hit some codepaths I wasn't
>> >> able
>> >> to until now.
>> >>
>> >> > >
>> >> > >    cifs seems to have support for it but the ksmbd server doesn't
>> >> > > seem
>> >> > >    to?
>> >> > No, you need to use mount options for this as I said.
>> >> > ksmbd have supported it but I found an issue related to chown and
>> >> > have
>> >> > fixed.
>> >> >
>> >> > Could you please check the below git branch (pulled David'tree +
>> >> > ksmbd
>> >> > fixes) ?
>> >> >
>> >> >   git clone --branch=for-christian
>> >> > https://github.com/namjaejeon/smb3-kernel
>> >>
>> >> Thanks, I've pulled that branch.
>> >
>> > There's two problems in this patch. I'll post and point out here
>> > quickly, if you don't mind:
>> Thanks for your reiview!
>> >
>> > commit fd7d13c387798cbc3abd68ecc07b2c868c6d96cb
>> > Author: Namjae Jeon <namjae.jeon@samsung.com>
>> > Date:   Sat Aug 21 14:39:43 2021 +0900
>> >
>> >     ksmbd: fix permission check issue on chown and chmod
>> >
>> >     When commanding chmod and chown on cifs&ksmbd, ksmbd allows it
>> > without
>> > file
>> >     permissions check. There is code to check it in settattr_prepare.
>> >     Instead of setting the inode directly, update the mode and uid/gid
>> >     through notify_change.
>> >
>> >     Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
>> >     Signed-off-by: Steve French <stfrench@microsoft.com>
>> >
>> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> > index 0131997c2177..d329ea49fa14 100644
>> > --- a/fs/ksmbd/smb2pdu.c
>> > +++ b/fs/ksmbd/smb2pdu.c
>> > @@ -5861,10 +5861,15 @@ int smb2_set_info(struct ksmbd_work *work)
>> >                 break;
>> >         case SMB2_O_INFO_SECURITY:
>> >                 ksmbd_debug(SMB, "GOT SMB2_O_INFO_SECURITY\n");
>> > +               if (ksmbd_override_fsids(work)) {
>> > +                       rc = -ENOMEM;
>> > +                       goto err_out;
>> > +               }
>> >                 rc = smb2_set_info_sec(fp,
>> >
>> > le32_to_cpu(req->AdditionalInformation),
>> >                                        req->Buffer,
>> >                                        le32_to_cpu(req->BufferLength));
>> > +               ksmbd_revert_fsids(work);
>> >                 break;
>> >         default:
>> >                 rc = -EOPNOTSUPP;
>> > diff --git a/fs/ksmbd/smbacl.c b/fs/ksmbd/smbacl.c
>> > index 20455d810523..f28af33c0bd5 100644
>> > --- a/fs/ksmbd/smbacl.c
>> > +++ b/fs/ksmbd/smbacl.c
>> > @@ -1300,6 +1300,7 @@ int set_info_sec(struct ksmbd_conn *conn, struct
>> > ksmbd_tree_connect *tcon,
>> >         struct smb_fattr fattr = {{0}};
>> >         struct inode *inode = d_inode(path->dentry);
>> >         struct user_namespace *user_ns = mnt_user_ns(path->mnt);
>> > +       struct iattr newattrs;
>> >
>> >         fattr.cf_uid = INVALID_UID;
>> >         fattr.cf_gid = INVALID_GID;
>> > @@ -1309,12 +1310,28 @@ int set_info_sec(struct ksmbd_conn *conn,
>> > struct
>> > ksmbd_tree_connect *tcon,
>> >         if (rc)
>> >                 goto out;
>> >
>> > -       inode->i_mode = (inode->i_mode & ~0777) | (fattr.cf_mode &
>> > 0777);
>> > -       if (!uid_eq(fattr.cf_uid, INVALID_UID))
>> > -               inode->i_uid = fattr.cf_uid;
>> > -       if (!gid_eq(fattr.cf_gid, INVALID_GID))
>> > +       newattrs.ia_valid = ATTR_CTIME;
>> > +       if (!uid_eq(fattr.cf_uid, INVALID_UID)) {
>> > +               newattrs.ia_valid |= ATTR_UID;
>> > +               newattrs.ia_uid = fattr.cf_uid;
>> > +       }
>> > +       if (!gid_eq(fattr.cf_gid, INVALID_GID)) {
>> >                 inode->i_gid = fattr.cf_gid;
>> >
>> > This needs to be removed. If setattr_prepare() fails you will still end
>> > up with inode->i_gid changed, i.e. you're changing group ownership even
>> > if you fail the subsequent setattr_prepare().
>> Oops, My mistake, I am missing remove this line.
>> >
>> > -       mark_inode_dirty(inode);
>> > +               newattrs.ia_valid |= ATTR_GID;
>> > +               newattrs.ia_gid = fattr.cf_gid;
>> > +       }
>> > +       newattrs.ia_valid |= ATTR_MODE;
>> > +       newattrs.ia_mode = (inode->i_mode & ~0777) | (fattr.cf_mode &
>> > 0777);
>> > +       rc = setattr_prepare(user_ns, path->dentry, &newattrs);
>
> Sorry, I also forgot to mention that you need to remove this too. All of
> this is handled in the underlying filesystem's ->setattr. So this just
> needs a notify_change().
Done:)

Thanks for your review!
>

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[Namjae Jeon]

2021-08-21 20:11 GMT+09:00, Christian Brauner <christian.brauner@ubuntu.com>:
> On Sat, Aug 21, 2021 at 02:59:21PM +0900, Namjae Jeon wrote:
>> 2021-08-19 22:01 GMT+09:00, Christian Brauner
>> <christian.brauner@ubuntu.com>:
>> > On Thu, Aug 19, 2021 at 11:19:04AM +0900, Namjae Jeon wrote:
>> >> > On Tue, Aug 17, 2021 at 08:30:55AM +0900, Namjae Jeon wrote:
>> >> > > > 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: 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>
>> >> > > > ---
>> >> > > Hi Christian,
>> >> > >
>> >> > > > I merged David's for-next tree into cifsd-next to test this. I
>> >> > > > did
>> >> > > > only compile test this. If someone gives me a clear set of
>> >> > > > instructions how to test ksmbd on my local machine I can at
>> >> > > > least
>> >> > > > try to cut some time out of my week to do more reviews. (I'd
>> >> > > > especially like to see acl behavior with ksmbd.)
>> >> > >
>> >> > > There is "How to run ksmbd" section in patch cover letter.
>> >> > >
>> >> > > https://protect2.fireeye.com/v1/url?k=65ecaaf0-3a779239-65ed21bf-0cc47
>> >> > > a336fae-53bc47005a1a97a9&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> >> > > https%3A%2F%2Flkml.org%2Flkml%2F2021%2F8%2F5%2F54
>> >> > >
>> >> > > Let me know if it doesn't work well even if you try to run it with
>> >> > > this step.
>> >> > > And We will also check whether your patch work fine.
>> >> > >
>> >> > > >
>> >> > > > One more thing, the tree for ksmbd was very hard to find. I had
>> >> > > > to
>> >> > > > do a lot archeology to end up
>> >> > at:
>> >> > > >
>> >> > > > git://git.samba.org/ksmbd.git
>> >> > > This is also in the patch cover letter. See "Mailing list and
>> >> > > repositories" section.
>> >> > > I think that you can use :
>> >> > >
>> >> > > https://protect2.fireeye.com/v1/url?k=8af83a5d-d5630294-8af9b112-0cc47
>> >> > > a336fae-e471ffbdb93d05b7&q=1&e=e44c9f9f-d7ae-4768-8cc2-8f02d748fc6e&u=
>> >> > > https%3A%2F%2Fgithub.com%2Fnamjaejeon%2Fsmb3-kernel%2Ftree%2Fksmbd-v7-
>> >> > > series
>> >> > >
>> >> > > >
>> >> > > > Would be appreciated if this tree could be reflected in
>> >> > > > MAINTAINERS
>> >> > > > or somewhere else. The github repos with the broken out
>> >> > > > patches/module aren't really that helpful.
>> >> > > Okay, I will add git address of ksmbd in MAINTAINERS on next spin.
>> >> > >
>> >> > > >
>> >> > > > Thanks!
>> >> > > > Christian
>> >> > > Really thanks for your review and I will apply this patch after
>> >> > > checking it.
>> >> >
>> >> > Thank your for the pointers.
>> >> >
>> >> > Ok, so I've been taking the time to look into cifs and ksmbd today.
>> >> > My
>> >> > mental model was wrong. There
>> >> > are two things to consider here:
>> >> >
>> >> > 1. server: idmapped mounts with ksmbd
>> >> > 2. client: idmapped mounts with cifs
>> >> >
>> >> > Your patchset adds support for 1.
>> >> Right.
>> >>
>> >> > Let's say I have the following ksmbd config:
>> >> >
>> >> > root@f2-vm:~# cat /etc/ksmbd/smb.conf
>> >> > [global]
>> >> >         netbios name = SMBD
>> >> >         server max protocol = SMB3
>> >> > [test]
>> >> >         path = /opt
>> >> >         writeable = yes
>> >> >         comment = TEST
>> >> >         read only = no
>> >> >
>> >> > So /opt can be an idmapped mount and ksmb would know how to deal
>> >> > with
>> >> > that correctly, i.e. you could
>> >> > do:
>> >> >
>> >> > mount-idmapped --map-mount=b:1000:0:1 /opt /opt
>> >> >
>> >> > ksmbd.mountd
>> >> >
>> >> > and ksmbd would take the idmapping of /opt into account.
>> >> Right.
>> >>
>> >> >
>> >> > That however is different from 2. which is cifs itself being
>> >> > idmappable.
>> >> Right.
>> >>
>> >> > Whether or not that makes sense or is needed will need some
>> >> > thinking.
>> >> >
>> >> > In any case, this has consequences for xfstests and now I understand
>> >> > your earlier confusion. In
>> >> > another mail you pointed out that cifs reports that idmapped mounts
>> >> > are
>> >> > not supported. That is correct
>> >> > insofar as it means 2. is not supported, i.e. you can't do:
>> >> Right.
>> >>
>> >> >
>> >> > mount -t cifs -o username=foo,password=bar //server/files /mnt
>> >> >
>> >> > and then
>> >> >
>> >> > mount-idmapped --map-mount=b:1000:0:1 /mnt /mnt
>> >> >
>> >> > but that's also not what you want in order to test for ksmbd. What
>> >> > you
>> >> > want is to test 1.
>> >> Right. So we have manually tested it, not xfstests.
>> >>
>> >> >
>> >> > So your test setup would require you to setup an idmapped mount and
>> >> > have
>> >> > ksmbd use that which can then
>> >> > be mounted by a client.
>> >> >
>> >> > With your instructions I was at least able to get a ksmb instance
>> >> > running and be able to mount a
>> >> > client with -t cifs. All on the same machine, i.e. my server is
>> >> > localhost.
>> >> Okay.
>> >>
>> >> >
>> >> > However, I need to dig a bit into the semantics to make better
>> >> > assertions about what's going on.
>> >> Okay. And I have applied your patch to ksmbd.
>> >>
>> >> >
>> >> > Are unix extension supported with ksmb? Everytime I try to use
>> >> > "posix"
>> >> > as a mount option with mount -t cifs -o //127.0.0.1/test /mnt I get
>> >> > "uid=0" and "gid=0" and "noposix".
>> >> > I do set "unix extensions = yes" in both the samba and ksmbd
>> >> > smb.conf.
>> >> With posix mount option, It should work. It worked well before but it
>> >> is
>> >> strange now.
>> >>
>> >> I'm not sure this is the correct fix, But could you please try to
>> >> mount
>> >> with the below change ?
>> >>
>> >> diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
>> >> index eed59bc1d913..5fd0b0ddcc57 100644
>> >> --- a/fs/cifs/fs_context.c
>> >> +++ b/fs/cifs/fs_context.c
>> >> @@ -1268,8 +1268,10 @@ static int smb3_fs_context_parse_param(struct
>> >> fs_context *fc,
>> >>         case Opt_unix:
>> >>                 if (result.negated)
>> >>                         ctx->linux_ext = 0;
>> >> -               else
>> >> +               else {
>> >> +                       ctx->linux_ext = 1;
>> >>                         ctx->no_linux_ext = 1;
>> >> +               }
>> >>                 break;
>> >>         case Opt_nocase:
>> >>                 ctx->nocase = 1;
>> >
>> > That stops the bleeding indeed. :)
>> Okay, sorry for late response.
>> > I think a slightly nicer fix might be:
>> >
>> > diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
>> > index eed59bc1d913..424b8dc2232e 100644
>> > --- a/fs/cifs/fs_context.c
>> > +++ b/fs/cifs/fs_context.c
>> > @@ -1269,7 +1269,8 @@ static int smb3_fs_context_parse_param(struct
>> > fs_context *fc,
>> >                 if (result.negated)
>> >                         ctx->linux_ext = 0;
>> >                 else
>> > -                       ctx->no_linux_ext = 1;
>> > +                       ctx->linux_ext = 1;
>> > +               ctx->no_linux_ext = !ctx->linux_ext;
>> >                 break;
>> >         case Opt_nocase:
>> >                 ctx->nocase = 1;
>> >
>> > So with this patch applied I got unix permissions working after all. So
>> > I was able to do some more testing.
>> Okay.
>> >
>> > Just a few questions (independent of idmapped mounts):
>> >
>> > 1. Are the "uid=" and "gid=" mount options ignored when "username=" is
>> >    specified and "posix" is specified?
>> >
>> >    It seems that "uid=" and "gid=" have are silently ignored when
>> >    "posix' is set. They are neither used to report file ownership under
>> >    the cifs mountpoint nor are they relevant when determining ownership
>> >    on disk?
>> >
>> >    As an example, assume I have added a user "foo" with uid 1000 to
>> >    ksmbd via:
>> >
>> >            ksmbd.adduser -a foo
>> >
>> >    And I mounted a share via:
>> >
>> >            mount -t cifs -o
>> > username=foo,password=bar,posix,uid=1234,gid=1234,forceuid,forcegid
>> > //127.0.0.1/test /mnt
>> >
>> >    i) Ownership in /mnt appears posix-correct, i.e. "uid=" and "gid="
>> > have
>> >       no effect on the reported ownership.
>> >
>> >    ii) Assume I'm logged in as the root user with uid 0. If I create
>> >        file or directory in /mnt it will be owned by user foo, i.e. uid
>> >        1000, i.e., the "uid=1234" and "gid=1234" mount option have zero
>> >        effect on the ownership of the files?
>> >
>> > 2. Are the "uid=" and "gid=" options ignored for permission checking
>> >    when "posix" is specified?
>> >
>> > 3. Am I correct in assuming that there is no mount option that makes
>> >    chown() or chmod() have an actual effect.
>> That will be an answer for 1,2, 3 question. There are mount options for
>> this.
>>  1. modefromsid
>>  2. idsfromsid
>>
>> You can use this mount option and please check it.
>
> Thank you! This works and finally I can hit some codepaths I wasn't able
> to until now.
>
>> >
>> >    cifs seems to have support for it but the ksmbd server doesn't seem
>> >    to?
>> No, you need to use mount options for this as I said.
>> ksmbd have supported it but I found an issue related to chown and have
>> fixed.
>>
>> Could you please check the below git branch (pulled David'tree + ksmbd
>> fixes) ?
>>
>>   git clone --branch=for-christian
>> https://github.com/namjaejeon/smb3-kernel
>
> Thanks, I've pulled that branch.
>
> I have a some patches for ksmb that I'll be sending out next week. I
> just need to test the changes and verify that it all makes sense.
Okay, Thanks for your work!
>
> Christian
>

Re: [PATCH] ksmbd: fix lookup on idmapped mounts

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4726 bytes --]

Hi Christian,

I love your patch! Yet something to improve:

[auto build test ERROR on 456af438ad490bac7ed954cb929bcec1df7f0c82]

url:    https://github.com/0day-ci/linux/commits/Christian-Brauner/ksmbd-fix-lookup-on-idmapped-mounts/20210816-200054
base:   456af438ad490bac7ed954cb929bcec1df7f0c82
config: s390-allyesconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ebbc41b9d3f7b7f6c558cb5d5f12ecb8134c9c2e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christian-Brauner/ksmbd-fix-lookup-on-idmapped-mounts/20210816-200054
        git checkout ebbc41b9d3f7b7f6c558cb5d5f12ecb8134c9c2e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/ksmbd/vfs.c: In function 'ksmbd_vfs_lock_parent':
>> fs/ksmbd/vfs.c:79:18: error: implicit declaration of function 'lookup_one'; did you mean 'lookup_bdev'? [-Werror=implicit-function-declaration]
      79 |         dentry = lookup_one(user_ns, child->d_name.name, parent,
         |                  ^~~~~~~~~~
         |                  lookup_bdev
   fs/ksmbd/vfs.c:79:16: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      79 |         dentry = lookup_one(user_ns, child->d_name.name, parent,
         |                ^
   fs/ksmbd/vfs.c: In function 'ksmbd_vfs_mkdir':
   fs/ksmbd/vfs.c:223:19: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     223 |                 d = lookup_one(user_ns, dentry->d_name.name, dentry->d_parent,
         |                   ^
   fs/ksmbd/vfs.c: In function '__ksmbd_vfs_rename':
   fs/ksmbd/vfs.c:753:18: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     753 |         dst_dent = lookup_one(dst_user_ns, dst_name, dst_dent_parent,
         |                  ^
   fs/ksmbd/vfs.c: In function 'ksmbd_vfs_fp_rename':
   fs/ksmbd/vfs.c:816:19: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     816 |         src_child = lookup_one(user_ns, src_dent->d_name.name, src_dent_parent,
         |                   ^
   cc1: some warnings being treated as errors
--
   fs/ksmbd/smb2pdu.c: In function 'process_query_dir_entries':
>> fs/ksmbd/smb2pdu.c:3542:24: error: implicit declaration of function 'lookup_one'; did you mean 'lookup_bdev'? [-Werror=implicit-function-declaration]
    3542 |                 dent = lookup_one(user_ns, priv->d_info->name,
         |                        ^~~~~~~~~~
         |                        lookup_bdev
   fs/ksmbd/smb2pdu.c:3542:22: warning: assignment to 'struct dentry *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    3542 |                 dent = lookup_one(user_ns, priv->d_info->name,
         |                      ^
   cc1: some warnings being treated as errors


vim +79 fs/ksmbd/vfs.c

    62	
    63	/**
    64	 * ksmbd_vfs_lock_parent() - lock parent dentry if it is stable
    65	 *
    66	 * the parent dentry got by dget_parent or @parent could be
    67	 * unstable, we try to lock a parent inode and lookup the
    68	 * child dentry again.
    69	 *
    70	 * the reference count of @parent isn't incremented.
    71	 */
    72	int ksmbd_vfs_lock_parent(struct user_namespace *user_ns, struct dentry *parent,
    73				  struct dentry *child)
    74	{
    75		struct dentry *dentry;
    76		int ret = 0;
    77	
    78		inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
  > 79		dentry = lookup_one(user_ns, child->d_name.name, parent,
    80				    child->d_name.len);
    81		if (IS_ERR(dentry)) {
    82			ret = PTR_ERR(dentry);
    83			goto out_err;
    84		}
    85	
    86		if (dentry != child) {
    87			ret = -ESTALE;
    88			dput(dentry);
    89			goto out_err;
    90		}
    91	
    92		dput(dentry);
    93		return 0;
    94	out_err:
    95		inode_unlock(d_inode(parent));
    96		return ret;
    97	}
    98	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 66437 bytes --]