Questions about smbd option "strict rename"

[Jeremy Allison]

On Fri, Nov 20, 2015 at 10:01:46AM +0100, Ralph Boehme wrote:
> Hi!
> 
> I stumbled upon an inconsitency in "strict rename".
> 
> The following set of patches added this option and a test:
> 
> ---8<---
> commit 035fd7200d8a025cdb8bfae30c264757aa3cb193
> Author: Volker Lendecke <vl at samba.org>
> Date:   Thu Sep 25 01:30:33 2014 +0200
> 
>     s3:smbd: Don't rename a dir with files open underneath
> 
>     This is an EXPENSIVE check. We'll have to guard this with an option
> 
>     Signed-off-by: Volker Lendecke <vl at samba.org>
>     Reviewed-by: Jeremy Allison <jra at samba.org>
>     Reviewed-by: Stefan Metzmacher <metze at samba.org>
> 
> commit 5f60dcc38ca275aedeb1d67611b5acf9b26361d5
> Author: Jeremy Allison <jra at samba.org>
> Date:   Fri Oct 24 13:57:04 2014 -0700
> 
>     selftest:Samba3: use "strict rename = yes"
> 
>     Signed-off-by: Jeremy Allison <jra at samba.org>
>     Reviewed-by: Stefan Metzmacher <metze at samba.org>
> 
> commit b0a434386dc2f77df89811bc3f56c4cc7fb7b16c
> Author: Jeremy Allison <jra at samba.org>
> Date:   Fri Oct 24 13:57:04 2014 -0700
> 
>     s3:param: Add new option "strict rename".
> 
>     Control whether smbd can rename directories containing
>         open files. Defaults to "no" (meaning we *can* do
> 	    such renames).
> 
>     Signed-off-by: Jeremy Allison <jra at samba.org>
>     Reviewed-by: Stefan Metzmacher <metze at samba.org>
> ---8<---
> 
> 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 ?

> - "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;
        }


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

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

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

> "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 :-).

> <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()) ?

Does that work ? We promise in the 'strict
rename = no' docs we won't do open checks at all,
so we should probably stick to that.

Jeremy.