mkdir-dup test flapping

[Andrew Bartlett]

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 https://bugzilla.samba.org/show_bug.cgi?id=11780
> 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
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 samba.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...

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