5917 User-mode SMB server

Review Request #51 — Created May 11, 2015 and submitted


5917 User-mode SMB server
See the issue

It's also available as a webrev:

Has been used in production for more than a year.
smbtorture, MS CIFS test suite

  • 0
  • 0
  • 40
  • 15
  • 55
Description From Last Updated
  1. You

  2. You're right this is a hack, you should do it some other way. Note that we define things in Makefile.master such that you can easily prepend (in theory)

    1. Unfortunately, adding these to CPPFLAGS.master does not work.
      In these (special) cases, we must have the "fake" includes
      before the ones from ENVCPPFLAGS etc in CPPFLAGS.master.

      I guess I can add a CPPFLAGS.first that really allows prepend.

    2. CPPFLAGS = -I../foo $(CPPFLAGS.master) doesn't work? If it doesn't, I'm not sure how a new variable would.

    3. See the comment in the updated diff. Need to add -I... before ENVCPPFLAGS*
      and CPPFLAGS.master doesn't help with that.

    4. What I was telling you to do here is exactly what you did, just without the extra variable... :)

  3. same as the other "hack hack"

    1. Ditto previous: CPPFLAGS.master does not work...

  4. usr/src/cmd/smbsrv/bind-helper/Makefile (Diff revision 1)

    Remove, don't comment out

  5. usr/src/cmd/smbsrv/fksmbd/.dbxrc (Diff revision 1)

    Shipping this seems somewhat ok, but you should remove the parts that are user-preferential.

  6. usr/src/cmd/smbsrv/fksmbd/.dbxrc (Diff revision 1)

    If CODEMGR_WS is set, ROOT should already be set too. What you're doing here would break MULTI_PROTO=yes

  7. usr/src/cmd/smbsrv/fksmbd/.dbxrc (Diff revision 1)

    Consider setting the _32 and _64 variants, explicitly and correctly.

    1. The user-level SMB stuff is 32-bit only, so I'll skip this for now.

  8. usr/src/cmd/smbsrv/fksmbd/Makefile (Diff revision 1)

    yet another 'hack hack'

  9. usr/src/cmd/smbsrv/fksmbd/Run.sh (Diff revision 1)

    bldenv or build env or build environment

  10. usr/src/cmd/smbsrv/fksmbd/Run.sh (Diff revision 1)

    Same comments here as for dbxrc

  11. usr/src/cmd/smbsrv/fksmbd/Run.sh (Diff revision 1)

    It'd be even more careful to make yourself a temp directory under there.

  12. Either do it or decide not to and remove the XXX

  13. usr/src/cmd/smbsrv/fksmbd/fksmbd_shr.c (Diff revision 1)

    Is hardcoding this really ok?

    1. Added a better comment. OK?

  14. usr/src/cmd/smbsrv/smbd/server.xml (Diff revision 1)

    Seems like part of a separate bug/feature?

    1. OK, I split out all the "netbios_enable" changes. See:

  15. usr/src/cmd/smbsrv/smbd/smbd_doorsvc.c (Diff revision 1)

    Why is this not conditional on the fd any longer?

    1. We want to do the fdetach even if we already closed the fd.

  16. usr/src/cmd/smbsrv/smbd/smbd_doorsvc.c (Diff revision 1)

    Well, can we give it a better name then? because holy shit.

  17. usr/src/cmd/smbsrv/smbd/smbd_main.c (Diff revision 1)

    "FAKE" seems a poor name for this, but I'm not immediately coming up with anything better.

    1. OK, so... I'm not sure what to do to resolve this...

    2. Would you like this better?
      #ifdef FKSMBD

    3. I changed it as shown.

  18. usr/src/cmd/smbsrv/smbd/smbd_main.c (Diff revision 1)

    Again, seems part of a separate bug/feature

  19. usr/src/cmd/smbsrv/smbd/smbd_main.c (Diff revision 1)

    cstyle, tabs and spaces muddled

  20. usr/src/cmd/smbsrv/smbd/smbd_main.c (Diff revision 1)

    Not sure what "Later" is referring to here. Is this describing how the code behaves, or is it a note to yourself?

    If the latter, remove it, or do what it suggests

  21. usr/src/cmd/smbsrv/smbd/smbd_spool.c (Diff revision 1)

    More intermixed tabs and spaces, might as well fix it too

  22. usr/src/lib/libfakekernel/Makefile.com (Diff revision 1)

    (I'm aware taht both work, but we use parens pretty much exclusively)

  23. usr/src/lib/libfakekernel/Makefile.com (Diff revision 1)

    Remove, don't comment out

  24. This seems like something that'll bite you in the ass, and should probably be done now

    1. Clarified in the comment why thats's OK.

  25. If you're trying to describe why this isn't here, write it out as a more full comment

  26. Comment isn't really necessary.

    1. I thought it helpful... It's rare to bypass this shim that way.

    2. You OK with it staying? or should I remove it?

  27. Another thing that seems to be a separate bug/feature (at least it's all the same separate bug/feature)

  28. That same new bug/feature again

  29. Studio historically doesn't like trailing commas here. I can't remember whether that's a c99 thing or in general

  30. mixed tabs and spaces

  31. New feature again

  32. This is an... unusual way to go about this. I presume that's what the old comment is suggesting.

    Perhaps leave the mention that you'd prefer a real USDT provider?

    1. Is that your preference? What's there works fine for me.

  33. New feature again

  34. Why is this change in this wad?

    1. There were changes to signing as a result of needing to do both a kernel-mode and user-mode signing check function.

  35. usr/src/uts/common/fs/smbsrv/smb_fem.c (Diff revision 1)

    The old condition seemed more legible than breaking the ternary over 3 lines

  36. usr/src/uts/common/fs/smbsrv/smb_fem.c (Diff revision 1)

    3 line ternary comment again

  37. This doesn't seem entirely related to this change either?

    1. These chagnes were necessary to allow testing change notify in fksmbd.
      There are no FEM callbacks in fksmbd, so without these changes, the
      change notification features would not work.

      Even without that these changes help with supportability by
      avoiding the complex situation where smbsrv calls some VFS
      function which (via FEM) calls back into smbsrv. By having
      smbsrv handle those cases itself, we can avoid that mess.

  38. If this file is a copy or something from elsewhere, is there a way to make the diffs show that? It's unreviewable otherwise

    1. This code used to be in smb_kutil.c (same dir)
      I'm not sure how to show that as a diff.
      It's a straight copy/paste.

  39. You made the CIFS server support zones already, so presumably "This needs to be fixed"

    1. I believe all the chagnes like this were because something wouldn't build for us in libfksmbsrv.
      Sorry I don't remember more details.

    2. There were quite a few places where we tried to minimize includes,
      and somtimes that meant an anonymous struct is in view but not the
      typedef name. I think that's what was going on here.

    3. I do see that the anonymous struct thing does not apply here,
      so if you'd like me to revert that bit, let me know.

  40. usr/src/uts/common/fs/smbsrv/smb_net.c (Diff revision 1)

    I know you didn't do this, I just want to point out how horrific it is, and that you should do that already.

    1. This file will get attention soon. Let's ignore that for now.

  41. Don't XXX comment, either fix it or remove the comment.

    1. This goes away in later commits anyway.

  42. Don't XXX, either do what needs to be done or remove the comment if it's wrong.

  43. That same other bug/feature again

  44. Don't XXX, either fix it or remove the comment if it doesn't need fixing

  45. Do we really accept that spelling?

    1. That spelling appears in _NOTE() things...

  46. usr/src/uts/common/fs/smbsrv/smb_tree.c (Diff revision 1)

    Seems like an unrelated, and potentially very important, fix.

    1. These were just warning fixes. While making this build for user-mode, we (finally) went through and cleaned up warnings.

    2. No security impact?

    3. Nope. See the alloc + size computation preceeding this.

  47. usr/src/uts/common/fs/smbsrv/smb_vops.c (Diff revision 1)

    again, seems unrelated and perhaps important

    1. Ditto (just warning fixes)

  48. usr/src/uts/common/fs/smbsrv/smb_vops.c (Diff revision 1)

    again, seems unrelated and perhaps important

    1. Ditto (just warning fixes)

  49. usr/src/uts/common/fs/smbsrv/smb_vops.c (Diff revision 1)

    and again

    1. Ditto (just warning fixes)

  50. usr/src/uts/common/smbsrv/smb_ioctl.h (Diff revision 1)

    follow the style of the rest of the file, with tabs between type and member

  51. usr/src/uts/common/smbsrv/smb_kproto.h (Diff revision 1)

    This seems messy.

  52. usr/src/uts/common/smbsrv/smb_ktypes.h (Diff revision 1)

    This is changing in size, is it really part of this bug?

    1. It's not really changing the size, but just coming up with a place to stash the session key, in a way that works for both kernel/user code (IIRC).
      This too gets extensive changes later, when "extended security" comes. I'd rather leave it alone to avoid conflicts later.

    1. I believe all the chagnes like this were because something wouldn't build for us in libfksmbsrv.
      Sorry I don't remember more details. OK?

  53. usr/src/uts/intel/smbsrv/Makefile (Diff revision 1)

    Seems like a separate bug

    1. No, this was part of making the signing code work both kernel/user mode.

  54. usr/src/uts/sparc/smbsrv/Makefile (Diff revision 1)

    seems like a separate bug

    1. No, this was part of making the signing code work both kernel/user mode.

  55. You already know I'm pretty skeptical about how invasive this is, for what it brings (which I'm sure is valuable to you). One of the things that would show it's general utility is making libzpool use it, but you've decided not to do that. Have you looked into whether it would be hard, and whether the work you've done would be sufficient for it?

    How maintainable is this scheme over the long term, how likely are we to run into things that cannot be faked?

    I'm not against the concept (which surprises me, I thought I was), but going about it in such a currently smb-specific way worries me as far as its actual applicability, and the way it reaches all over the headers to get what it needs (and only what it needs).

    The general use of the "FAKE" name is somewhat worrysome, in that it doesn't really tell you much about what it is if you're new to the system, and given it's now sprinkled all over the headers, it seems like a more descriptive name would be for the best.

    I may have other thoughts to come too. I skipped out on a lot of the specifics of the library implementation.

    It'd be good to get reviewers with architectural experience, given how far reaching you are. Consider pinging people like rmustacc, eschrock (or whoever is a good alternative for eschrock) etc.

  2. usr/src/cmd/mdb/Makefile.common (Diff revision 3)

    RB thinks there's trailing whitespace here, fwiw.

  1. Ship It!
Review request changed

Status: Closed (submitted)