Questions about smbd option "strict rename"

Mon Nov 23 22:16:31 UTC 2015

On Mon, Nov 23, 2015 at 01:33:44PM -0800, Jeremy Allison wrote:
> On Mon, Nov 23, 2015 at 10:22:35PM +0100, Ralph Boehme wrote:
> > On Mon, Nov 23, 2015 at 12:59:35PM +0100, Stefan Metzmacher wrote:
> > > Am 22.11.2015 um 13:49 schrieb Ralph Boehme:
> > > > 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:
> > > >>> - "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?
> > > 
> > > I don't agree, sorry.
> > > 
> > > We should provide the semantics the client asked for
> > > and try to behave like a windows server.
> > > 
> > > I think we should fix the manpage and use file_find_subpath()
> > > if lp_strict_rename() is false.
> > 
> > I'm leaning towards that interpretation of "strict rename" too.
> 
> I have a patch in preparation that implements this. It's
> found an existing bug in files_below_forall() I'll probably
> log as a separate bug....

And here is the patch that implements this interpretation of "strict rename"
and updates the docs.

Note it depends on the previous patch for bug:

https://bugzilla.samba.org/show_bug.cgi?id=11615

Bug 11615 - have_file_open_below() fails to enumerate files open under a directory correctly.

in order to pass the tests and enumerate open
files correctly.

Please review !

Thanks,

	Jeremy.
-------------- next part --------------
From a7916f395305afb980fd5c15d2c60a89dab2ea1f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 23 Nov 2015 11:27:56 -0800
Subject: [PATCH 1/2] s3: smbd: Change semantics of strict rename to search the
 file open db.

Without strict rename just look in local process. POSIX renames are
already dealt with above.

Documentation change to follow.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=11065

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/smbd/dir.c   |  2 +-
 source3/smbd/proto.h |  2 ++
 source3/smbd/reply.c | 12 +++++++++++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
index cfc1635..4c77559 100644
--- a/source3/smbd/dir.c
+++ b/source3/smbd/dir.c
@@ -1963,7 +1963,7 @@ static int have_file_open_below_fn(struct file_id fid,
 	return 1;
 }
 
-static bool have_file_open_below(connection_struct *conn,
+bool have_file_open_below(connection_struct *conn,
 				 const struct smb_filename *name)
 {
 	struct have_file_open_below_state state = {};
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index be51182..55e8286 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -232,6 +232,8 @@ long TellDir(struct smb_Dir *dirp);
 bool SearchDir(struct smb_Dir *dirp, const char *name, long *poffset);
 NTSTATUS can_delete_directory(struct connection_struct *conn,
 				const char *dirname);
+bool have_file_open_below(connection_struct *conn,
+			const struct smb_filename *name);
 
 /* The following definitions come from smbd/dmapi.c  */
 
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index c437135..efef613 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -2676,7 +2676,17 @@ static NTSTATUS can_rename(connection_struct *conn, files_struct *fsp,
 		/* If no pathnames are open below this
 		   directory, allow the rename. */
 
-		if (file_find_subpath(fsp)) {
+		if (lp_strict_rename(SNUM(conn))) {
+			/*
+			 * Strict rename, check open file db.
+			 */
+			if (have_file_open_below(fsp->conn, fsp->fsp_name)) {
+				return NT_STATUS_ACCESS_DENIED;
+			}
+		} else if (file_find_subpath(fsp)) {
+			/*
+			 * No strict rename, just look in local process.
+			 */
 			return NT_STATUS_ACCESS_DENIED;
 		}
 		return NT_STATUS_OK;
-- 
2.6.0.rc2.230.g3dd15c0


From 25a6ca7c3218c449272dae23c34ae0df06ee939f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 23 Nov 2015 11:32:48 -0800
Subject: [PATCH 2/2] s3: docs: Fix "strict rename" doc to match code.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=11065

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 docs-xml/smbdotconf/tuning/strictrename.xml | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/docs-xml/smbdotconf/tuning/strictrename.xml b/docs-xml/smbdotconf/tuning/strictrename.xml
index 5478863..0fe3385 100644
--- a/docs-xml/smbdotconf/tuning/strictrename.xml
+++ b/docs-xml/smbdotconf/tuning/strictrename.xml
@@ -15,10 +15,17 @@
     Samba system the cost is even greater than the non-clustered
     case.</para>
 
-    <para>For this reason the default is "no", and it is recommended
-    to be left that way unless a specific Windows application requires
-    it to be changed.</para>
+    <para>When set to "no" smbd only checks the local process
+    the client is attached to for open files below a directory
+    being renames, and doesn't search the open file database.</para>
 
+    <para>Because of the expense in fully searching the database,
+    the default is "no", and it is recommended to be left that way
+    unless a specific Windows application requires it to be changed.</para>
+
+    <para>If the client has requested UNIX extensions (POSIX
+    pathnames) then renames are always allowed and this parameter
+    has no effect.</para>
 </description>
 
 <value type="default">no</value>
-- 
2.6.0.rc2.230.g3dd15c0

