Bug 14604 - SMB share is still accessible after deleting it from the registry.
Summary: SMB share is still accessible after deleting it from the registry.
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.12.9
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-30 13:53 UTC by Lev
Modified: 2021-01-05 20:36 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lev 2020-12-30 13:53:55 UTC
Assume there are two users "dima" and "lev" connected to two shares "smb3" and "smb4" from the same Windows client:

# net conf listshares
global
clone1
smb2
smb4
smb3

# net status sessions
PID     Username      Group         Machine                        
-------------------------------------------------------------------
  23443   ZADARA2\dima  ZADARA2\domain users  10.2.11.110  (ipv4:10.2.11.110:55382)
  23443   ZADARA2\lev   ZADARA2\domain users  10.2.11.110  (ipv4:10.2.11.110:55382)
  
# net status shares
Service      pid     machine       Connected at
-------------------------------------------------------
smb3         23443   10.2.11.110   Wed Dec 30 14:40:25 2020
smb4         23443   10.2.11.110   Wed Dec 30 14:40:26 2020
smb3         23443   10.2.11.110   Wed Dec 30 14:34:40 2020
smb4         23443   10.2.11.110   Wed Dec 30 00:04:17 2020

Now I want to delete the share "smb3" and close all connections to it:

# net conf delshare smb3
delete_share_security: Failed to delete entry for share smb3: NT_STATUS_NOT_FOUND    <-- harmless but annoying message from https://github.com/zadarastorage/zadara-samba/commit/ff35160dc6415e1ef5d57717d7636f38ea25a8f0#diff-fa14626f82ba5c586b2cbdcd183432dff0f605d68191cb0717db82a15600dc65

# echo $?
0

# net conf listshares
global
clone1
smb2
smb4

# smbcontrol all close-share smb3

[2020/12/30 14:45:07.602223,  1, pid=23443] ../../source3/smbd/conn_idle.c:128(conn_force_tdis)
  conn_force_tdis: Forcing close of share 'smb3' (wire_id=0x22f28611)
[2020/12/30 14:45:07.602613,  1, pid=23443] ../../source3/smbd/conn_idle.c:128(conn_force_tdis)
  conn_force_tdis: Forcing close of share 'smb3' (wire_id=0x8ba65444)
[2020/12/30 14:45:07.602813,  1, pid=23443] ../../source3/smbd/conn_idle.c:256(conn_force_tdis_done)
  conn_force_tdis_done: Closing share 'smb3' (wire_id=0x22f28611)
[2020/12/30 14:45:07.603409,  1, pid=23443] ../../source3/smbd/service.c:1229(close_cnum)
  vm110 (ipv4:10.2.11.110:55382) closed connection to service smb3
[2020/12/30 14:45:07.610345,  1, pid=23443] ../../source3/smbd/conn_idle.c:256(conn_force_tdis_done)
  conn_force_tdis_done: Closing share 'smb3' (wire_id=0x8ba65444)
[2020/12/30 14:45:07.611073,  1, pid=23443] ../../source3/smbd/service.c:1229(close_cnum)
  vm110 (ipv4:10.2.11.110:55382) closed connection to service smb3

# net status shares
Service      pid     machine       Connected at
-------------------------------------------------------
smb4         23443   10.2.11.110   Wed Dec 30 14:40:26 2020
smb4         23443   10.2.11.110   Wed Dec 30 00:04:17 2020

However array ServicePtrs[] still contains configuration for "smb3"!

# gdb -p 23443
(gdb) p iNumServices
$4 = 5
(gdb) p ServicePtrs[4]->szService
$5 = 0x7f778cd83ab0 "smb3"
(gdb) p ServicePtrs[4]->valid    
$6 = true
(gdb) p *ServicePtrs[4]->hosts_allow
$7 = 0x7f778cd6fce0 "10.2.11.110"

As a result users still may connect, despite the share was deleted from the registry:

C:\Users\dima>net use \\10.2.11.26\smb3
The command completed successfully.

[2020/12/30 14:51:39.481539,  1, pid=23443, effective(0, 0), real(0, 0)] ../../source3/smbd/service.c:947(make_connection_snum)
  vm110 (ipv4:10.2.11.110:55382) connect to service smb3 initially as user ZADARA2\dima (uid=5001116, gid=5000513) (pid 23443)
  
# net status shares

Service      pid     machine       Connected at
-------------------------------------------------------
smb4         23443   10.2.11.110   Wed Dec 30 14:40:26 2020
smb3         23443   10.2.11.110   Wed Dec 30 14:51:39 2020
smb4         23443   10.2.11.110   Wed Dec 30 00:04:17 2020

I think this is regression in the commit https://github.com/samba-team/samba/commit/7891302ab8eeba8261b92171a4d429e2f538b89a.

Before this commit function conn_force_tdis() closed the required connections and then called reload_services(). But now reload_services() is called from conn_force_tdis_done() every time the connection is closed. In the scenario above it is called twice.
When the 1st connection is closed, lp_file_list_changed() returns True, but then lp_killunused() -> conn_snum_used() returns True, because of the 2nd connection.
And when the 2nd connection is closed, lp_file_list_changed() returns False, because the 1st call to lp_file_list_changed() updated f->modtime.
So lp_killunused() nevers calls free_service_byindex(), and ServicePtrs[] remains not updated.

* Connection to smb4 is needed to hold the process running after connections to smb3 are closed.
Comment 1 Jeremy Allison 2021-01-01 07:41:10 UTC
Analysis looks good. Any chance you can suggest a patch for this ?

Thanks,

Jeremy.
Comment 2 Jeremy Allison 2021-01-01 07:58:15 UTC
At a naive glance changing the last 'bool test' parameter from "true" to 'false' inside conn_force_tdis_done() might fix this. Can you check ?
Comment 3 Lev 2021-01-03 13:56:56 UTC
(In reply to Jeremy Allison from comment #2)

Hi Jeremy,

Yes, simple patch

diff --git a/source3/smbd/conn_idle.c b/source3/smbd/conn_idle.c
index cf5a417bff7..1a774942b25 100644
--- a/source3/smbd/conn_idle.c
+++ b/source3/smbd/conn_idle.c
@@ -273,5 +273,5 @@ static void conn_force_tdis_done(struct tevent_req *req)
        * uid in the meantime. Ensure we're still root.
        */
        change_to_root_user();
-       reload_services(sconn, conn_snum_used, true);
+       reload_services(sconn, conn_snum_used, false);
 }
 
fixed the issue. 

The only downside I see is that now reload_services() will do all its stuff after each connection close, and not only once, as it was in 4.11, but probably this is good enough.

Thanks, 
 Lev.
Comment 4 Jeremy Allison 2021-01-03 20:49:07 UTC
It's not that bad. Remember this code is only called from conn_force_tdis_done() (not the *FORCE* part :-), not the regular tcon disconnect path. So it's only called when smbcontrol is being used to force disconnect, not a regular client disconnect.
Comment 5 Jeremy Allison 2021-01-03 20:49:27 UTC
s/not the/note the/ (bad keyboard at home :-).
Comment 6 Jeremy Allison 2021-01-05 20:36:00 UTC
This could also be fixed by adding a tevent_wait_queue and moving the:

        change_to_root_user();
        reload_services(sconn, conn_snum_used, true);

calls into the trigger function of the last queue entry. But this might be too heavyweight a change for something that is only called because of an explicit admin smbcontrol request.