[SCM] Samba Shared Repository - branch master updated

[Jeremy Allison]

On Tue, Apr 27, 2010 at 09:18:54AM +0200, Stefan (metze) Metzmacher wrote:
> 
> I'm not sure this is correct.
> 
> I haven't looked in detail, but this looks like just hiding the real
> problem.
> 
> The real problem is likely that we're abusing the tevent_req guidelines.
> 
> I think 8f67f873ace91964da066c421986e260aceba75b is maybe ok, for
> getting stuff working, but I'd like to see the design changed.
> 
> smb2_deferred_open_timer() should not call smbd_smb2_request_dispatch().
> 
> The re-entrant should happen inside the smbd_smb2_create_* code,
> the place were it decides to go async, instead of two layers above.
> 
> I think the smbd_smb2_create_* should setup a
> smb2req->retry_callback(struct tevent_req *) function pointer.
> smb2_deferred_open_timer() would then just call it should just call it.
> 
> I'd like to have something similar for smb1 (I know it would be a lot of
> work), but the layer violation is really confusing.
> 
> The top level smb1/2 server code should not see any of this retry logic,
> it should just do a foo_send() call set it's callback
> on the returned tevent_req and get the final result with foo_recv().
> All magic should be in one spot in the lower level.

I disagree with this. Mainly because it will lead to a lot of
code duplication, in potentially security-sensitive paths.

The great thing about the way I have it working now, is that
it works identically to the SMB1 code paths in terms of reentrancy,
and allows us to re-use all the parameter parsing and setting of
the security context that we naturally have to have to make
non-reentrant code work.

If we break the smbd_smb2_request_dispatch() path we will see
the problem immediately. If we add specialized retry logic to
individual reentrant code paths it will be a source of logic
and possibly security bugs for some time to come.

The method I'm using, as it's identical to the SMB1 logic,
also allows me to hook in the backends in exactly the same
way that SMB1 uses them.

I'm planning to finish implementing blocking locks (the
remaining reentrant call) in the same way.

Once this gets done we'll have a working system, and if you
want to look at refactoring at that point then I'd be happy
to help.

But I want to get to a fully featured system quickly, and
that currently means re-implementing the SMB1 logic. Remember,
Volker also understands how this works, so that's at least
two people who can fix this :-).

Jeremy.