Bug 8935 - Unable to map drive when "default service" is assign in smb.conf file
Summary: Unable to map drive when "default service" is assign in smb.conf file
Status: REOPENED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.13.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-11 13:47 UTC by emorel@ca.ibm.com (mail address dead)
Modified: 2020-12-20 02:26 UTC (History)
4 users (show)

See Also:


Attachments
Raw patch for 3.6.x (4.57 KB, patch)
2013-09-27 13:51 UTC, Jeremy Allison
no flags Details
[patch] updated patch for 4.4.x (6.43 KB, patch)
2016-04-05 16:58 UTC, Guillaume Xavier Taillon (mail address dead)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description emorel@ca.ibm.com (mail address dead) 2012-05-11 13:47:48 UTC
with samba-3.6.5 When "default service" is assign in smb.conf file
and try to map a fileset in it, an error occur ( The network name cannot be found. )


*********  Part of the smb.conf file

default service = projects
.
.
.

[projects]
        path = /dfs/projects/%S
        read only = No
        browseable = No


**********

********** Exemple

Let say i try to map 
	net use * \\servername\testconnection /user:xxxxxxx

after entering my password i got error ( The network name cannot be found. )
when i look into log file i see Samba trying to map /dfs/projects/projects ?????

**********

********part of the log file

[2012/04/27 15:44:12.314600,  0] smbd/service.c:1064(make_connection_snum)
  '/dfs/projects/projects' does not exist or permission denied when connecting to [projects] Error was A file or directory in the path name does not exist.
[2012/04/27 15:44:12.315669,  3] smbd/connection.c:35(yield_connection)
  Yielding connection to projects
[2012/04/27 15:44:12.315797,  5] lib/messages.c:332(messaging_deregister)
  Deregistering messaging pointer for type 784 - private_data=20451458
[2012/04/27 15:44:12.315889,  3] smbd/error.c:81(error_packet_set)
  error packet at smbd/reply.c(803) cmd=117 (SMBtconX) NT_STATUS_BAD_NETWORK_NAME

********


DEBUGING

i made a change in the source3/smdb/service.c file at line 426
i change the call for function lp_add_service to replace *service_out by service_in who contain the original service name.
and now it work fine


original line

+424                          if (iService >= 0) {
+425                                  all_string_sub(*p_service_out, "_","/",0);
+426                                  iService = lp_add_service(*service_out, iService);
+427			      }	


modify line 

+424                          if (iService >= 0) {
+425                                  all_string_sub(*p_service_out, "_","/",0);
+426                                  iService = lp_add_service(service_in, iService);
+427			      }
Comment 1 Jeremy Allison 2012-05-11 14:05:09 UTC
I don't see the bug here.

You default service is "projects", and in the projects service definition you have a path of :

/dfs/projects/%S

So the service name you're connecting to is "projects", and the %S macro expands to the service name you're connecting to, so the path becomes "/dfs/projects/projects", which is exactly what you see in your log file.

Doesn't look like a bug to me.

Jeremy.
Comment 2 emorel@ca.ibm.com (mail address dead) 2012-05-11 15:06:22 UTC
Jeremy,


Maybe it's not a bug, but in previous version (before 3.6)
3.5.11
3.5.9
3.5.5
3.3.0
3.0.11

it was working with the smb.conf exemple i send you.
so i was wondering why the change have been made and how should i setup
my environement to work this way ?

in the previous version (before 3.6) it was working like this.
The %S macro was replace by testconnection and not by projects

net use * \\servername\testconnection /user:xxxxxxx
i expect to have a divre map on /dfs/projects/testconnection


Tks.
Comment 3 Yannick Bergeron (mail address dead) 2012-05-11 15:09:31 UTC
Hi Jeremy,

just fyi, Éric is working with me on this Samba service, if you remember our DCE/DFS/GPFS environment with Samba on AIX ;)
Comment 4 Jeremy Allison 2012-05-11 15:24:05 UTC
Oh I see, you expect it to be the original name. Ok, let me look at this..
Comment 5 Jeremy Allison 2012-05-11 15:39:39 UTC
Ok, I see the issue, and why it works with your patch.

However, there is a problem here.

The old way, whenever you connect to a service with an unknown name, it will create a new service with that name. This causes memory allocation within smbd. The new code, whenever you connect to a service with an unknown name, it connects you to the existing instance of the default service. This doesn't leak any memory.

If I change it back to the way you had it, any authenticated user could blow up smbd resource usage by running :

smbclient //server/<random_share_name>

in a loop, and bloat up the smbd server memory containing share definitions, as a possible DOS attack.

So, it's questionable whether your fix to restore the old behavior is the correct thing to do here.

Jeremy.
Comment 6 Yannick Bergeron (mail address dead) 2012-05-11 15:50:04 UTC
We don't pretend that our fix is the right way. It's just a way to get it works like before.
This is why we are now opening the bugzilla to report and understand the issue and get the right fix released

What would you suggest then?

Best regards,

