Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add man pages #278

Closed
wants to merge 5 commits into from
Closed

Add man pages #278

wants to merge 5 commits into from

Conversation

atheik
Copy link
Member

@atheik atheik commented Aug 28, 2022

ksmbd-tools: move verbose option to beginning of usage string
ksmbd-tools: allow adding root user in adduser
ksmbd-tools: remove global guest account fallback
ksmbd-tools: cleanup some log messages
ksmbd-tools: add man pages
ksmbd-tools: add some prints to shutdown and reload of control

Signed-off-by: atheik <atteh.mailbox@gmail.com>

@atheik atheik force-pushed the add-man-pages branch 3 times, most recently from 97176f4 to 9a840b1 Compare August 28, 2022 02:24
@atheik
Copy link
Member Author

atheik commented Aug 28, 2022

Sorry about the force-pushes. I mixed up some patches.

@atheik
Copy link
Member Author

atheik commented Aug 28, 2022

I added one more sort of related commit.

@namjaejeon
Copy link
Member

Applied, thanks for your patches!

@atheik
Copy link
Member Author

atheik commented Aug 29, 2022

@namjaejeon,

The description for the smb3 encryption parameter was this:

This parameter controls whether a remote client is allowed or required to use SMB encryption.

Now it's this:

This controls whether SMB3 encryption is required instead of offered.

Maybe I should have used "allowed" instead of "offered".

Regardless, aren't these both wrong? I can't get SMB3 encryption to work with smb3 encryption = no.

Should it be like this:
Edit: It does not determine whether it is required.

This controls whether the use of SMB3 encryption is required.
This controls whether the use of SMB3 encryption is allowed.

If so, do you mind I if add a new commit here that fixes this?

@namjaejeon
Copy link
Member

do you mind I if add a new commit here that fixes this?

No, You can push new commit, I will apply it.

@atheik
Copy link
Member Author

atheik commented Aug 29, 2022

@namjaejeon,

Ok. I'll add one more commit. Unless something comes up, that would be the last commit from me for this next release, since you had planned on releasing it soon.

Also, ksmbd-tools has this ksmbd-tools.spec file from downstream (OpenSUSE). It is multiple versions out of date. Should ksmbd-tools ship a downstream packaging script?

@atheik
Copy link
Member Author

atheik commented Aug 29, 2022

Right, so I misunderstood the original description:

This parameter controls whether a remote client is allowed or required to use SMB encryption.

I erroneously thought smb3 encryption = no means that server does not require encryption but still allows it if the client negotiates it.
I then erroneously thought smb3 encryption = yes means that encryption is required from the client.

smb3 encryption = no definitely disables support for SMB3 encryption.
smb3 encryption = yes definitely allows SMB3 encryption, but from what I read from smb2ops.c and smb2pdu.c it does not require it.
So, what does or required mean in the original description?

@namjaejeon
Copy link
Member

smb3 encryption = no definitely disables support for SMB3 encryption.
smb3 encryption = yes definitely allows SMB3 encryption, but from what I read from smb2ops.c and smb2pdu.c it does not require it.
So, what does or required mean in the original description?

Right. = no mean disable encryption support.
It was copied from smb.conf of samba. https://www.samba.org/samba/docs/current/man-html/smb.conf.5.html
So, we can change it to "It will turn on data encryption on sessions and share connections for those clients that support it."

@atheik
Copy link
Member Author

atheik commented Aug 30, 2022

@namjaejeon, what about my question regarding the .spec file? Do I leave it as is?

@namjaejeon
Copy link
Member

namjaejeon commented Aug 30, 2022

Also, ksmbd-tools has this ksmbd-tools.spec file from downstream (OpenSUSE). It is multiple versions out of date. Should ksmbd-tools ship a downstream packaging script?

There was already ksmbd-tools.spec file in ksmbd-tools.
Can we just update only version of ksmbd-tools.spec in ksmbd-tools ?

- Version:        3.4.2
+ Version:        3.4.5

@atheik
Copy link
Member Author

atheik commented Aug 30, 2022

@namjaejeon,

Also, ksmbd-tools has this ksmbd-tools.spec file from downstream (OpenSUSE). It is multiple versions out of date. Should ksmbd-tools ship a downstream packaging script?

There was already ksmbd-tools.spec file in ksmbd-tools.

I don't understand. I asked: why does ksmbd-tools ship a downstream packaging script?

Can we just update only version of ksmbd-tools.spec in ksmbd-tools ?

- Version:        3.4.2
+ Version:        3.4.5

