[PATCH] Use conn->session_info->security_token in posix_acls.c to make sysvolreset faster (was: Re: [PATCH] improve performance for samba-tool ntacl sysvolreset)

Andrew Bartlett abartlet at samba.org
Wed Jul 11 07:21:15 UTC 2018

On Wed, 2018-07-11 at 09:22 +0300, Uri Simchoni wrote:
> On 07/11/2018 08:33 AM, Andrew Bartlett wrote:
> > On Tue, 2018-07-10 at 07:49 +0300, Uri Simchoni wrote:
> > > Hi,
> > > 
> > > The idea of using whatever information we already have instead of
> > > re-calculating it is correct IMHO.
> > > 
> > > What I like less is the idea of an incomplete session_info that we have
> > > to second-guess, as in 3/16 (checking for null unix_info). We may miss
> > > checks now or on in the future, and null-case-handling may be
> > > non-uniform. AFAICT we don't have checks like this in the code, which is
> > > (hopefully) a testament for this field always being populated when the
> > > session_info is built.
> > > 
> > > So I'm just asking - what would it take to construct a session info with
> > > unix_info on the Python side.
> > 
> > I was very hopeful, but sadly the security_token_to_unix_token() that I
> > wrapped via auth_session_info_fill_unix() fails if winbindd isn't
> > running.  
> > 
> > Winbindd can't start until after provision, but provision and the
> > domain restore is an important user of this API.  (Internally in smbd
> > the SID->ID handling falls back to passdb if winbindd isn't up, but
> > this is not available to the token manipulation APIs right now. 
> > 
> > I could try for this, catch the exception and cope without the token in
> > that case, but for now I would prefer to proceed with the patch as-was.
> > 
> > I've added the bindings and tests that operate no token and with and
> > without the unix part of the token.
> > 
> > Let me know if you are OK with the above, or would prefer no token (and
> > slower if winbind is not up) to the partial token.
> > 
> > CI: https://gitlab.com/catalyst-samba/samba/pipelines/25524636
> > 
> > Thanks,
> > 
> > Andrew Bartlett
> > 
> I'd prefer the original patch, because unless I'm missing something, the
> new functionality (session_info_fill_unix) isn't being used, and isn't
> tested (seems like considerable effort has been made but 19/20 seems to
> be some infrastructure for tests and I don't see the tests themselves).

Correct, I don't use it.   But having written it I figured it might be
useful in the future, say when an smbd refactor makes this a stricter

The testing in via python test class inheritance, so doesn't look so
grand in the diff, but is in there.  We don't explore the full
structure in the patch that introduces it, but we do check it is
created (ie, a smoke test).  

The API it wraps is the core of how smbd works in the AD DC however, so
it is well used at this point.

> As I said before - I don't like adding null possibility, but I don't
> have the time to implement what I've proposed, I don't want to be in the
> way of progress, and the proposed patch doesn't break anything, it just
> IMO "adds entropy" to the code where not strictly needed.

OK.  Thanks!

> To recap:
> Setting the owner of of the default ACL to be BA instead of LA, a
> one-line-two-character diff, is likely to fix the performance problem,
> and if not, then smbd can be modified to support this as we strive to
> support files being owned by BA.
> Another thing I proposed was some special rule to support LA using BA ACE.


Andrew Bartlett
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

More information about the samba-technical mailing list