5917 User-mode SMB server

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

Information
gwr
illumos-gate
5917
Reviewers
general 
5917 User-mode SMB server

See the issue


It's also available as a webrev:

http://ma.nexenta.com/gwr/smb-usr1/
Has been used in production for more than a year.

smbtorture, MS CIFS test suite
  • 0
  • 0
  • 40
  • 15
  • 55
Description From Last Updated
richlowe
richlowe 
  1. You
  2. Show all issues 
    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. Show all issues 
    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) 
     
     
    Show all issues 
    Remove, don't comment out
  5. usr/src/cmd/smbsrv/fksmbd/.dbxrc (Diff revision 1) 
     
     
    Show all issues 
    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) 
     
     
    Show all issues 
    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) 
     
     
    Show all issues 
    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) 
     
     
    Show all issues 
    yet another 'hack hack'
  9. usr/src/cmd/smbsrv/fksmbd/Run.sh (Diff revision 1) 
     
     
    Show all issues 
    bldenv or build env or build environment
  10. usr/src/cmd/smbsrv/fksmbd/Run.sh (Diff revision 1) 
     
     
    Show all issues 
    Same comments here as for dbxrc
  11. usr/src/cmd/smbsrv/fksmbd/Run.sh (Diff revision 1) 
     
     
    Show all issues 
    It'd be even more careful to make yourself a temp directory under there.
  12. Show all issues 
    Either do it or decide not to and remove the XXX
  13. usr/src/cmd/smbsrv/fksmbd/fksmbd_shr.c (Diff revision 1) 
     
     
    Show all issues 
    Is hardcoding this really ok?
    1. Added a better comment. OK?
  14. usr/src/cmd/smbsrv/smbd/server.xml (Diff revision 1) 
     
     
    Show all issues 
    Seems like part of a separate bug/feature?
    1. OK, I split out all the "netbios_enable" changes. See:
      
https://www.illumos.org/issues/5921
      
https://www.illumos.org/rb/r/52/
  15. usr/src/cmd/smbsrv/smbd/smbd_doorsvc.c (Diff revision 1) 
     
     
    Show all issues 
    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) 
     
     
    Show all issues 
    Well, can we give it a better name then? because holy shit.
  17. usr/src/cmd/smbsrv/smbd/smbd_main.c (Diff revision 1) 
     
     
    Show all issues 
    "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) 
     
     
    Show all issues 
    Again, seems part of a separate bug/feature
  19. usr/src/cmd/smbsrv/smbd/smbd_main.c (Diff revision 1) 
     
     
    Show all issues 
    cstyle, tabs and spaces muddled
  20. usr/src/cmd/smbsrv/smbd/smbd_main.c (Diff revision 1) 
     
     
    Show all issues 
    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) 
     
     
    Show all issues 
    More intermixed tabs and spaces, might as well fix it too
  22. usr/src/lib/libfakekernel/Makefile.com (Diff revision 1) 
     
     
    Show all issues 
    $(NOT_RELEASE_BUILD) not ${NOT_RELEASE_BUILD} 
    
(I'm aware taht both work, but we use parens pretty much exclusively)
  23. usr/src/lib/libfakekernel/Makefile.com (Diff revision 1) 
     
     
    Show all issues 
    Remove, don't comment out
  24. Show all issues 
    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. Show all issues 
    If you're trying to describe why this isn't here, write it out as a more full comment
  26. Show all issues 
    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. Show all issues 
    Another thing that seems to be a separate bug/feature (at least it's all the same separate bug/feature)
  28. Show all issues 
    That same new bug/feature again
  29. Show all issues 
    Studio historically doesn't like trailing commas here.  I can't remember whether that's a c99 thing or in general
  30. Show all issues 
    mixed tabs and spaces
  31. Show all issues 
    New feature again
  32. Show all issues 
    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. Show all issues 
    New feature again
  34. Show all issues 
    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) 
     
     
    Show all issues 
    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) 
     
     
    Show all issues 
    3 line ternary comment again
  37. Show all issues 
    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. Show all issues 
    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. Show all issues 
    You made the CIFS server support zones already, so presumably "This needs to be fixed"
  40. Show all issues 
    Why?
    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.
  41. usr/src/uts/common/fs/smbsrv/smb_net.c (Diff revision 1) 
     
     
    Show all issues 
    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.
  42. Show all issues 
    Don't XXX comment, either fix it or remove the comment.
    1. This goes away in later commits anyway.
  43. Show all issues 
    Don't XXX, either do what needs to be done or remove the comment if it's wrong.
  44. Show all issues 
    That same other bug/feature again
  45. Show all issues 
    Don't XXX, either fix it or remove the comment if it doesn't need fixing
  46. Show all issues 
    Do we really accept that spelling?
    1. That spelling appears in _NOTE() things...
  47. usr/src/uts/common/fs/smbsrv/smb_tree.c (Diff revision 1) 
     
     
    Show all issues 
    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.
  48. usr/src/uts/common/fs/smbsrv/smb_vops.c (Diff revision 1) 
     
     
    Show all issues 
    again, seems unrelated and perhaps important
    1. Ditto (just warning fixes)
  49. usr/src/uts/common/fs/smbsrv/smb_vops.c (Diff revision 1) 
     
     
    Show all issues 
    again, seems unrelated and perhaps important
    1. Ditto (just warning fixes)
  50. usr/src/uts/common/fs/smbsrv/smb_vops.c (Diff revision 1) 
     
     
    Show all issues 
    and again
    1. Ditto (just warning fixes)
  51. usr/src/uts/common/smbsrv/smb_ioctl.h (Diff revision 1) 
     
     
    Show all issues 
    follow the style of the rest of the file, with tabs between type and member
  52. usr/src/uts/common/smbsrv/smb_kproto.h (Diff revision 1) 
     
     
    Show all issues 
    This seems messy.
  53. usr/src/uts/common/smbsrv/smb_ktypes.h (Diff revision 1) 
     
     
    Show all issues 
    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.
  54. usr/src/uts/common/smbsrv/smb_ktypes.h (Diff revision 1) 
     
     
    Show all issues 
    Why?
    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?
  55. usr/src/uts/intel/smbsrv/Makefile (Diff revision 1) 
     
     
    Show all issues 
    Seems like a separate bug
    1. No, this was part of making the signing code work both kernel/user mode.
  56. usr/src/uts/sparc/smbsrv/Makefile (Diff revision 1) 
     
     
    Show all issues 
    seems like a separate bug
    1. No, this was part of making the signing code work both kernel/user mode.
  57. 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.
gwr
gwr
gwr
gwr
trisk
trisk 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/mdb/Makefile.common
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    RB thinks there's trailing whitespace here, fwiw.
    
  
    
 
    

 
    
  



 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




gwr




   

   

  

 



 















 



 

  

   




gwr




   

   

  

 



 















 





 

  

   




richlowe




   

   







  

 



 

  


   

   

    
 
richlowe


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




gwr




   

   

  

 



 















 



 

  

   




gwr




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...