I am not familiar with RPM, but it looks like %install, %make_install, and %files sections would have to be changed as well. Also, the Source: section tarball is bzip2 compressed (.bz2), but the tarball for the 3.4.2 release is gzip compressed (.?gz). The packaging script also invokes ./autogen.sh so the source tarball is some automatically created archive (but codeload.github.com doesn't support bzip2). So, this wasn't meant to work on the 3.4.2 release tarball.

@namjaejeon
Copy link
Member

I don't understand. I asked: why does ksmbd-tools ship a downstream packaging script?

Ah, As you see e2fsprogs(ext4 utils), It has *.spec file in source. I have thought it is needed for other distributions as well as OpenSuse. https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/e2fsprogs.spec

I am not familiar with RPM, but it looks like %install, %make_install, and %files sections would have to be changed as well. Also, the Source: section tarball is bzip2 compressed (.bz2), but the tarball for the 3.4.2 release is gzip compressed (.?gz). The packaging script also invokes ./autogen.sh so the source tarball is some autogenerated archive. So, this wasn't meant to work on the 3.4.2 release tarball.

@ematsumiya Enzo can probably answer about this.

@atheik
Copy link
Member Author

atheik commented Aug 30, 2022

@namjaejeon, @ematsumiya,

I don't understand. I asked: why does ksmbd-tools ship a downstream packaging script?

Ah, As you see e2fsprogs(ext4 utils), It has *.spec file in source. I have thought it is needed for other distributions as well as OpenSuse. https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/e2fsprogs.spec

Ok. I am just used to seeing them hosted by the distributions themselves. I was confused since this one doesn't work for the 3.4.2 release tarball.

I am not familiar with RPM, but it looks like %install, %make_install, and %files sections would have to be changed as well. Also, the Source: section tarball is bzip2 compressed (.bz2), but the tarball for the 3.4.2 release is gzip compressed (.?gz). The packaging script also invokes ./autogen.sh so the source tarball is some autogenerated archive. So, this wasn't meant to work on the 3.4.2 release tarball.

@ematsumiya Enzo can probably answer about this.

On second thought, It's probably a good idea to always run ./autogen.sh so that the spec file can be used with GitHub's automatically created archives. %config seems wrong for smb.conf.example since it's not something the user should edit with their own configuration. Also, requiring kernel 5.15 as a build requirement seems wrong. How about this:

diff --git a/ksmbd-tools.spec b/ksmbd-tools.spec
index 9ba6b49..13d0955 100644
--- a/ksmbd-tools.spec
+++ b/ksmbd-tools.spec
@@ -16,48 +16,50 @@
 #
 
 Name:           ksmbd-tools
-Version:        3.4.2
+Version:        e3fc5436af67bfb1559b85022cbcc892fe5323e0
 Release:        0
-Summary:        cifsd/ksmbd kernel server userspace utilities
+Summary:        ksmbd kernel server userspace utilities
 License:        GPL-2.0-or-later
 Group:          System/Filesystems
 Url:            https://github.com/cifsd-team/ksmbd-tools
-Source:         %{name}-%{version}.tar.bz2
+Source:         %{url}/archive/%{version}/%{name}-%{version}.tar.gz
 
-# ksmbd kernel module was only added in kernel 5.15
-BuildRequires:  kernel-default >= 5.15
 BuildRequires:  glib2-devel
 BuildRequires:  libnl3-devel
 BuildRequires:  autoconf
 BuildRequires:  automake
-BuildRequires:	libtool
+BuildRequires:  libtool
+BuildRequires:  systemd-rpm-macros
 
 Requires(pre):	kernel-default >= 5.15
+Requires(pre):	systemd >= 245
 
 %description
-Set of utilities for creating and managing SMB3 shares for the ksmbd kernel
-module.
+Collection of userspace utilities for the ksmbd kernel server.
 
 %prep
 %setup -q
 
 %build
 ./autogen.sh
-%configure
+%configure --with-systemdsystemunitdir=%{_unitdir}
 make %{?_smp_mflags}
 
 %install
-mkdir -p %{buildroot}/%{_sysconfdir}/ksmbd
-
 %make_install
-install -m 644 -p smb.conf.example %{buildroot}%{_sysconfdir}/ksmbd
 
 %files
 %{_sbindir}/ksmbd.addshare
 %{_sbindir}/ksmbd.adduser
 %{_sbindir}/ksmbd.control
 %{_sbindir}/ksmbd.mountd
-%dir %{_sysconfdir}/ksmbd
-%config %{_sysconfdir}/ksmbd/smb.conf.example
+%{_mandir}/man1/ksmbd.addshare.1*
+%{_mandir}/man1/ksmbd.adduser.1*
+%{_mandir}/man1/ksmbd.control.1*
+%{_mandir}/man1/ksmbd.mountd.1*
+%{_mandir}/man5/ksmbdpwd.db.5*
+%{_mandir}/man5/smb.conf.5ksmbd*
+%{_sysconfdir}/ksmbd/smb.conf.example
+%{_unitdir}/ksmbd.service
 
 %changelog

Build like this:

rpmbuild --undefine=_disable_source_fetch -ba ./ksmbd-tools.spec

@ematsumiya
Copy link

@namjaejeon, @ematsumiya,

I don't understand. I asked: why does ksmbd-tools ship a downstream packaging script?

Ah, As you see e2fsprogs(ext4 utils), It has *.spec file in source. I have thought it is needed for other distributions as well as OpenSuse. https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/e2fsprogs.spec

Ok. I am just used to seeing them hosted by the distributions themselves. I was confused since this one doesn't work for the 3.4.2 release tarball.

I am not familiar with RPM, but it looks like %install, %make_install, and %files sections would have to be changed as well. Also, the Source: section tarball is bzip2 compressed (.bz2), but the tarball for the 3.4.2 release is gzip compressed (.?gz). The packaging script also invokes ./autogen.sh so the source tarball is some autogenerated archive. So, this wasn't meant to work on the 3.4.2 release tarball.

@ematsumiya Enzo can probably answer about this.

On second thought, It's probably a good idea to always run ./autogen.sh so that the spec file can be used with GitHub's automatically created archives. %config seems wrong for smb.conf.example since it's not something the user should edit with their own configuration.

Please note that the spec file was created only as a template for packagers/distros, and they can (and should!) customize it to their needs.

Running './autogen.sh' is necessary for, e.g., our build system (build.opensuse.org) projects.

Also, requiring kernel 5.15 as a build requirement seems wrong. How about this:

diff --git a/ksmbd-tools.spec b/ksmbd-tools.spec
index 9ba6b49..13d0955 100644
--- a/ksmbd-tools.spec
+++ b/ksmbd-tools.spec
@@ -16,48 +16,50 @@
 #
 
 Name:           ksmbd-tools
-Version:        3.4.2
+Version:        e3fc5436af67bfb1559b85022cbcc892fe5323e0
 Release:        0
-Summary:        cifsd/ksmbd kernel server userspace utilities
+Summary:        ksmbd kernel server userspace utilities
 License:        GPL-2.0-or-later
 Group:          System/Filesystems
 Url:            https://github.com/cifsd-team/ksmbd-tools
-Source:         %{name}-%{version}.tar.bz2
+Source:         %{url}/archive/%{version}/%{name}-%{version}.tar.gz
 
-# ksmbd kernel module was only added in kernel 5.15
-BuildRequires:  kernel-default >= 5.15
 BuildRequires:  glib2-devel
 BuildRequires:  libnl3-devel
 BuildRequires:  autoconf
 BuildRequires:  automake
-BuildRequires:	libtool
+BuildRequires:  libtool
+BuildRequires:  systemd-rpm-macros
 
 Requires(pre):	kernel-default >= 5.15
+Requires(pre):	systemd >= 245
 
 %description
-Set of utilities for creating and managing SMB3 shares for the ksmbd kernel
-module.
+Collection of userspace utilities for the ksmbd kernel server.
 
 %prep
 %setup -q
 
 %build
 ./autogen.sh
-%configure
+%configure --with-systemdsystemunitdir=%{_unitdir}
 make %{?_smp_mflags}
 
 %install
-mkdir -p %{buildroot}/%{_sysconfdir}/ksmbd
-
 %make_install
-install -m 644 -p smb.conf.example %{buildroot}%{_sysconfdir}/ksmbd
 
 %files
 %{_sbindir}/ksmbd.addshare
 %{_sbindir}/ksmbd.adduser
 %{_sbindir}/ksmbd.control
 %{_sbindir}/ksmbd.mountd
-%dir %{_sysconfdir}/ksmbd
-%config %{_sysconfdir}/ksmbd/smb.conf.example
+%{_mandir}/man1/ksmbd.addshare.1*
+%{_mandir}/man1/ksmbd.adduser.1*
+%{_mandir}/man1/ksmbd.control.1*
+%{_mandir}/man1/ksmbd.mountd.1*
+%{_mandir}/man5/ksmbdpwd.db.5*
+%{_mandir}/man5/smb.conf.5ksmbd*
+%{_sysconfdir}/ksmbd/smb.conf.example
+%{_unitdir}/ksmbd.service
 
 %changelog

Build like this:

rpmbuild --undefine=_disable_source_fetch -ba ./ksmbd-tools.spec

You're right about requiring kernel 5.15 to build and the smb.conf.example place.

Thinking about this again, maybe it's a better practice to put smb.conf.example in /usr/share/doc/ksmbd or /usr/share/ksmbd, and create an empty %{_sysconfdir}/ksmbd/smb.conf with something like:

#
# This file is an empty configuration file for ksmbd
#
# Configure it to your needs, or copy the example from /usr/share/ksmbd/smb.conf.example
# 
# For more information, see "man 5ksmbd smb.conf"
# 

Other than that, looks good to me.

@atheik
Copy link
Member Author

atheik commented Aug 30, 2022

@ematsumiya, @namjaejeon,

Running './autogen.sh' is necessary for, e.g., our build system (build.opensuse.org) projects.

Right, but I'm guessing that's due to a policy that you have, since there is no need to run it when using make dist tarballs.

Thinking about this again, maybe it's a better practice to put smb.conf.example in /usr/share/doc/ksmbd or /usr/share/ksmbd, and create an empty %{_sysconfdir}/ksmbd/smb.conf with something like:

#
# This file is an empty configuration file for ksmbd
#
# Configure it to your needs, or copy the example from /usr/share/ksmbd/smb.conf.example
# 
# For more information, see "man 5ksmbd smb.conf"
# 

Maybe you are already aware, but there was a related discussion at PR #256.
The discussion there was regarding whether to install smb.conf or smb.conf.example to sysconfdir.

There was a discussion on how installing smb.conf would work without packaging (e.g. no %config to protect the user's config file). The install target would install only smb.conf if it does not exist. If it exists, it would install as smb.conf.dist. I argued that then it would make sense that the installed smb.conf or smb.conf.dist would explicitly set global parameters and all sections' share parameters to the per-release default values. Actually, the current smb.conf.example does just that and is hopefully kept up-to-date per-release in the future. If a user bases their own configuration on the installed smb.conf, then they're protected from any future ksmbd-tools release changing the implicit default value of a parameter. This is because it is highly unlikely that a user would otherwise bother to explicitly set every parameter in the config file. The user can then check smb.conf.dist and become aware of any possible changes to the implicit default parameter values in the new release.

I think installing smb.conf this way would be very useful. But, I don't think that the installed smb.conf should then have any shares predefined, e.g. it shouldn't have a share that allows read-only access to /tmp, which is what the example share is ( which used to be called tmp, and before that, misleadingly, homes). The installed smb.conf could have such shares commented out and available for the user to choose. Jeon and I disagreed about commenting out the example share and so we reached consensus that just smb.conf.example should be installed. Hey, maybe this can be (endlessly) debated about again!

Here are the build system changes that allow installing smb.conf from back then: https://github.com/cifsd-team/ksmbd-tools/compare/9be6a264ea602f475a062c8b2c0890f2d7bee1d6..3b0530bf2d6788623f97e9cb2548b40a1d46b72c.

Other than that, looks good to me.

Thanks for the review. I really would like the decision of whether to install the unit file to happen at package installation time, i.e. rpm -i should install the unit file if systemd version is v245 or later. This would then be identical to the way its done without packaging. Do you happen to know of a good way to do this with RPM?

@namjaejeon
Copy link
Member

namjaejeon commented Sep 2, 2022

@atheik Are you okay to change smb.conf to ksmbd.conf ? we need to consider the upgrade issue of ksmbd-tools. So trying to open ksmbd.conf and if open failed, trying to open smb.conf again. need to add the "OLD" prefix for smb.conf path macro name...

@atheik
Copy link
Member Author

atheik commented Sep 2, 2022

@namjaejeon,

@atheik Are you okay to change smb.conf to ksmbd.conf ?

I don't have anything against it. But, I can't say I understand the problem or that I would like to contribute that change.

we need to consider the upgrade issue of ksmbd-tools. So trying to open ksmbd.conf and if open failed, trying to open smb.conf again. need to add the "OLD" prefix for smb.conf path macro name...

So, when would /etc/ksmbd/smb.conf actually stop working for the user? If the change to ksmbd.conf should be made, then I think it would be better to kick the users off the old name immediately.

If ksmbd-tools considers every smb.conf difference between it and Samba a bug, meaning that every parameter adopted from Samba should elicit the same behavior in ksmbd-tools, then how is it a problem that ksmbd-tools keeps using the smb.conf name? Isn't it a goal ksmbd-tools to have compatibility with Samba regarding what behavior smb.conf actually elicits?

@namjaejeon
Copy link
Member

@atheik Okay, Agreed. Let's do as you say... After this release, some people will complain on ISSUE about it, but let's try...

@atheik
Copy link
Member Author

atheik commented Sep 2, 2022

@namjaejeon,

@atheik Okay, Agreed. Let's do as you say... After this release, some people will complain on ISSUE about it, but let's try...

I meant if the change to ksmbd.conf should be made. Or, have you made the decision already? I don't think it's good that ksmbd-tools would have to break for its users like this. I think it can be well argued that ksmbd-tools should keep using the smb.conf name, as it always has, if the following statement is true for the ksmbd-tools project:

ksmbd-tools considers it a bug if a parameter adopted from Samba elicits different behavior in ksmbd-tools than it does in Samba. Meaning that ksmbd-tools should always change its behavior to be in line with Samba's when it comes to the parameters ksmbd-tools adopted from Samba. And, whenever Samba makes a change to some parameter that ksmbd-tools has adopted, ksmbd-tools considers its different behavior to Samba a bug that is to be fixed.

Is the above statement true for the ksmbd-tools project?

@namjaejeon
Copy link
Member

namjaejeon commented Sep 2, 2022

I meant if the change to ksmbd.conf should be made. Or, have you made the decision already? I don't think it's good that ksmbd-tools would have to break for its users like this. I think it can be well argued that ksmbd-tools should keep using the smb.conf name, as it always has, if the following statement is true for the ksmbd-tools project:

I told you that it is good chance to change name to ksmbd.conf before. and we can use fallback to old config name, i.e. "smb.conf" to avoid the upgrade break for old user. And we can print warning message(to guide to switch ksmbd.conf) if user is still using "smb.conf" name. and we can remove old name fallback codes on next-next release. Any thought ?

Is the above statement true for the ksmbd-tools project?

If we use smb.conf name, it is true. but if we switch it to ksmb.conf, we don't care some changes of smb.conf of samba.
There is no possibility that Samba will change parameters in smb.conf. Because it will cause more confusion for existing samba users as well as ksmbd.

@namjaejeon
Copy link
Member

@atheik In fact, I am thinking it's not bad to completely separate configuration from samba, and JRA agreed to that, and now seems to be the timing.

@atheik
Copy link
Member Author

atheik commented Sep 2, 2022

@namjaejeon,

I meant if the change to ksmbd.conf should be made. Or, have you made the decision already? I don't think it's good that ksmbd-tools would have to break for its users like this. I think it can be well argued that ksmbd-tools should keep using the smb.conf name, as it always has, if the following statement is true for the ksmbd-tools project:

I told you that it is good chance to change name to ksmbd.conf before.

I wasn't aware that you were in favor of it.

and we can use fallback to old config name, i.e. "smb.conf" to avoid the upgrade break for old user. And we can print warning message(to guide to switch ksmbd.conf) if user is still using "smb.conf" name. and we can remove old name fallback codes on next-next release. Any thought ?

The difference is whether the issue tracker will have complaints for just the next release or both the next release and the next-next release. Many users will (undeservingly) expect an effortless upgrade that does not require their attention. By spreading out these types of changes over multiple releases, you may start testing users' goodwill. The next release will require existing users' attention anyways (sysconfdir and runstatedir). You decide, as always.

Is the above statement true for the ksmbd-tools project?

If we use smb.conf name, it is true. but if we switch it to ksmb.conf, we don't care some changes of smb.conf of samba. There is no possibility that Samba will change parameters in smb.conf. Because it will cause more confusion for existing samba users as well as ksmbd.

If a user is in both read list and write list, ksmbd-tools will not allow write access. Samba, according to smb.conf(5), allows write access. After the change to ksmbd.conf, which one is correct? Samba, right? Why would ksmbd-tools ever break compatibility with those parameters (all but 7 of them) it adopted from Samba?

@atheik In fact, I am thinking it's not bad to completely separate configuration from samba, and JRA agreed to that, and now seems to be the timing.

Right, I could not deduce your position from our previous conversation. You have decided that smb.conf should be renamed to ksmbd.conf. That's fine, it is your decision to make. I can make the changes, but I don't know if adding the fallback code for smb.conf is a good idea. I guess it doesn't make a difference if I add the changes to this PR.

@atheik
Copy link
Member Author

atheik commented Sep 3, 2022

Commit ec2e234 was UB since string interning isn't required to be done as an optimization. I guess it's going to be a string comparison instead which means --config /etc/ksmbd/ksmbd.conf also triggers the fallback code. That's not really a problem though.

@namjaejeon
Copy link
Member

If a user is in both read list and write list, ksmbd-tools will not allow write access. Samba, according to smb.conf(5), allows write access. After the change to ksmbd.conf, which one is correct? Samba, right?

It is a bug:), I didn't check such combination before. but if we change name to ksmbd.conf, I think that it is not a bug. i.e. If the compatibility difference is not reasonable and it's for only samba, There's no reason to follow it.

Why would ksmbd-tools ever break compatibility with those parameters (all but 7 of them) it adopted from Samba?

This is because some parameters of samba are not suitable for ksmbd. There are differences because ksmbd can do things that Samba can't, and there are cases where I don't know how to properly implement it in the current ksmbd architecture. I need to consider more, but as you know, there are many issues in ksmbd kernel as well as ksmbd-tools. I really appreciate that a great developer like you is involved in the development of ksmbd.

Right, I could not deduce your position from our previous conversation.

At first I thought the same as you, but the more I thought about it, the more I realized it wasn't necessarily a bad thing. And I can't say it publicly, but there are other reasons as well.
Thanks for your PR, I will check it. BTW, I can see some conflict with this PR.

@namjaejeon
Copy link
Member

@atheik Okay, I will apply this.

@atheik

This comment was marked as outdated.

@atheik

This comment was marked as outdated.

@atheik
Copy link
Member Author

atheik commented Sep 15, 2022

@namjaejeon,

Regarding __caseless_lookup(), could you please take a look at this when you have the time?

[PATCH] ksmbd: make utf-8 file name comparison work in __caseless_lookup() (click the arrow to expand)
From 3c51ec2e899efa824583e796c3d9f8c7101b2c55 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Thu, 15 Sep 2022 14:06:58 +0300
Subject: [PATCH] ksmbd: make utf-8 file name comparison work in
 __caseless_lookup()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
 vfs.c | 22 ++++++++++++++++++----
 vfs.h |  1 +
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/vfs.c b/vfs.c
index fb2d763..5a62728 100644
--- a/vfs.c
+++ b/vfs.c
@@ -2023,6 +2023,7 @@ static int __caseless_lookup(struct dir_context *ctx, const char *name,
 			     unsigned int d_type)
 {
 	struct ksmbd_readdir_data *buf;
+	bool is_match;
 
 	buf = container_of(ctx, struct ksmbd_readdir_data, ctx);
 
@@ -2032,7 +2033,16 @@ static int __caseless_lookup(struct dir_context *ctx, const char *name,
 #else
 		return 0;
 #endif
-	if (!strncasecmp((char *)buf->private, name, namlen)) {
+	if (IS_ENABLED(CONFIG_UNICODE) && buf->um) {
+		const struct qstr q_buf = {.name = buf->private,
+					   .len = buf->used};
+		const struct qstr q_name = {.name = name,
+					    .len = namlen};
+
+		is_match = !utf8_strncasecmp(buf->um, &q_buf, &q_name);
+	} else
+		is_match = !strncasecmp((char *)buf->private, name, namlen);
+	if (is_match) {
 		memcpy((char *)buf->private, name, namlen);
 		buf->dirent_count = 1;
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 1, 0)
@@ -2056,7 +2066,8 @@ static int __caseless_lookup(struct dir_context *ctx, const char *name,
  *
  * Return:	0 on success, otherwise error
  */
-static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name, size_t namelen)
+static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name,
+				   size_t namelen, struct unicode_map *um)
 {
 	int ret;
 	struct file *dfilp;
@@ -2066,6 +2077,7 @@ static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name, size_t na
 		.private	= name,
 		.used		= namelen,
 		.dirent_count	= 0,
+		.um		= um,
 	};
 
 	dfilp = dentry_open(dir, flags, current_cred());
@@ -2129,7 +2141,8 @@ int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
 				break;
 
 			err = ksmbd_vfs_lookup_in_dir(&parent, filename,
-						      filename_len);
+						      filename_len,
+						      work->conn->um);
 			path_put(&parent);
 			if (err)
 				goto out;
@@ -2204,7 +2217,8 @@ int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
 				break;
 
 			err = ksmbd_vfs_lookup_in_dir(&parent, filename,
-						      filename_len);
+						      filename_len,
+						      work->conn->um);
 			if (err) {
 				path_put(&parent);
 				goto out;
diff --git a/vfs.h b/vfs.h
index dc2aec4..29c1d76 100644
--- a/vfs.h
+++ b/vfs.h
@@ -102,6 +102,7 @@ struct ksmbd_readdir_data {
 	unsigned int		used;
 	unsigned int		dirent_count;
 	unsigned int		file_attr;
+	struct unicode_map	*um;
 };
 
 /* ksmbd kstat wrapper to get valid create time when reading dir entry */
-- 
2.37.3

@namjaejeon
Copy link
Member

@atheik

[PATCH] ksmbd-tools: document utf-8 support in user and share names (click the arrow to expand)

Applied, Thanks!

@namjaejeon
Copy link
Member

@atheik

Regarding __caseless_lookup(), could you please take a look at this when you have the time?

Sure, I will check it tomorrow.

@namjaejeon
Copy link
Member

namjaejeon commented Sep 15, 2022

@atheik

Regarding __caseless_lookup(), could you please take a look at this when you have the time?

Looks good except the below warning.

CHECK: Unbalanced braces around else statement
#40: FILE: vfs.c:2043:
+	} else

@atheik
Copy link
Member Author

atheik commented Sep 16, 2022

@namjaejeon,

Here's one more documentation patch.

[PATCH] ksmbd-tools: document addshare not retaining ksmbd.conf formatting (click the arrow to expand)
From 9a8fb86fde8a58dfb8e00cea6f06b0ee7e4e4211 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Sat, 17 Sep 2022 02:26:44 +0300
Subject: [PATCH] ksmbd-tools: document addshare not retaining ksmbd.conf
 formatting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When addshare modifies ksmbd.conf, it does not retain existing
formatting, which is unexpected for the user. Document this in the man
pages. Also, make a few minor changes to wording.

Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
 addshare/ksmbd.addshare.1.in | 1 +
 ksmbd.conf.5.in              | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/addshare/ksmbd.addshare.1.in b/addshare/ksmbd.addshare.1.in
index 47dc0d7..d924418 100644
--- a/addshare/ksmbd.addshare.1.in
+++ b/addshare/ksmbd.addshare.1.in
@@ -12,6 +12,7 @@ ksmbd.addshare \- configure shares for ksmbd.conf of ksmbd.mountd
 {-V | -h}
 .SH DESCRIPTION
 \fBksmbd.addshare\fR configures shares for \fBksmbd.conf\fP(5) configuration file of \fBksmbd.mountd\fP(1) user mode daemon.
+\fBksmbd.addshare\fR modifies \fBksmbd.conf\fR such that its existing formatting is not retained.
 \fBksmbd.addshare\fR notifies \fBksmbd.mountd\fR of changes if it had made any.
 .SH OPTIONS
 .TP
diff --git a/ksmbd.conf.5.in b/ksmbd.conf.5.in
index b58cd61..e77d57b 100644
--- a/ksmbd.conf.5.in
+++ b/ksmbd.conf.5.in
@@ -4,6 +4,7 @@ ksmbd.conf \- the configuration file for ksmbd.mountd
 .SH DESCRIPTION
 \fBksmbd.conf\fR is the configuration file for \fBksmbd.mountd\fP(1) user mode daemon.
 \fBksmbd.addshare\fP(1) may be used for configuring shares for \fBksmbd.conf\fR.
+\fBksmbd.addshare\fR modifies \fBksmbd.conf\fR such that its existing formatting is not retained.
 \fBksmbd.addshare\fR notifies \fBksmbd.mountd\fR of changes, if it had made any, by sending the \fBSIGHUP\fR signal to \fBksmbd.mountd\fR.
 \fBksmbd.control --reload\fR can be used for notifying \fBksmbd.mountd\fR of changes when not using \fBksmbd.addshare\fR.
 \fBksmbd.conf\fR is expected to be at \fB@sysconfdir@/ksmbd/ksmbd.conf\fR by default. \" PATH_SMBCONF
@@ -11,11 +12,11 @@ A configuration file that may serve as an example can be found at \fB@sysconfdir
 .SH "FILE FORMAT"
 \fBksmbd.conf\fR consists of sections with each new section marking the end of the previous one.
 A new section begins with the section name enclosed in brackets (\fB[]\fR) followed by a newline.
-Each section may have parameter entries separated by newlines.
+Each section may contain parameter entries separated by newlines.
 A parameter entry consists of a parameter and a value, in that order, separated by an equal sign (\fB=\fR).
 The parameter may contain leading and trailing tabs and spaces.
 The value, which begins immediately after the equal sign, may contain leading tabs and spaces or be empty.
-Some parameter entries can be given a list of multiple values, in which case they are separated by commas, tabs, or spaces.
+Some parameter entries can be given a list of multiple values, in which case the values are separated by commas, tabs, or spaces.
 A semicolon (\fB;\fR) or a hash (\fB#\fR) marks the beginning of a comment which continues until the end of the line.
 .SH SHARES
 Each section name, except that of the \fBglobal\fR section, defines a shared resource, commonly referred to as a share.
-- 
2.37.3

@namjaejeon
Copy link
Member

Here's one more documentation patch.

Applied, Thanks!

@namjaejeon
Copy link
Member

Regarding __caseless_lookup(), could you please take a look at this when you have the time?

Will you send this patch to the mailing list ?

@atheik
Copy link
Member Author

atheik commented Sep 19, 2022

@namjaejeon,

Regarding __caseless_lookup(), could you please take a look at this when you have the time?

Will you send this patch to the mailing list ?

Sure. Also, looks like at least the mountd man page has to be moved to section 8, i.e. ksmbd.mountd(8). The same probably applies for addshare, adduser, and control. I'll do both today. Please don't do a release before I've moved the man pages.

@namjaejeon
Copy link
Member

@atheik Okay:)

@atheik
Copy link
Member Author

atheik commented Sep 19, 2022

@namjaejeon,

[PATCH v2] ksmbd-tools: move utility man pages to section 8 (click the arrow to expand)
From c759a7fdc47ec2c2d9fd5293ba4f157eb6a60d7c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Thu, 22 Sep 2022 14:04:56 +0300
Subject: [PATCH v2] ksmbd-tools: move utility man pages to section 8
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Section 8 is for administration and privileged commands, which includes
daemons, that either can be or are only used by root. Move the utility
man pages there. Also, do another minor wording fix to ksmbd.conf(5).

Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
 README.md                                            |  8 ++++----
 addshare/Makefile.am                                 |  4 ++--
 addshare/addshare.c                                  |  2 +-
 .../{ksmbd.addshare.1.in => ksmbd.addshare.8.in}     | 10 +++++-----
 addshare/meson.build                                 |  6 +++---
 adduser/Makefile.am                                  |  4 ++--
 adduser/adduser.c                                    |  2 +-
 adduser/{ksmbd.adduser.1.in => ksmbd.adduser.8.in}   | 10 +++++-----
 adduser/meson.build                                  |  6 +++---
 control/Makefile.am                                  |  4 ++--
 control/control.c                                    |  2 +-
 control/{ksmbd.control.1.in => ksmbd.control.8.in}   | 10 +++++-----
 control/meson.build                                  |  6 +++---
 ksmbd-tools.spec                                     |  8 ++++----
 ksmbd.conf.5.in                                      | 12 ++++++------
 ksmbdpwd.db.5.in                                     |  8 ++++----
 mountd/Makefile.am                                   |  4 ++--
 mountd/{ksmbd.mountd.1.in => ksmbd.mountd.8.in}      |  8 ++++----
 mountd/meson.build                                   |  6 +++---
 mountd/mountd.c                                      |  2 +-
 20 files changed, 61 insertions(+), 61 deletions(-)
 rename addshare/{ksmbd.addshare.1.in => ksmbd.addshare.8.in} (91%)
 rename adduser/{ksmbd.adduser.1.in => ksmbd.adduser.8.in} (92%)
 rename control/{ksmbd.control.1.in => ksmbd.control.8.in} (86%)
 rename mountd/{ksmbd.mountd.1.in => ksmbd.mountd.8.in} (93%)

diff --git a/README.md b/README.md
index 0e76b80..384d0da 100644
--- a/README.md
+++ b/README.md
@@ -82,10 +82,10 @@ respectively.
 
 Manual pages:
 ```sh
-man 1 ksmbd.addshare
-man 1 ksmbd.adduser
-man 1 ksmbd.control
-man 1 ksmbd.mountd
+man 8 ksmbd.addshare
+man 8 ksmbd.adduser
+man 8 ksmbd.control
+man 8 ksmbd.mountd
 man 5 ksmbd.conf
 man 5 ksmbdpwd.db
 ```
diff --git a/addshare/Makefile.am b/addshare/Makefile.am
index 60803d3..30ee948 100644
--- a/addshare/Makefile.am
+++ b/addshare/Makefile.am
@@ -7,9 +7,9 @@ sbin_PROGRAMS = ksmbd.addshare
 
 ksmbd_addshare_SOURCES = share_admin.c addshare.c share_admin.h
 
-EXTRA_DIST = ksmbd.addshare.1.in
+EXTRA_DIST = ksmbd.addshare.8.in
 
-man_MANS = ksmbd.addshare.1
+man_MANS = ksmbd.addshare.8
 $(man_MANS): %: %.in; @$(in_script) $< >$@
 
 CLEANFILES = $(man_MANS)
diff --git a/addshare/addshare.c b/addshare/addshare.c
index ec57596..e91fe5c 100644
--- a/addshare/addshare.c
+++ b/addshare/addshare.c
@@ -55,7 +55,7 @@ static void usage(int status)
 			"  -V, --version               output version information and exit\n"
 			"  -h, --help                  display this help and exit\n"
 			"\n"
-			"See ksmbd.addshare(1) for more details.\n");
+			"See ksmbd.addshare(8) for more details.\n");
 }
 
 static const struct option opts[] = {
diff --git a/addshare/ksmbd.addshare.1.in b/addshare/ksmbd.addshare.8.in
similarity index 91%
rename from addshare/ksmbd.addshare.1.in
rename to addshare/ksmbd.addshare.8.in
index d924418..0d32e44 100644
--- a/addshare/ksmbd.addshare.1.in
+++ b/addshare/ksmbd.addshare.8.in
@@ -1,4 +1,4 @@
-.TH KSMBD.ADDSHARE "1" "" "ksmbd-tools @ksmbd_tools_version@" "User Commands"
+.TH KSMBD.ADDSHARE "8" "" "ksmbd-tools @ksmbd_tools_version@" "System Administration"
 .SH NAME
 ksmbd.addshare \- configure shares for ksmbd.conf of ksmbd.mountd
 .SH SYNOPSIS
@@ -11,7 +11,7 @@ ksmbd.addshare \- configure shares for ksmbd.conf of ksmbd.mountd
 .B ksmbd.addshare
 {-V | -h}
 .SH DESCRIPTION
-\fBksmbd.addshare\fR configures shares for \fBksmbd.conf\fP(5) configuration file of \fBksmbd.mountd\fP(1) user mode daemon.
+\fBksmbd.addshare\fR configures shares for \fBksmbd.conf\fP(5) configuration file of \fBksmbd.mountd\fP(8) user mode daemon.
 \fBksmbd.addshare\fR modifies \fBksmbd.conf\fR such that its existing formatting is not retained.
 \fBksmbd.addshare\fR notifies \fBksmbd.mountd\fR of changes if it had made any.
 .SH OPTIONS
@@ -64,6 +64,6 @@ For bug reports, use the issue tracker at https://github.com/cifsd-team/ksmbd-to
 \fBksmbd.conf\fP(5)
 .TP
 \fBUtilities\fR
-\fBksmbd.adduser\fP(1),
-\fBksmbd.control\fP(1),
-\fBksmbd.mountd\fP(1)
+\fBksmbd.adduser\fP(8),
+\fBksmbd.control\fP(8),
+\fBksmbd.mountd\fP(8)
diff --git a/addshare/meson.build b/addshare/meson.build
index bbde90f..c68f80d 100644
--- a/addshare/meson.build
+++ b/addshare/meson.build
@@ -18,8 +18,8 @@ addshare = executable(
 )
 
 configure_file(
-  input: 'ksmbd.addshare.1.in',
-  output: 'ksmbd.addshare.1',
-  install_dir: get_option('mandir') / 'man1',
+  input: 'ksmbd.addshare.8.in',
+  output: 'ksmbd.addshare.8',
+  install_dir: get_option('mandir') / 'man8',
   configuration: in_data,
 )
diff --git a/adduser/Makefile.am b/adduser/Makefile.am
index 1d72df9..14d30d2 100644
--- a/adduser/Makefile.am
+++ b/adduser/Makefile.am
@@ -8,9 +8,9 @@ sbin_PROGRAMS = ksmbd.adduser
 ksmbd_adduser_SOURCES = md4_hash.c user_admin.c adduser.c md4_hash.h \
                         user_admin.h
 
-EXTRA_DIST = ksmbd.adduser.1.in
+EXTRA_DIST = ksmbd.adduser.8.in
 
-man_MANS = ksmbd.adduser.1
+man_MANS = ksmbd.adduser.8
 $(man_MANS): %: %.in; @$(in_script) $< >$@
 
 CLEANFILES = $(man_MANS)
diff --git a/adduser/adduser.c b/adduser/adduser.c
index 626b3db..47c7390 100644
--- a/adduser/adduser.c
+++ b/adduser/adduser.c
@@ -54,7 +54,7 @@ static void usage(int status)
 			"  -V, --version               output version information and exit\n"
 			"  -h, --help                  display this help and exit\n"
 			"\n"
-			"See ksmbd.adduser(1) for more details.\n");
+			"See ksmbd.adduser(8) for more details.\n");
 }
 
 static const struct option opts[] = {
diff --git a/adduser/ksmbd.adduser.1.in b/adduser/ksmbd.adduser.8.in
similarity index 92%
rename from adduser/ksmbd.adduser.1.in
rename to adduser/ksmbd.adduser.8.in
index 3d4a2c6..b1429ff 100644
--- a/adduser/ksmbd.adduser.1.in
+++ b/adduser/ksmbd.adduser.8.in
@@ -1,4 +1,4 @@
-.TH KSMBD.ADDUSER "1" "" "ksmbd-tools @ksmbd_tools_version@" "User Commands"
+.TH KSMBD.ADDUSER "8" "" "ksmbd-tools @ksmbd_tools_version@" "System Administration"
 .SH NAME
 ksmbd.adduser \- configure users for ksmbdpwd.db of ksmbd.mountd
 .SH SYNOPSIS
@@ -11,7 +11,7 @@ ksmbd.adduser \- configure users for ksmbdpwd.db of ksmbd.mountd
 .B ksmbd.adduser
 {-V | -h}
 .SH DESCRIPTION
-\fBksmbd.adduser\fR configures users for \fBksmbdpwd.db\fP(5) user database of \fBksmbd.mountd\fP(1) user mode daemon.
+\fBksmbd.adduser\fR configures users for \fBksmbdpwd.db\fP(5) user database of \fBksmbd.mountd\fP(8) user mode daemon.
 \fBksmbd.adduser\fR can parse \fBksmbd.conf\fP(5) configuration file so as to guard against deletion of users that are depended on.
 \fBksmbd.adduser\fR notifies \fBksmbd.mountd\fR of changes if it had made any.
 .SH OPTIONS
@@ -67,6 +67,6 @@ For bug reports, use the issue tracker at https://github.com/cifsd-team/ksmbd-to
 \fBksmbd.conf\fP(5)
 .TP
 \fBUtilities\fR
-\fBksmbd.addshare\fP(1),
-\fBksmbd.control\fP(1),
-\fBksmbd.mountd\fP(1)
+\fBksmbd.addshare\fP(8),
+\fBksmbd.control\fP(8),
+\fBksmbd.mountd\fP(8)
diff --git a/adduser/meson.build b/adduser/meson.build
index 1c80332..ce6c235 100644
--- a/adduser/meson.build
+++ b/adduser/meson.build
@@ -20,8 +20,8 @@ adduser = executable(
 )
 
 configure_file(
-  input: 'ksmbd.adduser.1.in',
-  output: 'ksmbd.adduser.1',
-  install_dir: get_option('mandir') / 'man1',
+  input: 'ksmbd.adduser.8.in',
+  output: 'ksmbd.adduser.8',
+  install_dir: get_option('mandir') / 'man8',
   configuration: in_data,
 )
diff --git a/control/Makefile.am b/control/Makefile.am
index e4195e8..4876b6d 100644
--- a/control/Makefile.am
+++ b/control/Makefile.am
@@ -7,9 +7,9 @@ sbin_PROGRAMS = ksmbd.control
 
 ksmbd_control_SOURCES = control.c
 
-EXTRA_DIST = ksmbd.control.1.in
+EXTRA_DIST = ksmbd.control.8.in
 
-man_MANS = ksmbd.control.1
+man_MANS = ksmbd.control.8
 $(man_MANS): %: %.in; @$(in_script) $< >$@
 
 CLEANFILES = $(man_MANS)
diff --git a/control/control.c b/control/control.c
index 5656f0a..3cc51a1 100644
--- a/control/control.c
+++ b/control/control.c
@@ -34,7 +34,7 @@ static void usage(int status)
 			"  -V, --version            output version information and exit\n"
 			"  -h, --help               display this help and exit\n"
 			"\n"
-			"See ksmbd.control(1) for more details.\n");
+			"See ksmbd.control(8) for more details.\n");
 }
 
 static const struct option opts[] = {
diff --git a/control/ksmbd.control.1.in b/control/ksmbd.control.8.in
similarity index 86%
rename from control/ksmbd.control.1.in
rename to control/ksmbd.control.8.in
index 3b955f6..31a0a2d 100644
--- a/control/ksmbd.control.1.in
+++ b/control/ksmbd.control.8.in
@@ -1,4 +1,4 @@
-.TH KSMBD.CONTROL "1" "" "ksmbd-tools @ksmbd_tools_version@" "User Commands"
+.TH KSMBD.CONTROL "8" "" "ksmbd-tools @ksmbd_tools_version@" "System Administration"
 .SH NAME
 ksmbd.control \- control ksmbd.mountd and ksmbd daemons
 .SH SYNOPSIS
@@ -8,7 +8,7 @@ ksmbd.control \- control ksmbd.mountd and ksmbd daemons
 .B ksmbd.control
 {-V | -h}
 .SH DESCRIPTION
-\fBksmbd.control\fR controls \fBksmbd.mountd\fP(1) user mode and \fBksmbd\fR kernel mode daemons.
+\fBksmbd.control\fR controls \fBksmbd.mountd\fP(8) user mode and \fBksmbd\fR kernel mode daemons.
 .SH OPTIONS
 .TP
 \fB\-s\fR, \fB\-\-shutdown\fR
@@ -46,6 +46,6 @@ For bug reports, use the issue tracker at https://github.com/cifsd-team/ksmbd-to
 .SH "SEE ALSO"
 .TP
 \fBUtilities\fR
-\fBksmbd.addshare\fP(1),
-\fBksmbd.adduser\fP(1),
-\fBksmbd.mountd\fP(1)
+\fBksmbd.addshare\fP(8),
+\fBksmbd.adduser\fP(8),
+\fBksmbd.mountd\fP(8)
diff --git a/control/meson.build b/control/meson.build
index 8a118d1..ff83bee 100644
--- a/control/meson.build
+++ b/control/meson.build
@@ -16,8 +16,8 @@ control = executable(
 )
 
 configure_file(
-  input: 'ksmbd.control.1.in',
-  output: 'ksmbd.control.1',
-  install_dir: get_option('mandir') / 'man1',
+  input: 'ksmbd.control.8.in',
+  output: 'ksmbd.control.8',
+  install_dir: get_option('mandir') / 'man8',
   configuration: in_data,
 )
diff --git a/ksmbd-tools.spec b/ksmbd-tools.spec
index 0eef4df..392665a 100644
--- a/ksmbd-tools.spec
+++ b/ksmbd-tools.spec
@@ -53,10 +53,10 @@ make %{?_smp_mflags}
 %{_sbindir}/ksmbd.adduser
 %{_sbindir}/ksmbd.control
 %{_sbindir}/ksmbd.mountd
-%{_mandir}/man1/ksmbd.addshare.1*
-%{_mandir}/man1/ksmbd.adduser.1*
-%{_mandir}/man1/ksmbd.control.1*
-%{_mandir}/man1/ksmbd.mountd.1*
+%{_mandir}/man8/ksmbd.addshare.8*
+%{_mandir}/man8/ksmbd.adduser.8*
+%{_mandir}/man8/ksmbd.control.8*
+%{_mandir}/man8/ksmbd.mountd.8*
 %{_mandir}/man5/ksmbd.conf.5*
 %{_mandir}/man5/ksmbdpwd.db.5*
 %{_sysconfdir}/ksmbd/ksmbd.conf.example
diff --git a/ksmbd.conf.5.in b/ksmbd.conf.5.in
index b049b0f..8c0a33f 100644
--- a/ksmbd.conf.5.in
+++ b/ksmbd.conf.5.in
@@ -2,8 +2,8 @@
 .SH NAME
 ksmbd.conf \- the configuration file for ksmbd.mountd
 .SH DESCRIPTION
-\fBksmbd.conf\fR is the configuration file for \fBksmbd.mountd\fP(1) user mode daemon.
-\fBksmbd.addshare\fP(1) may be used for configuring shares for \fBksmbd.conf\fR.
+\fBksmbd.conf\fR is the configuration file for \fBksmbd.mountd\fP(8) user mode daemon.
+\fBksmbd.addshare\fP(8) may be used for configuring shares for \fBksmbd.conf\fR.
 \fBksmbd.addshare\fR modifies \fBksmbd.conf\fR such that its existing formatting is not retained.
 \fBksmbd.addshare\fR notifies \fBksmbd.mountd\fR of changes, if it had made any, by sending the \fBSIGHUP\fR signal to \fBksmbd.mountd\fR.
 \fBksmbd.control --reload\fR can be used for notifying \fBksmbd.mountd\fR of changes when not using \fBksmbd.addshare\fR.
@@ -17,7 +17,7 @@ A parameter entry consists of a parameter and a value, in that order, separated
 The parameter may contain leading and trailing tabs and spaces.
 The value, which begins immediately after the equal sign, may contain leading tabs and spaces or be empty.
 Some parameter entries can be given a list of multiple values, in which case the values are separated by commas, tabs, or spaces.
-For a list of users, all users in a system group can be specified by giving the group name prefixed with \fB@\fR.
+For a list of users, all users in a system group can be specified by giving the group name prefixed with an at sign (\fB@\fR).
 A semicolon (\fB;\fR) or a hash (\fB#\fR) marks the beginning of a comment which continues until the end of the line.
 .SH SHARES
 Each section name, except that of the \fBglobal\fR section, defines a shared resource, commonly referred to as a share.
@@ -343,6 +343,6 @@ For bug reports, use the issue tracker at https://github.com/cifsd-team/ksmbd-to
 .SH "SEE ALSO"
 .TP
 \fBUtilities\fR
-\fBksmbd.addshare\fP(1),
-\fBksmbd.adduser\fP(1),
-\fBksmbd.mountd\fP(1)
+\fBksmbd.addshare\fP(8),
+\fBksmbd.adduser\fP(8),
+\fBksmbd.mountd\fP(8)
diff --git a/ksmbdpwd.db.5.in b/ksmbdpwd.db.5.in
index fb8d5f5..dcb4c08 100644
--- a/ksmbdpwd.db.5.in
+++ b/ksmbdpwd.db.5.in
@@ -2,8 +2,8 @@
 .SH NAME
 ksmbdpwd.db \- the user database for ksmbd.mountd
 .SH DESCRIPTION
-\fBksmbdpwd.db\fR is the user database for \fBksmbd.mountd\fP(1) user mode daemon.
-\fBksmbd.adduser\fP(1) may be used for configuring users for \fBksmbdpwd.db\fR.
+\fBksmbdpwd.db\fR is the user database for \fBksmbd.mountd\fP(8) user mode daemon.
+\fBksmbd.adduser\fP(8) may be used for configuring users for \fBksmbdpwd.db\fR.
 \fBksmbd.adduser\fR notifies \fBksmbd.mountd\fR of changes, if it had made any, by sending the \fBSIGHUP\fR signal to \fBksmbd.mountd\fR.
 \fBksmbd.control --reload\fR can be used for notifying \fBksmbd.mountd\fR of changes when not using \fBksmbd.adduser\fR.
 \fBksmbdpwd.db\fR is expected to be at \fB@sysconfdir@/ksmbd/ksmbdpwd.db\fR by default. \" PATH_PWDDB
@@ -23,5 +23,5 @@ For bug reports, use the issue tracker at https://github.com/cifsd-team/ksmbd-to
 .SH "SEE ALSO"
 .TP
 \fBUtilities\fR
-\fBksmbd.adduser\fP(1),
-\fBksmbd.mountd\fP(1)
+\fBksmbd.adduser\fP(8),
+\fBksmbd.mountd\fP(8)
diff --git a/mountd/Makefile.am b/mountd/Makefile.am
index db8796f..1b97872 100644
--- a/mountd/Makefile.am
+++ b/mountd/Makefile.am
@@ -8,9 +8,9 @@ sbin_PROGRAMS = ksmbd.mountd
 ksmbd_mountd_SOURCES = worker.c ipc.c rpc.c rpc_srvsvc.c rpc_wkssvc.c \
                        mountd.c smbacl.c rpc_samr.c rpc_lsarpc.c
 
-EXTRA_DIST = ksmbd.mountd.1.in
+EXTRA_DIST = ksmbd.mountd.8.in
 
-man_MANS = ksmbd.mountd.1
+man_MANS = ksmbd.mountd.8
 $(man_MANS): %: %.in; @$(in_script) $< >$@
 
 CLEANFILES = $(man_MANS)
diff --git a/mountd/ksmbd.mountd.1.in b/mountd/ksmbd.mountd.8.in
similarity index 93%
rename from mountd/ksmbd.mountd.1.in
rename to mountd/ksmbd.mountd.8.in
index 8008709..bd28987 100644
--- a/mountd/ksmbd.mountd.1.in
+++ b/mountd/ksmbd.mountd.8.in
@@ -1,4 +1,4 @@
-.TH KSMBD.MOUNTD "1" "" "ksmbd-tools @ksmbd_tools_version@" "User Commands"
+.TH KSMBD.MOUNTD "8" "" "ksmbd-tools @ksmbd_tools_version@" "System Administration"
 .SH NAME
 ksmbd.mountd \- start ksmbd.mountd and ksmbd daemons
 .SH SYNOPSIS
@@ -58,6 +58,6 @@ For bug reports, use the issue tracker at https://github.com/cifsd-team/ksmbd-to
 \fBksmbdpwd.db\fP(5)
 .TP
 \fBUtilities\fR
-\fBksmbd.addshare\fP(1),
-\fBksmbd.adduser\fP(1),
-\fBksmbd.control\fP(1)
+\fBksmbd.addshare\fP(8),
+\fBksmbd.adduser\fP(8),
+\fBksmbd.control\fP(8)
diff --git a/mountd/meson.build b/mountd/meson.build
index d651d02..3c602df 100644
--- a/mountd/meson.build
+++ b/mountd/meson.build
@@ -24,8 +24,8 @@ mountd = executable(
 )
 
 configure_file(
-  input: 'ksmbd.mountd.1.in',
-  output: 'ksmbd.mountd.1',
-  install_dir: get_option('mandir') / 'man1',
+  input: 'ksmbd.mountd.8.in',
+  output: 'ksmbd.mountd.8',
+  install_dir: get_option('mandir') / 'man8',
   configuration: in_data,
 )
diff --git a/mountd/mountd.c b/mountd/mountd.c
index f1bf7db..de37323 100644
--- a/mountd/mountd.c
+++ b/mountd/mountd.c
@@ -61,7 +61,7 @@ static void usage(int status)
 			"  -V, --version           output version information and exit\n"
 			"  -h, --help              display this help and exit\n"
 			"\n"
-			"See ksmbd.mountd(1) for more details.\n");
+			"See ksmbd.mountd(8) for more details.\n");
 }
 
 static struct option opts[] = {
-- 
2.37.3

[PATCH] ksmbd-tools: improve log message for missing ksmbd.conf in adduser (click the arrow to expand)
From 13c4b65081ae29e618a96bc3b9486a8c4245c556 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Mon, 19 Sep 2022 11:47:51 +0300
Subject: [PATCH] ksmbd-tools: improve log message for missing ksmbd.conf in
 adduser
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When ksmbd.conf does not exist, adduser cannot guard against deletion
of users that are depended on. That includes, but is not limited to,
the guest account. Clarify this in the associated log message.

Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
 adduser/adduser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/adduser/adduser.c b/adduser/adduser.c
index 47c7390..bf49475 100644
--- a/adduser/adduser.c
+++ b/adduser/adduser.c
@@ -95,7 +95,7 @@ static int parse_configs(char *pwddb, char *smbconf)
 	ret = cp_parse_smbconf(smbconf);
 	if (ret == -ENOENT) {
 		pr_info("Configuration file does not exist, "
-			"guest account is unknown\n");
+			"cannot guard against user deletion\n");
 		ret = 0;
 	} else if (ret)
 		pr_err("Failed to parse configuration file\n");
