Questions about smbd option "strict rename"

Tue Nov 24 11:03:37 UTC 2015

On 2015-11-23 at 14:16 -0800, Jeremy Allison wrote:
> 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 !

See comments inline below.

> Thanks,
> 
> 	Jeremy.

> 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

LGTM

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

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

?

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

LGTM apart from the comments above.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151124/9dd7f21b/signature.sig>