Questions about smbd option "strict rename"

[Ralph Boehme]

Hi Jeremy,

On Fri, Nov 20, 2015 at 01:45:08PM -0800, Jeremy Allison wrote:
> On Fri, Nov 20, 2015 at 10:01:46AM +0100, Ralph Boehme wrote:
> > ...
> > According to the commit message and the manpage description, "strict
> > rename = no" means we won't check for open files when we do a
> > directory rename.
> > 
> > If this is correct, we have two bugs:
> 
> Just to clarify, you mean: "we have two bugs in our rename path",
> yes ?

yes. I wasn't suggesting we're down to 0 bugs besided those. :)

> > - "strict rename = no": doesn't work, opens are always checked,
> >   regardless of the setting of "strict rename".  can_rename(), the
> >   function where we do this check when renaming a directory, is
> >   missing a check for lp_strict_rename() or similar.
> 
> Yep. That's how it was supposed to work. The code in
> can_rename() should probably be the same as the code in
> source/smbd/dir.c which is:
> 
>         if (!lp_posix_pathnames() &&
>             lp_strict_rename(SNUM(conn)) &&
>             have_file_open_below(fsp->conn, fsp->fsp_name))
>         {
>                 return NT_STATUS_ACCESS_DENIED;
>         }

Ok, thanks for clarifying. Does everybody agree? Metze?

> > - "strict rename = yes": the function called to do the actual check
> >   for open files is file_find_subpath() which only checks for opens in
> >   that process and doesn't traverse locking.tdb
> 
> Yep - that was older code and I remember deciding
> (with Volker I think) that per-process checks were
> probably 'good enough'.
> 
> > In private conversation metze suggested that "strict rename" is
> > supposed to have completely different semantics:
> > 
> > - "strict rename = no": check for opens, but only in the current
> >   process, avoids expensive locking.tdb traversal, deny rename if open
> >   found
> 
> Hmmmm. We also need to have 'lp_posix_pathnames()' in the
> mix too I'm afraid.

Imo we need a different check for this and don't hang this off
lp_posix_pathnames(). POSIX pathnames capability is one thing, POSIX
rename is imo a different one, I'd argue in SMB2/3 UNIX extensions
we'll want to define two distinct capabilities anyway.

> > - "strict rename = yes": check for opens by traversing locking.tdb,
> >   deny rename if open found
> 
> That's the have_file_open_below() semantic I think...

Yes.

> > Either way, I can provide fixes, but first we should agree upon the
> > semantics of "strict rename". :)
> > 
> > I am currently pursuing this case, because not allowing directory
> > renames with open files is a major pita for OS X users. OS X expects
> > POSIX rename semantics and that's what OS X SMB server implements. I
> > know this contradicts MS-SMB2/FSA.
> 
> How does their client ever work against Windows ? Windows
> does strict renames.

Same pita as Samba. :) It works against their own SMB server. OS X
being a UNIX system expects POSIX rename behaviour from filesystems.

Adding Steve to the loop, wonder whether he gets similar reports for
the Linux client.

> > "strict rename = no" with the semantics described in the manpage,
> > would give OS X users an easy workaround. An alternative would be
> > something like my patch attached to the bugreport that tracks this
> > problem for OS X users:
> 
> Hate that patch, sorry :-).

Now that's an argument! :) If "strict rename = no" does the same
trick, I'm fine. Otherwise I'll have to put some more love to the
patch until you're happy too. ;)

> > <https://bugzilla.samba.org/show_bug.cgi?id=11065>
> > <https://attachments.samba.org/attachment.cgi?id=10648>
> 
> How about we standardize on the:
> 
>         if (!lp_posix_pathnames() &&
>             lp_strict_rename(SNUM(conn)) &&
>             have_file_open_below(fsp->conn, fsp->fsp_name))
>         {
>                 return NT_STATUS_ACCESS_DENIED;
>         }
> 
> behavior which does the traverse, and ignore
> the half-assed per-process check (which means
> we can remove the function file_find_subpath()) ?

That's what I would have done besides lp_posix_pathnames(). strict
rename could be reused for SMB2 UNIX extensions POSIX rename
behaviour, so I'd suggest only checking strict rename:

         if (lp_strict_rename(SNUM(conn)) &&
             have_file_open_below(fsp->conn, fsp->fsp_name))
         {
                 return NT_STATUS_ACCESS_DENIED;
         }
	 
and possibly force strict rename to no for CIFS UNIX extensions in
case CIFS_UNIX_POSIX_PATHNAMES_CAP is enabled:

--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -3976,6 +3976,7 @@ static void call_trans2setfsinfo(connection_struct *conn,
	if (xconn->smb1.unix_info.client_cap_low & CIFS_UNIX_POSIX_PATHNAMES_CAP) {
		lp_set_posix_pathnames();
+		lp_do_parameter(SNUM(conn), "strict rename", "no");
		mangle_change_to_posix();
	}

However, I'd question whether CIFS_UNIX_POSIX_PATHNAMES_CAP implies
POSIX rename behaviour.

Thanks!
-Ralph

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de