-- 
2.37.3

When you prepare a release, please remember to update the .spec file version also. It can be either a commit or a tag. Like this, assuming you tag 3.4.6:

diff --git a/ksmbd-tools.spec b/ksmbd-tools.spec
index 392665a..fae425b 100644
--- a/ksmbd-tools.spec
+++ b/ksmbd-tools.spec
@@ -16,7 +16,7 @@
 #
 
 Name:           ksmbd-tools
-Version:        e3fc5436af67bfb1559b85022cbcc892fe5323e0
+Version:        3.4.6
 Release:        0
 Summary:        ksmbd kernel server userspace utilities
 License:        GPL-2.0-or-later

@atheik
Copy link
Member Author

atheik commented Sep 19, 2022

@namjaejeon,

Regarding __caseless_lookup(), could you please take a look at this when you have the time?

Will you send this patch to the mailing list ?

If utf8_strncasecmp() fails, there is no fallback to strncasecmp() like there is with the share name casefolding. Is it a problem? Also, strncasecmp() assumes ISO8859-1 as it calls tolower(). We cannot use nls_strnicmp() since nls_tolower() does nothing for UTF-8. It does ASCII lower case conversion only with load_nls("ascii"). How should this be dealt with?

@namjaejeon
Copy link
Member

namjaejeon commented Sep 19, 2022

