mkdir-dup test flapping

Andrew Bartlett abartlet at
Thu Mar 10 01:34:36 UTC 2016

On Wed, 2016-03-09 at 09:56 -0800, Jeremy Allison wrote:
> On Tue, Mar 08, 2016 at 08:54:32PM -0800, Jeremy Allison wrote:
> > 
> > On Wed, Mar 09, 2016 at 04:11:05PM +1300, Douglas Bagnall wrote:
> > > 
> > > We looked at this some more, and Andrew seemed to understand and
> > > wrote
> > > the attached patch.
> > > 
> > > We need to decide if the change in owner is enough reason to
> > > refuse
> > > the open(), or if we should just continue.
> > > 
> > > On the basis that the owner change is not significant, we
> > > reworked the
> > > code to do the lstat() after the fstat(), and not compare the
> > > owner.
> > Fix is really close, but still needs a little work -
> > the call to:
> > 
> > fsp->file_id = vfs_file_id_from_sbuf(conn, &smb_dname->st);
> > 
> > needs to be moved too (as in the race condition
> > when need_lstat is true smb_dname->st isn't correctly
> > initialized yet), or the lstat needs to be done both
> > before and after. Doing the lstat twice might be
> > safer as I'll need to check what uses fsp->file_id
> > to ensure moving to after the lstat is safe.
> > 
> > *Really* nice work though - thanks ! It's late
> > here now so I'll take a look at this first thing
> > in thr morning (US Pacific time) and post an
> > updated patch for you to look at tomorrow.
> OK - here it is. I looked over this very carefully
> and this is what I came up with.
> There's no security reason for re-doing the
> lstat after the vfs_fsp_stat() call as if the
> owner and SD were changed, any operation on
> the directory is still protected by the new
> owner/ACL that was placed upon the directory.
> Anyone with permission to do those changes
> can do them whether we have the directory
> open or not.
> For paranoia's sake I did add an extra ISDIR
> check once we (should) have a file handle to
> make sure the pathname the fsp is pointing to
> is a directory when exiting the function
> (or course, as with all races there's
> nothing to stop a properly credentialed
> used renaming/replacing that pathname
> later, but that's what a fileserver is
> designed to do :-).
> I created bug
> to track this and get it back-ported to
> release brances.
> 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

A summary with detailed information can be found in:

commit 68708672142f00dd59f91589d57fb3b3a593ab80
Author: Jeremy Allison <jra at>
Date:   Wed Mar 9 09:43:09 2016 -0800

    s3: smbd: Fix error in mkdir race condition detection.
    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
    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 samba.o
    and Douglas Bagnall <douglas.bagnall at>.

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...

Andrew Bartlett
Andrew Bartlett
Authentication Developer, Samba Team
Samba Development and Support, Catalyst IT

More information about the samba-technical mailing list