Releases, locking and ldb

[Andrew Bartlett]

On Wed, 2017-06-28 at 00:05 +0200, Stefan Metzmacher wrote:
> Am 27.06.2017 um 11:46 schrieb Andrew Bartlett:
> > On Tue, 2017-06-27 at 08:24 +0200, Stefan Metzmacher via samba-
> > technical wrote:
> > > Am 27.06.2017 um 04:36 schrieb Andrew Bartlett via samba-technical:
> > > > On Tue, 2017-06-27 at 08:07 +1200, Andrew Bartlett via samba-technical
> > > > wrote:
> > > > > On Mon, 2017-06-26 at 18:11 +0200, Stefan Metzmacher wrote:
> > > > > > 
> > > > > > Would sssd break with these changes?
> > > > > > 
> > > > > > Do we have other known external users, beside openchange?
> > > > > 
> > > > > I'm not sure.  I'll check with apt reverse depends when I get to the
> > > > > office. 
> > > > 
> > > > For Debian:
> > > > 
> > > > libldb1
> > > >   Reverse Depends: ldb-tools (>= 2:1.1.27-1+b1)
> > > >   Reverse Depends: libldb-dev (= 2:1.1.27-1+b1)
> > > >   Reverse Depends: python-ldb (= 2:1.1.27-1+b1)
> > > >   Reverse Depends: python-samba (>= 2:4.5.8+dfsg-2)
> > > >   Reverse Depends: python-sss (>= 1.15.0-3)
> > > >   Reverse Depends: python3-sss (>= 1.15.0-3)
> > > >   Reverse Depends: samba (>= 2:4.5.8+dfsg-2)
> > > >   Reverse Depends: samba-dsdb-modules (>> 2:4.5.8+dfsg-2)
> > > >   Reverse Depends: samba-libs (>= 2:4.5.8+dfsg-2)
> > > >   Reverse Depends: samba-testsuite (>= 2:4.5.8+dfsg-2)
> > > >   Reverse Depends: sssd-ad-common (>= 1.15.0-3)
> > > >   Reverse Depends: sssd-common (>= 1.15.0-3)
> > > >   Reverse Depends: sssd-dbus (>= 1.15.0-3)
> > > >   Reverse Depends: sssd-proxy (>= 1.15.0-3)
> > > >   Reverse Depends: sssd-tools (>= 1.15.0-3)
> > > > 
> > > > That limits the fallout pretty well it seems.
> > > 
> > > The good thing is that due to LDB_MODULE_CHECK_VERSION()
> > > Samba and sssd are only runtime compatible with the version
> > > they're build against.
> > > 
> > > Would "ldb:tdb: Ensure we correctly decrement ltdb->read_lock_count"
> > > affect any old caller in any bad way?
> > 
> > I presume you mean some other hypothetical application beyond Samba and
> > SSSD?
> 
> I mean Samba and also SSSD.
> 
> > The primary change is that during the ldb callbacks, the DB is locked,
> > rather than unlocked.  It is no longer possible to start a transaction
> > (write operation) during an ldb callback.  Instead, as good practice
> > would already dictate, you must start the transaction before the
> > search. 
> 
> What I want to find out is, does this fix actually make the situation
> in Samba 4.6 or sssd actively worse than what we currently have there?
> Or is this fix just not enough to fix all the mess?

The "ldb:tdb: Ensure we correctly decrement ltdb->read_lock_count"
patch both fixes and creates issues.  If it was a monotonic
improvement, it would have landed already, but without the full DB
locking it just changes us from read corruption do deadlock-detected. 

I took on the full db locking not just as a further improvement, it was
 also required simply to make the patches pass autobuild.  I only got
autobuild to finally pass, or even have a strictly smaller set of
errors, when the full set was done.

> > > And what about "tdb: Remove locking from tdb_traverse_read()"?
> > 
> > I'm not sure about this one.  As we can't have one without the other I
> > don't know (and haven't studied) exactly what else it might impact. 
> > 
> > I don't think this without the ldb patches is actively harmful however.
> 
> Ok.
> 
> > > Whould something like this on the samba_dsdb module
> > > in 4.6 work around the problems (and keep it only as broken as it
> > > already is):
> > > - always call ldb_handle_use_global_event_context()
> > 
> > I don't think this case is worth worrying about.  It is rootdse
> > modified over LDAP in the single process model (only). 
> > 
> > > - overwrite read_lock/read_unlock() to be a no-op
> > 
> > No, this gets us to the point I was at during SambaXP, with random
> > LDB_ERR_BUSY / 'failed to upgrade hash locks', as we won't have a
> > global lock order.
> > 
> > I don't think it makes any difference to actually to have it pass the
> > read_lock()/read_unlock() down to sam.ldb.
> > 
> > I personally think we should just declare the incomparability, ideally
> > at build time, and move on.  (I tried to create an #ifdef hack to
> > detect this at build time, but wasn't able to build a reliable one
> > yet). 
> > 
> > I realise that everything that can happen will happen, but still: how
> > likely are they to be mixed in the real world?
> 
> Ok, I'm still trying to get every detail together in my head
> in order to judge for a useful solution.

Thank you for putting the time into this.

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