Questions about smbd option "strict rename"

Tue Nov 24 17:50:47 UTC 2015

On Tue, Nov 24, 2015 at 12:03:37PM +0100, Michael Adam wrote:
> On 2015-11-23 at 14:16 -0800, Jeremy Allison wrote:
> > On Mon, Nov 23, 2015 at 01:33:44PM -0800, Jeremy Allison wrote:
> > And here is the patch that implements this interpretation of "strict rename"
> > and updates the docs.

+1

> > 
> > 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 !
> 
> See comments inline below.

+1 for Michael's doc enhancements:

> s/renames/renamed/
> 
> Should we really mention the 'open file database' which is an
> implementation detail? Alternative:
> 
> ....below a directory being renamed, instead of checking for open
> files across all smbd processes.
> 
> or similar...
> 
> > +    <para>Because of the expense in fully searching the database,
> 
> similar here: Because of the expense in the full search, ...

updated patchset with my reviewed-by attached. Feel free to push.

Jeremy, how to proceed wrt that attaching POSIX rename behaviour to
POSIX pathnames is wrong imo. We need a seperate flag for this.

-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
-------------- next part --------------
From db74f0ec4fb25d88102cc738e223b4e50a8ffcb1 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>
Reviewed-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Michael Adam <obnox 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 3f99f88..50a0bd1 100644
--- a/source3/smbd/dir.c
+++ b/source3/smbd/dir.c
@@ -1961,7 +1961,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.5.0


From 5a647be651b4dcfe488346045ef906e7b9af22fc 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>
Reviewed-by: Ralph Boehme <slow at samba.org>
Reviewed-by: Michael Adam <obnox at samba.org>
---
 docs-xml/smbdotconf/tuning/strictrename.xml | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/docs-xml/smbdotconf/tuning/strictrename.xml b/docs-xml/smbdotconf/tuning/strictrename.xml
index 5478863..dacb879 100644
--- a/docs-xml/smbdotconf/tuning/strictrename.xml
+++ b/docs-xml/smbdotconf/tuning/strictrename.xml
@@ -15,10 +15,18 @@
     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
+    renamed, instead of checking for open files across all smbd
+    processes.</para>
 
+    <para>Because of the expense in the full search, 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.5.0

