[RFC] Simplify async path based VFS functions: remove tevent and pthreadpool_tevent wrappers

[Jeremy Allison]

On Wed, Dec 19, 2018 at 05:00:20PM +0100, Stefan Metzmacher wrote:
> Am 19.12.18 um 14:02 schrieb Ralph Böhme:
> > Hi folks,
> > 
> > as you may or may not know, since a few months Samba has a fancy new
> > machinery
> > that was added to properly support path based functions like getxattr()
> > in the
> > Samba VFS.
> > 
> > The machinery consists of:
> > 
> > 1. tevent wrapper
> > 
> > A tevent wrapper associates an existing tevent context with a bunch of
> > function
> > pointers that get called before and after the tevent event handlers.
> > 
> > This is useful to hide the complexity of becoming the correct user at
> > the right
> > time, especially in callbacks from tevent.
> 
> Here's a little bit of background why I have chosen that strategy
> (actually I started with it in 2012 already).
> 
> Until now it was the job of the core code in smbd to take care
> of any impersonation for the currently processed request.
> 
> We have change_to_user() and set_current_service() in the central
> protocol processing in order to support multiple sessions on
> an SMB connection.
> 
> In order to support oplock related async delays, we use an
> infrastructure to put requests from the SMB_VFS layer
> onto a deferred queue, while we're waiting for an internal notification
> message to arrive.
> 
> Once we're notified that there's a change to get any further,
> the deferred request is replayed internally, which means we completely
> redo the protocol processing and the resulting impersonation
> before calling into the SMB_VFS layer again.
> 
> This makes it relatively hard for SMB_VFS modules to keep state
> of the already started request, while the module don't have to
> take care about impersonation.
> 
> For file handle based calls like pread, pwrite, fsync, offload_read and
> offload_write we already added SMB_VFS_*_SEND/RECV hooks to make use of
> our typical tevent_req based async request handling, which makes it easy
> to have a per request state. But there's no impersonation enforcement at
> all, because the tevent event handlers don't restore the impersonation
> of the current request. And we may change back to root or any random
> other user attached to a different SMB session on the same connection.
> 
> All implementations of SMB_VFS_*_SEND/RECV within upstream Samba
> releases (<= 4.9.x) should not need impersonation for these calls as
> they all operate on an already existing file descriptor. But I already
> saw SMB VFS modules of OEMs, which may rely on impersonation being
> correct during the whole lifetime of an async request.
> 
> In order to make it possible to add path based SMB_VFS_*_SEND/RECV
> we need a solution that makes it very hard for module writers to get the
> impersonation of async requests wrong and introduce security problems.
> 
> With the existing strategy (that the core smbd handles all
> impersonation) in mind I implemented impersonation wrappers
> using tevent context wrappers. And the SMB_VFS_*_SEND() function
> would then only get the wrapper context and all impersonation is
> done for free.
> 
> > 2. pthreadpool_tevent wrapper
> > 
> > pthreadpool_tevent wrapper hides the complexity of impersonating the
> > correct
> > user and changing the current working directory to the base directory of a
> > service from the worker thread.
> > 
> > 3. VFS glue
> > 
> > This is a structure and some functions that bring together user and root
> > pthreadpool_tevent and tevent wrappers, taking into account system
> > support for
> > per-thread credentials and per-thread current working directory support.
> > 
> > VFS implementations can use the VFS glue and the supporting functions
> > that act
> > on the glue in a boilerplate fashion to implement path based VFS functions,
> > including support for funning worker thread job functions as root and in
> > the
> > correct working directory.
> 
> In order to hide more complexity I also added pthreadpool_tevent wrapper
> and the SMB_VFS glue. These are very complex inside, but make it
> relatively easy for SMB_VFS modules.
> 
> > The longterm plan is to pass an additional argument to all syscall
> > wrapping VFS
> > functions with the required internal information to perform
> > impersonation and
> > chdir(), eg an "impersonation_context".
> > 
> > This, together with some helper functions that work on the object and
> > can be
> > used the implementation of VFS functions, should result in reasonable
> > complexity
> > both in the implementation and for the user of the new API.
> 
> We're moving more and more to file handle based functions,
> which is a result of the clean SMB2/3 protocol (and we try to
> get rid of SMB1). At the same time we also try to move syscalls
> to helper threads.
> 
> This means doing the impersonation in the main smbd process/thread
> will not be needed most of the time.
> 
> So we thought that removing the complexity of the tevent/pthread_tevent
> wrappers would make it easier to understand the core smbd code and
> make it more explicit where impersonation might be required.
> 
> While we'll pass some kind of explicit impersonation context through
> the SMB_VFS layers, we'll also provide helper functions, which make
> it easier for module writers to trigger the impersonation, by just
> writing little syscall wrappers like this:
> 
> static void vfswrap_getxattrat_do(void *private_data)
> {
>         struct vfswrap_getxattrat_state *state = talloc_get_type_abort(
>                 private_data, struct vfswrap_getxattrat_state);
>         int ret;
> 
>         /*
>          * Here we simulate a getxattrat()
>          * call using fchdir();getxattr()
>          */
> 
>         ret = fchdir(state->dirfd);
>         if (ret == -1) {
>                 state->xattr_size = -1;
>                 state->vfs_aio_state.error = errno;
>                 goto end_profile;
>         }
> 
>         state->xattr_size = getxattr(state->name,
>                                      state->xattr_name,
>                                      state->xattr_value,
>                                      state->xattr_bufsize);
> }
> 
> Helper functions will execute such functions in the desired
> impersonation state. Depending on the available OS features
> and configuration this can run in the main thread or within
> a helper thread.
> 
> Pure user space file system may skip impersonation completely.
> 
> I think this is a good long term goal, but it will take some
> time and might not be completed until we remove support for
> SMB1.

Thanks so much for the explainations Ralph and Metze.

I must confess I had concerns about the complexity of
the wrappers, as when the code went in I spent a lot of
time going through it, and finally understood it.

However, by the next week the memory of that understanding
had faded, and I certainly couldn't guarantee that I could
safely use this code without re-doing the initial study
session.

A case of EENGINEERTOOSTUPID I'm afraid :-).

+1 on the plans to reduce the overall complexity (whilst
increasing it in a temporary way in some call paths).

My feeling is that once 4.10 has split and gone out
of the door we should finish the SMB3+ UNIX extensions
and then move aggressively to remove the SMB1 server
code (keep the client code around at least for a while).

This will make our solution much cleaner and maintainable
in the long term.

The days of SMB1 have already come to an end, and we need
to remove the possibilty of any more bugs in it from Samba.

All IMHO of course.

Cheers,

	Jeremy.