Yannick Bergeron
Comment 7 Jeremy Allison 2012-05-11 21:20:55 UTC
What I'm saying is the way it worked before might not be the correct thing to do. It's an obscure enough corner case that either behavior might be considered "correct". Look at the %S definition in smb.conf:

       %S
           the name of the current service, if any.

The current service is definitely "projects", as that's what the "default service" sets it to.

I'm inclined to leave the current code alone as is, and ask you to update your configuration to match to be honest, as it's cleaner and safer smbd server side code than before.

Jeremy.
Comment 8 Yannick Bergeron (mail address dead) 2012-05-14 11:50:30 UTC
Is the only alternative would be to define more than 1000 shares in smb.conf?
Knowing that each day, a few get created and deleted

The old behavior was kinda useful for this :/
Comment 9 Jeremy Allison 2012-05-14 16:44:26 UTC
Well hang on a minute. Don't usershares or other options help here ? I'm not convinced you're completely unable to adapt to the new behavior. What is the precise problem you're trying to solve ?

Jeremy.
Comment 10 Yannick Bergeron (mail address dead) 2012-05-14 18:04:57 UTC
Our filesets are separate in 2 categories: homes and projects
2000 projects are in /dfs/projects
2600 homes are in /dfs/home
Here is a simplified version of our smb.conf

[global]
	default service = projects

[homes]
        comment = Home Directories
        read only = No
        browseable = No

[projects]
        path = /dfs/projects/%S
        read only = No
        browseable = No



When someone want to map to his home directory, he does it against \\samba\username (ex: \\samba\jsmith for user "jsmith")
When someone want to map to a project, he does it against \\samba\projectname (ex: \\samba\foo for the project "foo")

It's been that way since the last 12 years and I need to keep it that way.
If this can't be rolled back, I don't what options are left.
Defining 4600 shares, even automatically, does not look like a very good solution.

If you know other options, I would be more than interested.
If you don't, I beg you to understand the case and to consider a solution that would allow the old behavior to work as before, maybe with a safety against the memory leak / potential denial of service.

Best regards,

Yannick Bergeron
Comment 11 Volker Lendecke 2012-05-15 15:03:46 UTC
We probably need something like autoshares (maybe we already have it?). I think Windows has such a thing: Share all subdirs of a directory as shares.
Comment 12 Yannick Bergeron (mail address dead) 2012-06-04 17:36:23 UTC
Fyi, we've slightly modified our patch because the last one was causing issues if we first entered the share name in uppercase and then trying to access it in lowercase.

We still understand it's not suitable because of the possibility to add tons of share dynamically in memory.

Have you thought about another way to manage our needs without getting a modification in Samba code?
If not, Volker proposal seems very interesting, but I hardly see when it would be implemented.
Wouldn't it be a easier to fix a limit to the number of share in this array and use a LRU algorithm when you hit the limit?

Any other proposal that could be done in a reasonable amount of time, for example, to be in 3.6 and master in the next 6 months?

Best regards,

Yannick Bergeron
Comment 13 Yannick Bergeron (mail address dead) 2012-06-04 17:40:55 UTC
hmm doesn't seem to be able to attach the patch

here is a quick copy/paste

***************
*** 422,427 ****
--- 422,430 ----
                                goto fail;
                        }
                        if (iService >= 0) {
+                               *p_service_out = talloc_strdup(ctx, service_in);
+                               all_string_sub(*p_service_out,"\\","/",0);
+                               strlower_m(*p_service_out);
                                all_string_sub(*p_service_out, "_","/",0);
                                iService = lp_add_service(*p_service_out, iService);
                        }
Comment 14 Karolin Seeger 2012-08-29 08:00:48 UTC
Re-assigning to Volker.
Comment 15 Yannick Bergeron (mail address dead) 2012-10-16 17:05:19 UTC
Hi,

quick reminder that we would really appreciate if this bug/patch could be reviewed and pushed in 3.6.x

Best regards,

Yannick Bergeron
Comment 16 Jeremy Allison 2012-10-17 07:11:21 UTC
What about this issue I raised previously ?:

--------------------------------------------------------
If I change it back to the way you had it, any authenticated user could blow up
smbd resource usage by running :

smbclient //server/<random_share_name>

in a loop, and bloat up the smbd server memory containing share definitions, as
a possible DOS attack.

So, it's questionable whether your fix to restore the old behavior is the
correct thing to do here.
--------------------------------------------------------

I'll discuss this with Volker and see what we should do here.

Jeremy.
Comment 17 Yannick Bergeron (mail address dead) 2012-10-17 11:57:58 UTC
Totally understand and agree to your point.

But I "guess" there are technical ways to prevent this
ex: fixing a maximum size to the array containing the share definitions and use a Least Recently Used algorithm to figure which one to keep once the maximum size has been reach?

Best regards,

Yannick Bergeron
Comment 18 Andrew Bartlett 2013-06-07 22:53:11 UTC
Could we solve this by moving the %S sub in 'path = ' from the loadparm code to the tree connect code path, like how the literal [homes] (not the extra [abartlet] share in my case) is handled?

