2023-05-21 06:02:25

by Steve French

[permalink] [raw]
Subject: [GIT PULL] ksmbd server fixes

Please pull the following changes since commit
f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6:

Linux 6.4-rc2 (2023-05-14 12:51:40 -0700)

are available in the Git repository at:

git://git.samba.org/ksmbd.git tags/6.4-rc2-ksmbd-server-fixes

for you to fetch changes up to e7b8b8ed9960bf699bf4029f482d9e869c094ed6:

ksmbd: smb2: Allow messages padded to 8byte boundary (2023-05-16
10:26:14 -0500)

----------------------------------------------------------------
Four ksmbd server fixes:
- two fixes for incorrect SMB3 message validation (one for client
which uses 8 byte padding, and one for empty bcc)
- two fixes for out of bounds bugs: one for username offset checks (in
session setup) and the other for create context name length checks (in
open requests)
----------------------------------------------------------------
Chih-Yen Chang (3):
ksmbd: fix global-out-of-bounds in smb2_find_context_vals
ksmbd: fix wrong UserName check in session_user
ksmbd: allocate one more byte for implied bcc[0]

Gustav Johansson (1):
ksmbd: smb2: Allow messages padded to 8byte boundary

fs/ksmbd/connection.c | 3 ++-
fs/ksmbd/oplock.c | 5 +++--
fs/ksmbd/oplock.h | 2 +-
fs/ksmbd/smb2misc.c | 5 ++++-
fs/ksmbd/smb2pdu.c | 19 +++++++++----------
5 files changed, 19 insertions(+), 15 deletions(-)

--
Thanks,

Steve


2023-05-21 18:32:49

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT PULL] ksmbd server fixes

On Sat, May 20, 2023 at 10:14 PM Steve French <[email protected]> wrote:
>
> Four ksmbd server fixes:

This reply is not really directly related to this pull (which I just
did), but more of a reaction to getting the cifs and ksmbd pulls next
to each other again.

We talked about directory layout issues some time ago, and there's
kind of beginnings of it, but it never happened, and the parts that
*did* happen I'm not super-happy about. That "fs/smbfs_common/"
subdirectory is just fairly ugly.

Would you mind horribly to just bite the bullet, and rename things,
and put it all under "smbfs". Something like

mkdir fs/smbfs
git mv fs/cifs fs/smbfs/client
git mv fs/ksmbd fs/smbfs/server
git mv fs/smbfs_common fs/smbfs/common

plus the required Makefile editing to just make it all build right?

And if you prefer just "fs/smb" over "fs/smbfs", that sounds fine to
me too, but I guess this all really does just the filesystem part
(rather than all the printing and whatever other stuff that smb also
contains).

I dunno. I just feel like the current organization and naming is this
odd half-way state, and we could just fairly easily do the above.

I could do it myself, of course, and git will sort out any rename
issues. But me doing it seems silly when you maintain all three
pieces.

Linus

2023-05-21 18:32:59

by pr-tracker-bot

[permalink] [raw]
Subject: Re: [GIT PULL] ksmbd server fixes

The pull request you sent on Sun, 21 May 2023 00:14:35 -0500:

> git://git.samba.org/ksmbd.git tags/6.4-rc2-ksmbd-server-fixes

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e2065b8c1b0120d47b327364a1a09090bdc11f31

Thank you!

--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

2023-05-21 19:04:43

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT PULL] ksmbd server fixes

On Sun, May 21, 2023 at 11:06 AM Linus Torvalds
<[email protected]> wrote:
>
> Would you mind horribly to just bite the bullet, and rename things,
> and put it all under "smbfs". Something like
>
> mkdir fs/smbfs
> git mv fs/cifs fs/smbfs/client
> git mv fs/ksmbd fs/smbfs/server
> git mv fs/smbfs_common fs/smbfs/common
>
> plus the required Makefile editing to just make it all build right?

Just because I wanted to test it, here's a patch that is based on the
above directory moves, with the (tiny) extra work to set up the
Kconfig and Makefile changes, and some trivial "fix up include file
paths".

It's "tested" in the sense that it passes an "allmodconfig" build for
me, but that's literally all I checked.

I *did* end up changing the Kconfig variable name for SMBFS_COMMON to
be just SMBFS. It made more sense that way with the re-org. And I
think the patch looks pretty good, with things like

-#include "../smbfs_common/arc4.h"
+#include "../common/arc4.h"

just looking better as a result of this all. No?

But what I did *not* do was to fix up things like paths in the
documentation build etc. The location of the resulting module.ko files
obviously change, for example, and I find a reference to
fs/cifs/ioctl.c in the ioctl list etc.

Again - I'm not at all saying that you *have* to do this. And I am
*not* going to commit this diff to my own tree, for example.

I'm just sending the diff out to make it really easy for you to try it
out if you happen to agree with what I'm suggesting.

What do you think?

Linus


Attachments:
patch.diff (35.71 kB)

2023-05-21 19:14:15

by Steve French

[permalink] [raw]
Subject: Re: [GIT PULL] ksmbd server fixes

