Bug 9733 - smbcontrol close-share is not working
Summary: smbcontrol close-share is not working
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-21 20:50 UTC by Jeremy Allison
Modified: 2013-04-08 06:52 UTC (History)
0 users

See Also:


Attachments
git-am fix for 3.6.next. (1.52 KB, patch)
2013-03-21 21:09 UTC, Jeremy Allison
ddiss: review-
Details
git-am fix for master and 4.0.next. (1.60 KB, patch)
2013-03-21 21:10 UTC, Jeremy Allison
ddiss: review+
Details
Modified git-am fix for 3.6.x (1.79 KB, patch)
2013-03-22 16:24 UTC, Jeremy Allison
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2013-03-21 20:50:57 UTC
Reported by an oem. If a Windows client has a share mounted (not it has to be a Windows client, for reasons explained below), and then the admin runs the command:

bin/smbcontrol <pid> close-share <share-name>

against the share, then smbd still holds a handle
to the pathname in the share definition, so it cannot
be unmounted.

I've walked through this from a gdb session,
and what is happening is that the close-share [sharename]
command is correctly terminating all resources and
changing $CWD back to the safe value of '/'.

What is then occuring is the Windows
client reconnect logic is firing silently underneath
Samba.

So before you can unmount the device etc. Windows
reconnects to the share resource, and is holding
the directory open again.

So I next tested removing the share definition
from the smb.conf, causing the smbd to reload it's
share definitions by doing:

bin/smbcontrol smbd reload-config

and *then* doing:

bin/smbcontrol <pid> close-share <share-name>

Unfortunately, this also fails :-(. The reason
for this is subtle. Once the reload-config
has been done, the share definition internally
in the smbd serving the client with the share
open is kept around, even though the share definition
has been removed. This is so that removing a share
definition in smb.conf doesn't automatically
disconnect existing users.

Then when the close-share request is done, the
existing connection is removed - but the definition
of the share is kept inside smbd. That means when
the client immediately reconnects, once any call
fails, the share definition is found and reconnected
successfully - keeping the $cwd open.

I have fixed this by the attached patch. What it
does is forces an atomic reload of a changed smb.conf
file once the share is forcibly disconnected as part
of the disconnection operation.

What that means is that if the share was removed from
smb.conf then once it's forcibly disconnected it
is also removed from the available shares, thus
disallowing any automatic reconnect (and so the share
directory is now available for unmount)

After inclusing this patch, the correct way to shut
down a share definition is:

a). Edit smb.conf to *remove* the share definition
you want to shut down.
b). bin/smbcontrol <pid> close-share <share-name>

The share will then not be available to automatically
reconnect from the client and you can umount
the underlying mount point.

This cannot be reproduced with smbclient
as it doesn't have auto reconnect logic.

Patches for 3.6.next and 4.0.next to follow.

Jeremy.
Comment 1 Jeremy Allison 2013-03-21 21:09:28 UTC
Created attachment 8671 [details]
git-am fix for 3.6.next.
Comment 2 Jeremy Allison 2013-03-21 21:10:51 UTC
Created attachment 8672 [details]
git-am fix for master and 4.0.next.

Will also post to samba-technical for master.
Comment 3 David Disseldorp 2013-03-22 11:39:44 UTC
Comment on attachment 8672 [details]
git-am fix for master and 4.0.next.

looks good.
Comment 4 David Disseldorp 2013-03-22 11:49:57 UTC
Comment on attachment 8671 [details]
git-am fix for 3.6.next.

Looks like the sharename="*" code path needs to be explicitly covered in 3.6. E.g:

diff --git a/source3/smbd/conn.c b/source3/smbd/conn.c
index 418c42a..c301164 100644
--- a/source3/smbd/conn.c
+++ b/source3/smbd/conn.c
@@ -481,7 +481,7 @@ void msg_force_tdis(struct messaging_context *msg,
        if (strcmp(sharename, "*") == 0) {
                DEBUG(1,("Forcing close of all shares\n"));
                conn_close_all(sconn);
-               return;
+               goto done;
        }
 
        if (sconn->using_smb2) {
@@ -513,6 +513,7 @@ void msg_force_tdis(struct messaging_context *msg,
                }
        }
 
+done:
        change_to_root_user();
        reload_services(msg, -1, true);
 }
Comment 5 Jeremy Allison 2013-03-22 16:24:36 UTC
Created attachment 8675 [details]
Modified git-am fix for 3.6.x

Great catch David - thanks !

Are you ok with me pushing the 4.0.x/master fix to master with your Reviewed-by: line or do you want to do it ?

Cheers,

Jeremy.
Comment 6 David Disseldorp 2013-03-22 16:31:26 UTC
(In reply to comment #5)
> Created attachment 8675 [details]
> Modified git-am fix for 3.6.x
> 
> Great catch David - thanks !
> 
> Are you ok with me pushing the 4.0.x/master fix to master with your
> Reviewed-by: line or do you want to do it ?

Feel free to push to master, I was holding out as you'd mentioned a samba-technical post.

Reviewed-by: David Disseldorp <ddiss@samba.org>
Comment 7 David Disseldorp 2013-03-22 16:32:56 UTC
Comment on attachment 8675 [details]
Modified git-am fix for 3.6.x

Looks good.
Comment 8 Jeremy Allison 2013-03-22 17:04:19 UTC
You missed it. I posted the "git-am fix for master and4.0.next" as a [PATCH] on samba-technical@samba.org yesterday :-).
Comment 9 Jeremy Allison 2013-03-22 17:07:32 UTC
Oh, my mistake. I neglected to CC: samba-technical :-(
Comment 10 Jeremy Allison 2013-03-22 17:10:24 UTC
Re-assigned to Karolin for inclusion in 4.0.next and 3.6.next.
Jeremy.
Comment 11 Karolin Seeger 2013-04-07 19:53:36 UTC
Pushed to v3-6-test and autobuild-v4-0-test.
Comment 12 Karolin Seeger 2013-04-08 06:52:33 UTC
Pushed to v4-0-test.
Closing out bug report.

Thanks!