That is, we already allocate significant memory when we connect a tree, have limits there and wouldn't add 'shares' to the loadparm / smb.conf array.

The downside is that like [homes] it wouldn't work for non-VFS users, of which there are a number in the srvsvc, printing and msdfs areas (see git grep lp_pathname).
Comment 19 Ty! Boyack 2013-09-20 16:36:00 UTC
Has there been any additional movement on this?  We rely pretty heavily on this default service working to avoid having errors in all of our network mount points.  Having to list all of the shares in this file is a duplication of what we already do with NFS automounters and such, so this has been a very valuable feature.

Andrew sounded like he had a solution possibility that could help out, or I was wondering about other options:

1) could we have a run-time option to risk the denial of service to get back the old behavior?  We understand the risk, and in some environments it's an acceptable risk.  (but I agree that the default behavior should be to NOT incur that risk)

2) Could it be done in such a way that the share lookup is done (even in a cursory sense, maybe by something with reusable memory even if it had to serialize those requests) before the memory allocation for the whole share, and only allocates the memory if the share looks valid?

One other question I should ask for the good of this list and people who may be coming across the same problem -- if this is not the correct way to do this, is there a way to achieve the same behavior of what we were used (that is, having a "default service" that maps to "path = /path/to/%S", without distinct entries for all %S possibilites)?  If I'm just stuck on the wrong tool I'm happy to change.
Comment 20 Yannick Bergeron (mail address dead) 2013-09-23 12:19:22 UTC
Hi,

I would also appreciate some news regarding this issue.
We need to update to the latest 3.6.x before April 2014 and might do it between November 2013 and February 2014.
I would prefer to use the vanilla code instead of a custom local patch to fix this issue.
But if there is a way to achieve the same with configuration (without defining 4000 shares), that would also be a valid alternative.

Best regards,

Yannick Bergeron.
Comment 21 Jeremy Allison 2013-09-27 12:35:36 UTC
Well the man page is now wrong in describing the current behaviour. It states:

Also note that the apparent service name will be changed to equal that of the requested service,
this is very useful as it allows you to use macros like %S to make a wildcard service.

Which is now incorrect as it describes the previous behaviour.

So at the very least the man page needs updating. No one has yet come up with a work-around for the memory leak issue. Until that happens, then I can't see a way to get around this issue.

Jeremy.
Comment 22 Jeremy Allison 2013-09-27 12:50:33 UTC
Ok, I'll go with this fix so long as we have a "max default services" parameter (maybe default it to 10) that prevents memory blowup.

Jeremy.
Comment 23 Jeremy Allison 2013-09-27 13:51:58 UTC
Created attachment 9246 [details]
Raw patch for 3.6.x

Ok, here is a raw patch for 3.6.x that I think will do what you want, but in a restricted way.

It limits the number of default shares per smbd to 10, user configurable with a new global parameter "max default services".

Please test this out and if you can confirm it does what you want, I'll add the XML docs and propose it as a fix for master, 4.1.x, and 4.0.x.

Jeremy.
Comment 24 Ty! Boyack 2014-05-16 22:02:34 UTC
[Forgive me if this is a repost -- it looks like my comment didn't get added correctly]

Thanks for the patch, Jeremy.  I have not had the time to test it, but wanted to ask another question.  I don't have any 3.6 systems in production, but I do have 4.0 and 4.1 systems.  Do you think that patch would apply on those sources?  I will try to get some time to test this, but would really prefer to work on the 4.0 branch so that if it works we can move forward rather than waiting for it to get pushed to 4.  If it needs to be done in 3.6 I can try that, it's just not as straightfoward of a comparison for me.

This is still a very important patch to us -- we've worked out some other processes to keep things in sync without this feature, but we've had a lot more process failures this way.  So I really appreciate you putting the patch together!

-Ty
Comment 25 Yannick Bergeron (mail address dead) 2014-05-20 11:30:39 UTC
fyi, we've tried to use this patch in 3.6.22 and ended up with issues.
First attempt was with the patch but we forgot to put the "max default services" in smb.conf
We experienced random issues from a few workstation that were not able to find a network share (even with higher debug level it wasn't making much sense)

Then we've set the parameter in smb.conf and started to experience high cpu from the parent smbd process (and a few smbd childs sometimes) after 24-36hrs. 

We've finally updated to 3.6.22 with a custom patch similar to what we've applied to 3.6.5 back in time.

Next upgrade will propbably be to 4.0.x or 4.1.x so we'll pick what is there and address issues if they remains

Best regards,

Yannick Bergeron
Comment 26 Guillaume Xavier Taillon (mail address dead) 2016-04-05 16:58:00 UTC
Created attachment 11964 [details]
[patch] updated patch for 4.4.x

Hello Jeremy,
Yannick has moved and no longer works with us, but your patch has been running on our servers for quite some time now, and from what I gather, without issues.

Here it is updated for 4.4.0 and so far still works as intended on AIX.