On Sun, May 21, 2023 at 1:06 PM Linus Torvalds
<[email protected]> wrote:
<...>

> We talked about directory layout issues some time ago, and there's
> kind of beginnings of it, but it never happened, and the parts that
> *did* happen I'm not super-happy about. That "fs/smbfs_common/"
> subdirectory is just fairly ugly.
>
> Would you mind horribly to just bite the bullet, and rename things,
> and put it all under "smbfs". Something like
>
> mkdir fs/smbfs
> git mv fs/cifs fs/smbfs/client
> git mv fs/ksmbd fs/smbfs/server
> git mv fs/smbfs_common fs/smbfs/common
>
> plus the required Makefile editing to just make it all build right?
>
> And if you prefer just "fs/smb" over "fs/smbfs", that sounds fine to
> me too, but I guess this all really does just the filesystem part
> (rather than all the printing and whatever other stuff that smb also
> contains).

Should be easy to move and more intuitive, although I would prefer
that we use fs/smb or fs/smb3 as the directory name instead of
fs/smbfs (since that old filesystem fs/smbfs was removed in 2.6.27.
So reusing the fs/smbfs directory name could confuse people looking
at git logs). I am fine with doing (and related Kconfig and trivial
include path changes):

git mv fs/cifs fs/smb/client
git mv fs/ksmbd fs/smb/server
git mv fs/smbfs_common fs/smb/common

This also might be a good time to take another step toward
moving people away from the old, less secure, SMB1 ("cifs")
dialect by adding a second .ko file to the client (e.g. add
"smb3.ko" in fs/smb/client) which would have support for the old
SMB1/CIFS dialect removed. We would keep cifs.ko for
compatibility purposes (cifs.ko would still have its current
config options so would have SMB1/CIFS enabled by default
but smb3.ko would only support modern, more secure dialects
and also would be smaller than cifs.ko since it would build with
CIFS_ALLOW_INSECURE_LEGACY disabled). There is a
precedent for something somewhat similar to this e.g. the NFS
client has four kernel modules in the fs/nfs directory. On a
somewhat related note - at the Storage Developer Conference
last fall there was an interesting set of discussions around splitting
out the RDMA mapping layer ("smbdirect") by removing RDMA
related code from ksmbd.ko and cifs.ko and creating an
"smbdirect.ko" module for various RDMA use cases (not just cifs.ko
and ksmbd but perhaps also could help some user space apps,
Samba etc. in the future) - this "smbdirect.ko" (RDMA mapping
layer) could also go in fs/smb/common in the future.

I would be happy to do the move (to fs/smb) of the directories and
update the config soon (seems reasonably low risk) - let me know if you
want me to send it this week or wait till 6.5-rc
--
Thanks,

Steve

2023-05-21 19:26:31

by Linus Torvalds

[permalink] [raw]
Subject: Re: [GIT PULL] ksmbd server fixes

On Sun, May 21, 2023 at 12:03 PM Steve French <[email protected]> wrote:
>
> I would be happy to do the move (to fs/smb) of the directories and
> update the config soon (seems reasonably low risk) - let me know if you
> want me to send it this week or wait till 6.5-rc

So I think the "do it now or wait until the 6.5 merge window" is
entirely up to you.

We've often intentionally done big renames during the "quiet time"
after the merge window is oven, just because doing them during the
merge window can be somewhat painful with unnecessary conflicts.

I would *not* want to do it during the last week of the release, just
in case there are small details that need to be fixed up, but doing it
now during the rc3/rc4 kind of timeframe is not only fairly quiet, but
also gives us time to find any surprises.

So in that sense, doing it now is likely one of the better times, and
a pure rename should not be risky from a code standpoint.

At the same time, doing it during the merge window isn't *wrong*
either. Despite the somewhat painful merge with folio changes, I
don't think fs/cifs/ or fs/ksmbd/ normally have a lot of conflicts,
and git does handle rename conflicts fairly well unless there's just
lots of complexity.

So it's really fine either way. The normal kind of "big changes"
should obviously always be merge window things, but pure renames
really are different and are often done outside of the merge window
(the same way I intentionally did the MAINTAINERS re-ordering just
*after* the merge window)

But we don't do renames often enough to have any kind of strict rules
about things like this.

So I think "whenever is most convenient for you" is the thing to aim
for here. This is *not* a "only during merge window" kind of thing.

Linus

2023-05-22 04:51:30

by Steve French

[permalink] [raw]
Subject: Re: [GIT PULL] ksmbd server fixes

On Sun, May 21, 2023 at 2:21 PM Linus Torvalds
<[email protected]> wrote:
>
> On Sun, May 21, 2023 at 12:03 PM Steve French <[email protected]> wrote:
> >
> > I would be happy to do the move (to fs/smb) of the directories and
> > update the config soon (seems reasonably low risk) - let me know if you
> > want me to send it this week or wait till 6.5-rc
>
> So I think the "do it now or wait until the 6.5 merge window" is
> entirely up to you.
>
> We've often intentionally done big renames during the "quiet time"
> after the merge window is oven, just because doing them during the
> merge window can be somewhat painful with unnecessary conflicts.
>
> I would *not* want to do it during the last week of the release, just
> in case there are small details that need to be fixed up, but doing it
> now during the rc3/rc4 kind of timeframe is not only fairly quiet, but
> also gives us time to find any surprises.
>
> So in that sense, doing it now is likely one of the better times, and
> a pure rename should not be risky from a code standpoint.
>
> At the same time, doing it during the merge window isn't *wrong*
> either. Despite the somewhat painful merge with folio changes, I
> don't think fs/cifs/ or fs/ksmbd/ normally have a lot of conflicts,
> and git does handle rename conflicts fairly well unless there's just
> lots of complexity.
>
> So it's really fine either way. The normal kind of "big changes"
> should obviously always be merge window things, but pure renames
> really are different and are often done outside of the merge window
> (the same way I intentionally did the MAINTAINERS re-ordering just
> *after* the merge window)
>
> But we don't do renames often enough to have any kind of strict rules
> about things like this.
>
> So I think "whenever is most convenient for you" is the thing to aim
> for here. This is *not* a "only during merge window" kind of thing.
>
> Linus

Here are two patches:
1) Move CIFS/SMB3 related client and server files (cifs.ko and ksmbd.ko
and helper modules) to new fs/smb subdirectory (fs/smbfs was not used
to avoid confusion with the directory of the same name removed in 2.6.27
release and we also avoid using CONFIG_SMB_FS for the same reason)

fs/cifs --> fs/smb/client
fs/ksmbd --> fs/smb/server
fs/smbfs_common --> fs/smb/common

2) With the fs/cifs directory moved to fs/smb/client, correct mentions
of this in Documentation and comments.

Follow on patch can change Documentation/filesystems/cifs -->
Documentation/filesystems/smb (since it contains both server
and client documentation)


--
Thanks,

Steve


Attachments:
0001-smb-move-client-and-server-files-to-common-directory.patch (42.37 kB)
0002-cifs-correct-references-in-Documentation-to-old-fs-c.patch (5.50 kB)
Download all attachments

2023-05-22 08:23:08

by ronnie sahlberg

[permalink] [raw]
Subject: Re: [GIT PULL] ksmbd server fixes

Sounds like a good plan.

On Mon, 22 May 2023 at 14:39, Steve French via samba-technical
<[email protected]> wrote:
>
> On Sun, May 21, 2023 at 2:21 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Sun, May 21, 2023 at 12:03 PM Steve French <[email protected]> wrote:
> > >
> > > I would be happy to do the move (to fs/smb) of the directories and
> > > update the config soon (seems reasonably low risk) - let me know if you
> > > want me to send it this week or wait till 6.5-rc
> >
> > So I think the "do it now or wait until the 6.5 merge window" is
> > entirely up to you.
> >
> > We've often intentionally done big renames during the "quiet time"
> > after the merge window is oven, just because doing them during the
> > merge window can be somewhat painful with unnecessary conflicts.
> >
> > I would *not* want to do it during the last week of the release, just
> > in case there are small details that need to be fixed up, but doing it
> > now during the rc3/rc4 kind of timeframe is not only fairly quiet, but
> > also gives us time to find any surprises.
> >
> > So in that sense, doing it now is likely one of the better times, and
> > a pure rename should not be risky from a code standpoint.
> >
> > At the same time, doing it during the merge window isn't *wrong*
> > either. Despite the somewhat painful merge with folio changes, I
> > don't think fs/cifs/ or fs/ksmbd/ normally have a lot of conflicts,
> > and git does handle rename conflicts fairly well unless there's just
> > lots of complexity.
> >
> > So it's really fine either way. The normal kind of "big changes"
> > should obviously always be merge window things, but pure renames
> > really are different and are often done outside of the merge window
> > (the same way I intentionally did the MAINTAINERS re-ordering just
> > *after* the merge window)
> >
> > But we don't do renames often enough to have any kind of strict rules
> > about things like this.
> >
> > So I think "whenever is most convenient for you" is the thing to aim
> > for here. This is *not* a "only during merge window" kind of thing.
> >
> > Linus
>
> Here are two patches:
> 1) Move CIFS/SMB3 related client and server files (cifs.ko and ksmbd.ko
> and helper modules) to new fs/smb subdirectory (fs/smbfs was not used
> to avoid confusion with the directory of the same name removed in 2.6.27
> release and we also avoid using CONFIG_SMB_FS for the same reason)
>
> fs/cifs --> fs/smb/client
> fs/ksmbd --> fs/smb/server
> fs/smbfs_common --> fs/smb/common
>
> 2) With the fs/cifs directory moved to fs/smb/client, correct mentions
> of this in Documentation and comments.
>
> Follow on patch can change Documentation/filesystems/cifs -->
> Documentation/filesystems/smb (since it contains both server
> and client documentation)
>
>
> --
> Thanks,
>
> Steve