mkdir-dup test flapping

[Andrew Bartlett]

On Wed, 2016-03-09 at 17:49 -0800, Jeremy Allison wrote:
> On Thu, Mar 10, 2016 at 02:34:36PM +1300, Andrew Bartlett wrote:
> > 
> > > 
> > > Let me know what you think !
> > We are puzzled.  While it ran successfully 1000 times locally (in
> > the
> > same testenv environment), when run in make test on the Catalyst
> > Cloud,
> > with your patch it failed in 2/10 of the runs I started this
> > morning:
> > 
> > Testing SMB2 Create Directory with multiple connections
> > waiting for replies
> > UNEXPECTED(failure): samba3.smb2.create.mkdir-dup(nt4_dc)
> > REASON: Exception: Exception:
> > ../source4/torture/smb2/create.c:1628:
> > File 1 returned status NT_STATUS_ACCESS_DENIED
> > 
> > FAILED (1 failures, 0 errors and 0 unexpected successes in 0
> > testsuites)
> > 
> > A summary with detailed information can be found in:
> >   ./bin/ab/summary
> > 
> > commit 68708672142f00dd59f91589d57fb3b3a593ab80
> > Author: Jeremy Allison <jra at samba.org>
> > Date:   Wed Mar 9 09:43:09 2016 -0800
> > 
> >     s3: smbd: Fix error in mkdir race condition detection.
> >     
> >     BUG: https://bugzilla.samba.org/show_bug.cgi?id=11780
> >     
> >     The smb2.create.mkdir-dup test creates a race, and against an
> > AD DC
> >     this can cause a flapping test if the lstat() and stat() calls
> > are
> >     made either side of the chown() due to creation of a file by
> >     administrator.
> >     
> >     We know when we've hit the race, so relax the check_same_stat()
> >     check in that specific case.
> >     
> >     Fix based on an original patch by Andrew Bartlett <abartlet at sam
> > ba.o
> > rg>
> >     and Douglas Bagnall <douglas.bagnall at catalyst.net.nz>.
> > 
> > 
> > Sorry to drop this on you at the end of the work day.  
> > 
> > I've started a number of runs with our patch, but I don't expect a
> > different result.  We have this one on the line, but it's still
> > fighting as we reel it in...
> Can you get me a debug level 10 log showing which
> of the ACCESS_DENIED is triggering ?
> 
> Then I can try and concoct a scenario that might
> allow that to happen :-).

Isn't there still a race between whatever code first calls stat() and
fills in smb_dname->st and the fstat() in vfs_stat_fsp()?

The directory could have been crated by the time we enter this
function, but not be chowned() until just before vfs_stat_fsp().

Is there anything that checking the file ownership (rather than
checking the IS_DIR and dev/inode) is protecting?  Why do we stat()
this twice in any case?

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   
https://catalyst.net.nz/services/samba