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

[Namjae Jeon <linkinjeon@kernel.org>]

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
>