Re: (ITS#6104) race condition with cancel operation

[h.b.furuseth@usit.uio.no]

Howard Chu writes:
> I'd prefer to condense it all into a bitfield.

Right... keep both o_abandon and o_cancel (maybe renamed to o_dummy) for
RE24 though, otherwise we have a silent binary incompatible change with
already-compiled slapd modules.  The change will in any case break
modules that play with o_cancel though.

> We already ensure that
> the c_mutex is held when setting these flags.

Not yet, but c_mutex it is:-)  Must protect the following code.
Or rather, bitflag code which will replace it:

- o_cancel access in cancel.c, connection.c:connection_operation(),
  back-relay/op.c.

- o_abandon setting in overlays/syncprov.c:syncprov_op_abandon(), and
  in overlays/retcode.c where hopefully the first case should have been
	if ( op2.o_abandon ) op->o_abandon = 1;
  so that we can never reset o_abandon once it is set.

> It would be a lot of
> overhead to grab the mutex just to read the flag,
> and would require additional analysis to make sure the new locking
> behavior doesn't introduce new deadlocks.

Checking o_abandon!=0 works unmutexed.  (Well, technically any
such unmutexed access is wrong, but in practice we should be OK.)
As long as we:
- never reset o_abandon like retcode above can do, and
- never unmutexed assume consistency betweeen abandon-related variables
  set by different threads.

So the equivalent of this should be OK:
  if ( op->o_abandon ) rs->sr_err = SLAPD_ABANDON;
  ...  
  if ( rs->sr_err == SLAPD_ABANDON ) {
     lock c_mutex;
     more careful o_abandon checks;
     unlock c_mutex;
  }

  
While I'm looking at this:

The ldap_pvt_thread_yield() in cancel.c is really ugly.  There is no
cond to attach to it to without breaking binary compatibility, but even
a even a global mutex+cond might be better, with threads broadcasting on
it when it might be relevant.

It's still ugly though.  Cancelling an expensive operation ought to be a
friendly operation, but instead it is unfriendly in that yet another
thread is occupied, solely to wait for that operation.  What would make
sense (for RE25) would be to redo Cancel so that the cancelled thread is
responsible for sending the Cancel response or resubmitting an operation
which will.

Also, the be_cancel implementation in at least back-relay looks wrong.
Cancel isn't dispatched on DN, and back-relay with no 'relay' directive
configured cannot know where to dispatch.  I haven't checked how it
works, but it seems a call to be_cancel means the Cancel operation
_might_ belong to this backend.  But it might not, which means
back-relay shouldn't set op->o_cancel in this case.  What it should do
I don't know though.  And I don't know if syncprov is doing any better.

-- 
Hallvard