@atheik Yes, fallback is needed. BTW, Why should we use nls_strnicmp() for ascii ? Why not strncasecmp() ?

@atheik
Copy link
Member Author

atheik commented Sep 20, 2022

@namjaejeon,

@atheik Yes, fallback is needed.

Okay. Would this be alright then?

[PATCH] ksmbd: make utf-8 file name comparison work in __caseless_lookup() (click the arrow to expand)
From cd7c1bc1751ee3d8609cef24f2bada989d1d0828 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Atte=20Heikkil=C3=A4?= <atteh.mailbox@gmail.com>
Date: Tue, 20 Sep 2022 20:42:43 +0300
Subject: [PATCH] ksmbd: make utf-8 file name comparison work in
 __caseless_lookup()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

TODO

Signed-off-by: Atte Heikkilä <atteh.mailbox@gmail.com>
---
 vfs.c | 23 +++++++++++++++++++----
 vfs.h |  1 +
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/vfs.c b/vfs.c
index fb2d763..760c193 100644
--- a/vfs.c
+++ b/vfs.c
@@ -2023,6 +2023,7 @@ static int __caseless_lookup(struct dir_context *ctx, const char *name,
 			     unsigned int d_type)
 {
 	struct ksmbd_readdir_data *buf;
+	int cmp;
 
 	buf = container_of(ctx, struct ksmbd_readdir_data, ctx);
 
@@ -2032,7 +2033,17 @@ static int __caseless_lookup(struct dir_context *ctx, const char *name,
 #else
 		return 0;
 #endif
-	if (!strncasecmp((char *)buf->private, name, namlen)) {
+	if (IS_ENABLED(CONFIG_UNICODE) && buf->um) {
+		const struct qstr q_buf = {.name = buf->private,
+					   .len = buf->used};
+		const struct qstr q_name = {.name = name,
+					    .len = namlen};
+
+		cmp = utf8_strncasecmp(buf->um, &q_buf, &q_name);
+	}
+	if (!(IS_ENABLED(CONFIG_UNICODE) && buf->um) || cmp < 0)
+		cmp = strncasecmp((char *)buf->private, name, namlen);
+	if (!cmp) {
 		memcpy((char *)buf->private, name, namlen);
 		buf->dirent_count = 1;
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(6, 1, 0)
@@ -2056,7 +2067,8 @@ static int __caseless_lookup(struct dir_context *ctx, const char *name,
  *
  * Return:	0 on success, otherwise error
  */
-static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name, size_t namelen)
+static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name,
+				   size_t namelen, struct unicode_map *um)
 {
 	int ret;
 	struct file *dfilp;
@@ -2066,6 +2078,7 @@ static int ksmbd_vfs_lookup_in_dir(const struct path *dir, char *name, size_t na
 		.private	= name,
 		.used		= namelen,
 		.dirent_count	= 0,
+		.um		= um,
 	};
 
 	dfilp = dentry_open(dir, flags, current_cred());
@@ -2129,7 +2142,8 @@ int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
 				break;
 
 			err = ksmbd_vfs_lookup_in_dir(&parent, filename,
-						      filename_len);
+						      filename_len,
+						      work->conn->um);
 			path_put(&parent);
 			if (err)
 				goto out;
@@ -2204,7 +2218,8 @@ int ksmbd_vfs_kern_path(struct ksmbd_work *work, char *name,
 				break;
 
 			err = ksmbd_vfs_lookup_in_dir(&parent, filename,
-						      filename_len);
+						      filename_len,
+						      work->conn->um);
 			if (err) {
 				path_put(&parent);
 				goto out;
diff --git a/vfs.h b/vfs.h
index dc2aec4..29c1d76 100644
--- a/vfs.h
+++ b/vfs.h
@@ -102,6 +102,7 @@ struct ksmbd_readdir_data {
 	unsigned int		used;
 	unsigned int		dirent_count;
 	unsigned int		file_attr;
+	struct unicode_map	*um;
 };
 
 /* ksmbd kstat wrapper to get valid create time when reading dir entry */
-- 
2.37.3

BTW, Why should we use nls_strnicmp() for ascii ? Why not strncasecmp() ?

Like I said, strncasecmp() assumes ISO8859-1 since it calls tolower(). Since buf->private and name are not ISO8859-1, it is strange to treat them as such. I suppose it doesn't matter here since strncasecmp() corrupts both arguments the same way in the fallback scenario (for bytes in the non-ASCII range), so the comparison works. The share name ASCII fallback at least checks with isascii() so that it doesn't call tolower() for bytes outside the ASCII range. nls_strnicmp() would need load_nls("ascii") since nls_tolower() does nothing with load_nls("utf8").

@atheik
Copy link
Member Author

atheik commented Sep 20, 2022

@namjaejeon,

BTW, Why should we use nls_strnicmp() for ascii ? Why not strncasecmp() ?

To clarify, I don't think there is anything wrong with using strncasecmp() for ASCII. But since buf->private and name are not limited to the ASCII range, and since that additional range is not ISO8859-1 compatible, it seems odd to use strncasecmp() here. When strncasecmp() calls to tolower() for bytes in the C0 to DE range, the result only makes sense for ISO8859-1 compatible encodings. And, the intention here (to my understanding) is that the comparison should be made when ignoring only the ASCII case. It doesn't break anything and is probably not worth fixing. I was just confused by it.

@namjaejeon
Copy link
Member

@atheik there is unix charset parameter in smb.conf of samba. I think that user can change charset they want. ksmbd-tools send this info to ksmbd kernel. ksmbd kernel will call load_nl() using it. how do you think ?

unix charset (G)
Specifies the charset the unix machine Samba runs on uses. Samba needs to know this in order to be able to convert text to the charsets other SMB clients use.

This is also the charset Samba will use when specifying arguments to scripts that it invokes.

Default: unix charset = UTF-8

Example: unix charset = ASCII

@atheik
Copy link
Member Author

atheik commented Sep 21, 2022

@namjaejeon,

@atheik there is unix charset parameter in smb.conf of samba. I think that user can change charset they want. ksmbd-tools send this info to ksmbd kernel. ksmbd kernel will call load_nl() using it. how do you think ?

Then, would there still be a need for a fallback if either utf8_strncasecmp() or utf8_casefold() fails?

@namjaejeon
Copy link
Member

@atheik

There is a fallback codes at other place utf8_strncasecmp is used. I didn't take a look deeply because I'm looking at other pressing issues. If you add "unix charset" parameter, I'm worried that it probably needs a lot of changes overall. In particular, the previously forced use of UTF8 (select NLS_UTF8) and CONFIG_UNICODE should be used only for UTF8, right?

@atheik
Copy link
Member Author

atheik commented Sep 22, 2022

@namjaejeon,

@atheik

There is a fallback codes at other place utf8_strncasecmp is used. I didn't take a look deeply because I'm looking at other pressing issues. If you add "unix charset" parameter, I'm worried that it probably needs a lot of changes overall.

Okay. I will look into this further.

In particular, the previously forced use of UTF8 (select NLS_UTF8) and CONFIG_UNICODE should be used only for UTF8, right?

Yes, that is my understanding of it as well.

Would you consider the ksmbd-tools patches at #278 (comment)? I updated the first patch since I had forgotten to change the section numbers at README.md.

@namjaejeon
Copy link
Member

@atheik I have applied 2 patches now. Thanks for your work:)

When you prepare a release, please remember to update the .spec file version also. It can be either a commit or a tag. Like this, assuming you tag 3.4.6:

Okay! I will do that. Thanks.

@namjaejeon
Copy link
Member

@atheik

Okay. I will look into this further.

Really thanks a lot!

@atheik
Copy link
Member Author

atheik commented Sep 24, 2022

@namjaejeon,

One more thing to note regarding UTF-8 share names. When CONFIG_UNICODE is not set, not only UTF-8 share names will be case-sensitive, but they won't work at all. When ksmbd sends a share config request with the ASCII lowercased share name (containing also UTF-8 characters outside of the ASCII range), ksmbd-tools looks it up instead of the UTF-8 casefolded share name, and so the share is not found. So, existing users that rely on UTF-8 share names must set CONFIG_UNICODE. We might also try to work around this in share_config_request() or shm_lookup_share() of ksmbd-tools by looking up also the ASCII lowercased share name, since we don't know whether the kernel has CONFIG_UNICODE set.

@namjaejeon
Copy link
Member

@atheik

Hm.. How about ksmbd sending the info that utf8 config is enable or not in kernel to ksmbd-tools using ipc ?

/*
 * IPC request to fetch net share config.
 */
struct ksmbd_share_config_request {
        __u32   handle;
        __s8    share_name[KSMBD_REQ_MAX_SHARE_NAME]; /* share name */
        __u32   reserved[16];           /* Reserved room */
};

We can use one byte from reserved[].

struct ksmbd_share_config_request {
        __u32   handle;
        __s8    share_name[KSMBD_REQ_MAX_SHARE_NAME]; /* share name */
        __s8    utf8_config_enable;
        __s8   reserved[63];           /* Reserved room */
};

utf8_config_enable = IS_ENABLED(CONFIG_UNICODE) ? true : false;

@atheik
Copy link
Member Author

atheik commented Sep 25, 2022

@namjaejeon,

@atheik

Hm.. How about ksmbd sending the info that utf8 config is enable or not in kernel to ksmbd-tools using ipc ?

Yes, that's one option. Alternatively, how about this?

diff --git a/lib/management/share.c b/lib/management/share.c
index 02e45df..0904401 100644
--- a/lib/management/share.c
+++ b/lib/management/share.c
@@ -266,6 +266,14 @@ struct ksmbd_share *shm_lookup_share(char *name)
 
 	g_rw_lock_reader_lock(&shares_table_lock);
 	share = __shm_lookup_share(name);
+	if (!share) {
+		char *cf_name;
+
+		cf_name = shm_casefold_share_name(name, strlen(name));
+		if (cf_name)
+			share = __shm_lookup_share(cf_name);
+		g_free(cf_name);
+	}
 	if (share) {
 		ret = get_ksmbd_share(share);
 		if (!ret)

@namjaejeon
Copy link
Member

@atheik looks good to me:)

@atheik
Copy link
Member Author

atheik commented Sep 25, 2022

@namjaejeon,

Yes, that's one option. Alternatively, how about this?

With this patch, when CONFIG_UNICODE is not set, UTF-8 share names are case-insensitive but each UTF-8 case permutation of the share name triggers a share config request to user space. Like this:

smbclient --user=MyUserB --password= //127.0.0.1/eä
# requested share config

smbclient --user=MyUserB --password= //127.0.0.1/eä
# cached share config

smbclient --user=MyUserB --password= //127.0.0.1/Eä
# cached share config (ASCII character (e) changed case)

smbclient --user=MyUserB --password= //127.0.0.1/EÄ
# requested share config (non-ASCII character (ä) changed case)

smbclient --user=MyUserB --password= //127.0.0.1/eÄ
# cached share config

So, to ksmbd, share 1 (eä or Eä) and share 2 (eÄ and EÄ) are different shares trigger their own initial share config requests, respectively. This is a problem, right? With this, the behavior would be almost identical to #278 (comment) when only doing user space casefolding.

@namjaejeon
Copy link
Member

@atheik

This is a problem, right? With this, the behavior would be almost identical to #278 (comment) when only doing user space casefolding.

Yes.

@namjaejeon
Copy link
Member

@atheik And can we discuss ksmbd/ksmbd-tools on Discussions tab in ksmbd-tools github from now ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants