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

kernel: add new cifsd module and cifsd-tools #1788

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@Andy2244
Copy link
Contributor

Andy2244 commented Feb 3, 2019

Maintainer: me
Compile tested: arm/mips (master)
Run tested: arm (master)

Description:

  • adds cifs/smb kernel server module cifsd
  • adds userspace tools (cifsd, cifsadmin)

This is a new, simple smb2/3 compatible fileserver. The project is still relatively new, but worked fine for me as simple to setup fileshare. The devs plan to get this merged into upstream eventually, but may take a while.

The advantages over samba3/4 is size and potentially speed. I could not really test this, since my router is too powerfully, so i always get max samba network speeds. Yet i did observe lower cpu usage with this server.

I tested video, music streaming which worked fine. I also tested running portableapps.com from a mounted share, including Firefox Portable, which also works now.

In general the devs try to reuse samba config options, so that the smb.conf file looks very similar. Some advanced options you may use via samba are not yet implemented, like:

unix charset
smb encrypt
fstype
dos filemode
inherit owner

PS: The firmware size (squashfs-sysupgrade.bin) for mvebu (arm) increases by 860kb with it build, where 135kb is for the cifsd+tools and the remainder is for extra dependencies, notably glib2.

@diizzyy

This comment has been minimized.

Copy link
Member

diizzyy commented Feb 4, 2019

As long as it doesn't work reasonably well for everyday use I think we should hold off merging it or at least prevent buildbots to generate a binary but I don't think it's possible without also disabling compiling from source?

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 4, 2019

I think it works "reasonably well" in the context of someone who cant/wont use samba4, because of size and memory limitation and wants better speeds than samba3 or needs smbv3 support.

If you use my guest config example, just change the path and mount the location as network drive most common scenarios work fine. Even samba3 has problems running binarys directly from a share, because of locking and bad mapped memory region support.

PS: I mean we can keep it in snapshots and i will retest the next batch of dev merges. I was thinking having a alternative to samba4 is better, even if its not fully stable yet.

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 7, 2019

111k + 32k. Not bad.

I just tried this on a ramips NAS style device. It seems to work reasonably well. Still the same Unicode bugs.

Note that something like this probably needs to replace samba3 as Windows 10 and modern Linux has obsoleted CIFSv1.

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 7, 2019

111k + 32k. Not bad.

Yeah but keep in mind that the tools need glib2, which is rarely used by packages, so its around 800kb total, still way ahead of samba4.
Yes i also saw the potential to have something simple/small similar to the nfs-kernel-server.

PS: What "Unicode bugs" you got? Just want to check if its the same i got and have issues open for.

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 7, 2019

Was copying my Music to a share. Got this error.

rsync: mkstemp "/home/mangix/test/AccurateRip/Pop/2NE1/2nd Mini Album/.01 - 내가 제일 잘 나가 (I Am the Best).flac.YFQzKt" failed: No such file or directory (2)

and many like it. With Samba 3, the file would transfer with a totally messed up ASCII filename. With cifsd, it errors. Not a big deal since I can use normal rsync to transfer them afterwards.

The reason I rsync to a Samba share is because of speed (4MB/s vs. 22MB/s). AES is slow on mt7621.

edit: I forgot. Mounting the share with vers=2.0 or 2.1 turns encryption off. 3 and above enable it.

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 7, 2019

Ah ok, did not test this unicode scenario. This should probably work, since the devs are Korean and should have personal interest in getting this to work properly. I will add this to the issues, is there any more error output in the log?

PS: Btw how is performance on your low end device compared to samba3?

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 7, 2019

Hahaha I didn't immediately realize that example was Korean. I also get this spam in dmesg

kcifsd: check_invalid_char:143: found invalid character : 0x3f

Not sure if related.

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 7, 2019

Mounting the share with vers=2.0 or 2.1 turns encryption off. 3 and above enable it.

Is there a unencrypted version for smb3 in the specs? If so i can ask for a option to disable encryption, since that's a valid use case for us.

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 7, 2019

No idea. I have not tested Samba 4 with encryption disabled.

@bobafetthotmail

This comment has been minimized.

Copy link
Contributor

bobafetthotmail commented Feb 7, 2019

Mounting the share with vers=2.0 or 2.1 turns encryption off. 3 and above enable it.

Is there a unencrypted version for smb3 in the specs? If so i can ask for a option to disable encryption, since that's a valid use case for us.

Afaik yes, you can disable it in Windows server settings
https://blogs.msdn.microsoft.com/openspecification/2017/05/26/smb-2-and-smb-3-security-in-windows-10-the-anatomy-of-signing-and-cryptographic-keys/

It seems that there is nothing in the client that will refuse a connection if your server does not enable encryption, while there is a server-side option that will refuse connection to clients not supporting encryption.

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from 7b0bf9f to a8eda3e Feb 8, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 8, 2019

  • updated to latest git
  • several of my reported issues have been fixed, including the explorer (windows) browse problems

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from a8eda3e to a183a77 Feb 13, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 13, 2019

  • update to latest git
  • remove cifsdpwd.db file, default dummy user
  • use 8s ipc timout, to ensure service is stoped/closed fully
  • fixes win10 access errors
  • fix share properties page segfault
@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 13, 2019

May want to pass -DCONFIG_CIFSD_ACL to the cifsd module. mounting with mount.cifs -o cifsacl will fail.

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from a183a77 to 7e5818f Feb 14, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 14, 2019

  • update to latest git
  • build kmod with CONFIG_CIFSD_ACL

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from 7e5818f to b6368d1 Feb 15, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Feb 15, 2019

  • update to latest git
  • fixes unicode issues (inaccessible files, folders due to illegal chars)
  • fixes issues regarding the ability to run binary's from share
  • moved tools package to Filesystem (similar to nfsd)

PS: This update fixed two (namjaejeon/cifsd#175, namjaejeon/cifsd-tools#68) of the last big issues we had with this package and the only bigger outstanding issues is related to more complex locking aka correctly running firefox portable directly from a network share.
I think we can wait if the locking issues can be fixed in the next couple of days, but from my point of view i'm gtg. What do you think @neheb ?

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 15, 2019

CONFIG_NLS_DEFAULT could also get set to utf8 anyway.

I’ve been testing this since the beginning. Most of my problems have been solved.

Making encryption optional would also be nice. I currently work around this by setting server max protocol to 2.

@namjaejeon

This comment has been minimized.

Copy link

namjaejeon commented Feb 16, 2019

@neheb no need to set utf8 in CONFIG_NLS_DEFAULT with the latest cifsd master. I add the patch(namjaejeon/cifsd@5bef562) to fix it. CONFIG_NLS_DEFAULT could be get set to old one(iso8859-1). and I add new patch(namjaejeon/cifsd@cb0935b) to turn encryption off. Okay, I will add the parameter in smb.conf to control enable/disable.

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 16, 2019

Well, yeah. For size reasons, it might make sense to default it to utf8 anyways. I wonder if there's anything besides cifsd that really uses NLS in OpenWrt though...

Disabling encryption by default is fine with me. If people really want encrypted file transfer they can use rsync or scp. It's faster anyway.

@namjaejeon

This comment has been minimized.

Copy link

namjaejeon commented Feb 16, 2019

I wonder if there's anything besides cifsd that really uses NLS in OpenWrt though...

It is no issue. The use of utf8 by cifsd is independent and does not affect other modules.

Disabling encryption by default is fine with me. If people really want encrypted file transfer they can use rsync or scp. It's faster anyway.

Okay:) Thanks for your comment!

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from b6368d1 to b61fa95 Feb 16, 2019

if (lsmod | grep cifsd &>/dev/null); then
rmmod cifsd &>/dev/null
fi
modprobe cifsd 2>/dev/null

This comment has been minimized.

Copy link
@hauke

hauke Feb 16, 2019

Member

Why is it needed to remove the kernel module before this gets started?

This comment has been minimized.

Copy link
@Andy2244

Andy2244 Feb 16, 2019

Author Contributor

Some changed setting in smb.conf may not be fully reflected without a full remove, so i just added a extra try before start.

Show resolved Hide resolved package/kernel/cifsd/Makefile Outdated
+kmod-crypto-ccm
KCONFIG:= \
CONFIG_KEYS=y \
CONFIG_CRYPTO_ARC4=y

This comment has been minimized.

Copy link
@hauke

hauke Feb 16, 2019

Member

CONFIG_CRYPTO_ARC4 is already compiled in in OpenWrt.

This comment has been minimized.

Copy link
@Andy2244

Andy2244 Feb 16, 2019

Author Contributor

Just to ensure it stays this way if something changes or was locally changed. It was not clear to me how those cases are handled, since i assumed if you compile a custom kernel this setting always ensures you get a compatible one if you select this package?

This comment has been minimized.

Copy link
@Andy2244

Andy2244 Feb 25, 2019

Author Contributor

Ok i'm still confused, maybe you explain how things should work.

If you use KCONFIG:= and the package is selected than you always end up with a compatible kernel, if you build one. So for this reason i don't understand why we would remove the options from the package?

I understand that we use the generic/config template to ensure that those packages can be installed and work without needing a new kernel, but to me that's just a extra specific option we choose.

Is there any harm by including the needed options via KCONFIG:=? I find this more clear since it ensures a working kernel and documents what options a module actually needs.

This comment has been minimized.

Copy link
@diizzyy

diizzyy Feb 25, 2019

Member

I think the issue is that opkg doesn't know of kernel config depends which means that you can technically install a package which wont run on in the end unless whaterver you need is provided in a module.

Show resolved Hide resolved package/kernel/cifsd/Makefile Outdated
Show resolved Hide resolved package/kernel/cifsd/Makefile Outdated
Show resolved Hide resolved package/network/services/cifsd-tools/Makefile
Show resolved Hide resolved package/network/services/cifsd-tools/files/cifsd.init Outdated
@namjaejeon

This comment has been minimized.

Copy link

namjaejeon commented Feb 19, 2019

@neheb I added the patch (namjaejeon/cifsd@9b9f145) to make signing is defautly disable with SMB3. You don't need to downgrade SMB version to avoid signing and encryption overhead with the latest cifsd.

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Feb 19, 2019

Signing is just for authentication, right? I just tested on Windows with the previous version and was getting ~50MB/s (probably the limit for this RAID array) with ~40% sys usage according to top. Better than the 5 that I was getting with encryption.

edit: btw, it's probably better to implement AES128-GCM for cifsd as that more often has hardware accelerators (not on my platform though).

@namjaejeon

This comment has been minimized.

Copy link

namjaejeon commented Feb 19, 2019

No, If server signing is enable, all messages (such as read/write) are signed as well as authentication. and It was causing performance down with SMB3 before. Yep, It is in my pocket. thinking check it after closing urgent issues and requests. Anyway. You can use cifsd with SMB3 now:)

@diizzyy

This comment has been minimized.

Copy link
Member

diizzyy commented Mar 13, 2019

Can we switch to codeload? :-)

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from ea9b405 to d3d4e9a Mar 13, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Mar 13, 2019

Can we switch to codeload? :-)

Sure + done, but it always confuses me what i need to change for codeload.
Could we maybe make this into a simple PKG_SOURCE_PROTO change, without the need to add custom PKG_SOURCE, PKG_BUILD_DIR?

PKG_SOURCE_PROTO:=git -> PKG_SOURCE_PROTO:=git-codeload

@ynezz

This comment has been minimized.

Copy link
Member

ynezz commented Mar 13, 2019

Could we maybe make this into a simple PKG_SOURCE_PROTO change, without the need to add custom PKG_SOURCE, PKG_BUILD_DIR

Well, for example in your cifsd case, how would you then resolve complete URL, mainly the cifsd-team in the path?

Ok if you can confirm that this versions works on mips, i think we are gtg.

What about the missing procd support?

BTW, I think, that having configuration handled via UCI would be nice to have as well, not just because of LuCI, but also for preserving configuration during sysupgrades, easier configuration etc.

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Mar 13, 2019

What about the missing procd support?

As noted, procd + cifsd was crashing/restarting my router. Is procd now required for all new packages?
I would rather spend my time on trying to get wsdd2 + luci/uci ready and put the procd issues on a lower priority.

Well, for example in your cifsd case, how would you then resolve complete URL, mainly the cifsd-team in the path?

Given a git URL, it should be possible to automatically construct a valid codeload one out of it.
PKG_SOURCE_URL:=https://github.com/cifsd-team/cifsd-tools.git
PKG_SOURCE_URL:=https://codeload.github.com/cifsd-team/cifsd-tools/tar.gz/$(PKG_SOURCE_VERSION)?

@ynezz

This comment has been minimized.

Copy link
Member

ynezz commented Mar 13, 2019

As noted, procd + cifsd was crashing/restarting my router.

I know, I've also suggested to you to report it, so it could be fixed (From your description it looks like serious bug, so I guess, that it might have been fixed probably by now). The thing is, that some of the developers who might be able to fix this issue don't follow the GitHub parallel universe, but they do read bug reports occasionally.

Is procd now required for all new packages?

Well I'm not aware about such rule, but I've just quickly checked daemons in the repository and can't find any example of package which has daemon and is not using procd for it's handling.

@dedeckeh

This comment has been minimized.

Copy link
Contributor

dedeckeh commented Mar 13, 2019

As noted, procd + cifsd was crashing/restarting my router.

I know, I've also suggested to you to report it, so it could be fixed (From your description it looks like serious bug, so I guess, that it might have been fixed probably by now). The thing is, that some of the developers who might be able to fix this issue don't follow the GitHub parallel universe, but they do read bug reports occasionally.

Is procd now required for all new packages?

Well I'm not aware about such rule, but I've just quickly checked daemons in the repository and can't find any example of package which has daemon and is not using procd for it's handling.

There's very strong preference to use procd at least for the daemons in the core repo

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from d3d4e9a to 04de5b1 Apr 1, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Apr 1, 2019

  • update to git (2019-03-28)
  • adds options (bind interfaces only, interfaces, deadtime)
  • use procd (crashes router on stopping the service) FS#2215

@namjaejeon Regarding the procd crash, maybe its because cifsd -n, will spawn a identically named cifsd watcher process? Can we rename this extra process or make it optional? What does it actually do?

@sergey-senozhatsky

This comment has been minimized.

Copy link

sergey-senozhatsky commented Apr 2, 2019

@namjaejeon Regarding the procd crash, maybe its because cifsd -n, will spawn a identically named cifsd watcher process? Can we rename this extra process or make it optional?

I'm not Namjae, but we can rename that process, I guess...
I need to catch up with this whole discussion.

What does procd do if, say, firefox or chrome or httpd or postresql spawn a number of identically named processes?

What does it actually do?

It does some actual work (DCE RPC, IPC, etc.).

-ss

@sergey-senozhatsky

This comment has been minimized.

Copy link

sergey-senozhatsky commented Apr 4, 2019

@Andy2244 does it make any difference if you run cifsd in "alternative" --no-detach mode cifsd --n=2?

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Apr 4, 2019

@Andy2244 does it make any difference if you run cifsd in "alternative" --no-detach mode cifsd --n=2?
Will try the new option, if i get back from a trip next week, thanks.

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch 2 times, most recently from 90233cd to 0341e44 Apr 9, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Apr 9, 2019

  • updated to latest git (fixes procd crash)
  • updated examples
  • added bind interfaces only = yes , interfaces = br-lan to guest example

@sergey-senozhatsky ok the latest update seem to have fixed the procd issue, this means --n now works, but --n=2 still reboots/crashes. Does this make sense, what did you change for the --n option ?

PS: @diizzyy @ynezz can you verify the fix and see if anything else is missing for a initial commit?

@sergey-senozhatsky

This comment has been minimized.

Copy link

sergey-senozhatsky commented Apr 9, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Apr 9, 2019

We now setsid() in --n (--n=1) and don't setsid() in --n=2 mode.

So setsid() was not used before? My test build was a couple of weeks old.
Not sure why procd crashes without it set.

thanks for the fix anyway

@sergey-senozhatsky

This comment has been minimized.

Copy link

sergey-senozhatsky commented Apr 9, 2019

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Apr 15, 2019

@Andy2244 BUILD_DIR should probably be in the kernel directory.

This is not yet ready for pulling. Big endian issues strike again (I previously only tested mounting, not using).

kernel: add new cifsd module and cifsd-tools
* adds cifs/smb kernel server module cifsd
* adds userspace tools (cifsd, cifsadmin)

Signed-off-by: Andy Walsh <andy.walsh44+github@gmail.com>

@Andy2244 Andy2244 force-pushed the Andy2244:cifsd branch from 0341e44 to 76d2d29 Apr 16, 2019

@Andy2244

This comment has been minimized.

Copy link
Contributor Author

Andy2244 commented Apr 16, 2019

BUILD_DIR should probably be in the kernel directory.

Done

@namjaejeon

This comment has been minimized.

Copy link

namjaejeon commented Apr 16, 2019

@Andy2244 Big endian issue is clear now. Rosen helped to improve the problem. Thanks Rosen!

@dengqf6

This comment has been minimized.

Copy link
Contributor

dengqf6 commented Apr 20, 2019

I've encountered redefinition of 'put_unaligned_be64' error when building against kernel 4.19
According to https://lore.kernel.org/patchwork/patch/729241/

I changed this include to <asm/unaligned.h> and it worked

@sergey-senozhatsky

This comment has been minimized.

Copy link

sergey-senozhatsky commented Apr 20, 2019

@dengqf6 yeah, it seems that asm/unaligned.h is what we need to include.

~/_next$ git grep "include <asm/unaligned.h>" | wc -l
522

Care to submit a patch (cifsd)?

@dengqf6

This comment has been minimized.

Copy link
Contributor

dengqf6 commented Apr 20, 2019

@namjaejeon

This comment has been minimized.

Copy link

namjaejeon commented Apr 21, 2019

@dengqf6 Applied! Thanks for your work!

@dengqf6

This comment has been minimized.

Copy link
Contributor

dengqf6 commented Apr 21, 2019

The latest git version builds flawlessly. But I cannot get it working on Windows 10.
It reports either 0x80070035 or 0x80070043 error, while not a single packet to TCP port 445 can be seen.
Is it a configuration issue, or it needs other packages such as wsdd2 to work?

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Apr 21, 2019

@dengqf6 should report upstream. Sounds like configuration issue.

@namjaejeon

This comment has been minimized.

Copy link

namjaejeon commented Apr 22, 2019

@dengqf6 have you updated latest cifsd-tools as well kernel cifsd ? If you update only latest cifsd, It could not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.