Bug 7118 - nmbd problems with socket address
Summary: nmbd problems with socket address
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: Nmbd (show other bugs)
Version: 3.5.0rc2
Hardware: Other Linux
: P3 normal
Target Milestone: ---
Assignee: Stefan Metzmacher
QA Contact: Samba QA Contact
URL:
Keywords:
: 3851 4166 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-02-08 12:20 UTC by Stefan Metzmacher
Modified: 2011-07-30 09:27 UTC (History)
7 users (show)

See Also:


Attachments
Patch for v3-5 (22.44 KB, patch)
2010-02-08 12:21 UTC, Stefan Metzmacher
metze: review? (jra)
Details
Additional git-am patch needed for v3-5. (2.62 KB, patch)
2010-02-08 20:46 UTC, Jeremy Allison
metze: review+
Details
Patch for v3-4 (22.44 KB, patch)
2010-02-10 04:58 UTC, Stefan Metzmacher
jra: review-
Details
git-am fix for 3.5.0. (5.05 KB, patch)
2010-02-10 14:41 UTC, Jeremy Allison
metze: review+
Details
New patches for v3-4 (with "nmbd bind explicit broadcast = no" as default) (27.56 KB, patch)
2010-02-11 05:48 UTC, Stefan Metzmacher
kseeger: review? (jra)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2010-02-08 12:20:29 UTC
nmbd doesn't work when the socket address option is used.
Comment 1 Stefan Metzmacher 2010-02-08 12:21:04 UTC
Created attachment 5297 [details]
Patch for v3-5
Comment 2 Jeremy Allison 2010-02-08 17:30:14 UTC
*** Bug 3851 has been marked as a duplicate of this bug. ***
Comment 3 Jeremy Allison 2010-02-08 17:30:47 UTC
*** Bug 4166 has been marked as a duplicate of this bug. ***
Comment 4 Jeremy Allison 2010-02-08 20:46:02 UTC
Created attachment 5300 [details]
Additional git-am patch needed for v3-5.

Here is one additional patch I think is necessary. It fixes a comment typo, turns on "nmbd bind explicit broadcast" by default, and fixes the manpage to show this (and be more explicit about what the option does).

Metze please review. If you agree I'll re-assign to Karolin for inclusion in 3.5.1 (I think this is too late for 3.5.0).

Jeremy.
Comment 5 Stefan Metzmacher 2010-02-09 01:36:49 UTC
Comment on attachment 5300 [details]
Additional git-am patch needed for v3-5.

Looks good for 3.5.0

But I'll propose the backport to 3.4.x without this change
Comment 6 Stefan Metzmacher 2010-02-09 04:17:19 UTC
If we change the default to yes, then we need it in 3.5.0.
We can't change it within the 3.5.x releases.

I'd really like to get the feature into 3.5.0,
while I'm happy to keep it of by default.
Comment 7 Stefan Metzmacher 2010-02-09 04:18:50 UTC
...and that would allow people with problems to turn it on (and we have really fixed all the other bugreports)

Comment 8 Stefan Metzmacher 2010-02-10 04:58:15 UTC
Created attachment 5316 [details]
Patch for v3-4
Comment 9 Stefan Metzmacher 2010-02-10 05:01:57 UTC
Jeremy,

I think my patches should go into 3.5.0 and 3.4.6,
please ack and assign to Karolin.

And it's up to you if you want your patch in 3.5.0.
(Changing the default with 3.5.1 is not an option,
 we need it in 3.5.0 or keep the old default for all
 3.5.x releases)
Comment 10 Stefan Metzmacher 2010-02-10 08:07:09 UTC
Comment on attachment 5300 [details]
Additional git-am patch needed for v3-5.

Günther told me this:

14:30 < gd> Got a positive name query response from 172.31.1.50 ( 172.31.1.50 )
14:30 < gd> 172.31.1.50 mthelena<00>
14:31 < gd> 172.31.1.50 mthelena<00>

With the new default we process packets twice.

So we should not change the default in any version.

Jeremy please do a
git revert f2415c9bf9342aeb5
Comment 11 Stefan Metzmacher 2010-02-10 08:26:39 UTC
Sorry I mean 3f2415c9bf934
Comment 12 Jeremy Allison 2010-02-10 10:59:48 UTC
Will do (once I'm in at work). I'll look into this asap and get a proper fix.
Jeremy.
Comment 13 Stefan Metzmacher 2010-02-10 11:11:23 UTC
I really think the correct way to do this is to keep the old default,
or also use the subnet records as outgoing interfaces.

But I think we should just go with my patches only for 3.4.6 and 3.5.0.

And may create a different solution for 3.6.0.

Can you ack my changes so that Karolin can pick them?
Comment 14 Jeremy Allison 2010-02-10 12:42:10 UTC
Comment on attachment 5316 [details]
Patch for v3-4

Sorry, I don't want to ack this whilst there is the bug that gd discovered.

I'd rather not add code that's wrong, but disabled by default. I'd rather not add this and get it right. We can always add this option in 3.5.1 once we've worked out the real issue with the patch.

Jeremy.
Comment 15 Stefan Metzmacher 2010-02-10 13:06:21 UTC
There's no real bug.

I think it's just that we get the request twice from the kernel
if we listen on 0.0.0.0:137 and 172.31.9.255:137 and reply twice.

With "nmbd bind explicit broadcast = no" gd was getting just one
reply. 
Comment 16 Jeremy Allison 2010-02-10 13:24:05 UTC
No that's a bug :-). We need to detect the duplicate and discard it.
Jeremy.
Comment 17 Stefan Metzmacher 2010-02-10 13:41:20 UTC
So, you mean we should add a cache with client-ip:reqiest-id combinations
to see if we already got the request within the last minute or so?
Comment 18 Jeremy Allison 2010-02-10 13:45:43 UTC
Yes. I'm in the middle of coding this up. Realistically, a minute is *way* too long. Remember we're only trying to detect duplicate packets being sent by the kernel up both sockets once it's received a packet. Practically, a cache that is in existence only as long as listen_for_packets() is running (ie. only detects duplicates coming in on the same select() call) would almost certainly catch most duplicates.

Jeremy.
Comment 19 Jeremy Allison 2010-02-10 14:41:07 UTC
Created attachment 5322 [details]
git-am fix for 3.5.0.

Metze, here is the simple cache that fixes the duplicate packet problem. IMHO we should only be putting this code (all of the bugfix, not just this segment) into 3.5.0, not into 3.4.x as it's too big a change for earlier versions.

I've tested this and pushed to master. I'll turn on "
nmbd bind explicit broadcast" by default again in master now we're ok with the duplicates.

Jeremy.
Comment 20 Stefan Metzmacher 2010-02-11 02:24:25 UTC
Comment on attachment 5322 [details]
git-am fix for 3.5.0.

Ok, for 3.5.0
Comment 21 Stefan Metzmacher 2010-02-11 02:27:01 UTC
Ok, we'll keep it as custom patch in our 3.4 RPMs for now.

We can rethink about a backport with the old default for 3.4.7...
Comment 22 Stefan Metzmacher 2010-02-11 05:19:28 UTC
Comment on attachment 5300 [details]
Additional git-am patch needed for v3-5.

This is ok for 3.5.0 in combination with

https://bugzilla.samba.org/attachment.cgi?id=5322
Comment 23 Stefan Metzmacher 2010-02-11 05:24:33 UTC
Karolin, please pull the 3 attachments for v3-5 into v3-5-test
Comment 24 Karolin Seeger 2010-02-11 05:31:52 UTC
Pushed the three 3.5 patches.
Re-assigning to Metze to address 3.4.
Comment 25 Stefan Metzmacher 2010-02-11 05:48:15 UTC
Created attachment 5324 [details]
New patches for v3-4 (with "nmbd bind explicit broadcast = no" as default)

We might pick that for 3.4.7....
Comment 26 TAKAHASHI Motonobu 2010-03-04 06:53:32 UTC
Now I propose removal of "socket address" parameter.

If we want to use nmbd on specified (multiple) interfaces only for security reason, although simply "nmbd bind explicit broadcast = yes" is enough but we 
must change "socket address" parameter because its default value (0.0.0.0)
make nmbd accept packets from all interfaces. 

Here is the output of "netstat" with "nmbd bind explicit broadcast = yes" and
"socket address = 0.0.0.0 (default)".

-----
netstat -an | egrep '13[789]|445'
tcp        0      0 192.168.135.247:139     0.0.0.0:*               LISTEN
tcp        0      0 192.168.135.247:445     0.0.0.0:*               LISTEN
tcp6       0      0 fe80::20c:29ff:fe44:139 :::*                    LISTEN
tcp6       0      0 fe80::20c:29ff:fe44:445 :::*                    LISTEN
udp        0      0 192.168.135.255:137     0.0.0.0:*
udp        0      0 192.168.135.247:137     0.0.0.0:*
udp        0      0 0.0.0.0:137             0.0.0.0:* <-- needless
udp        0      0 192.168.135.255:138     0.0.0.0:*
udp        0      0 192.168.135.247:138     0.0.0.0:*
udp        0      0 0.0.0.0:138             0.0.0.0:* <-- needless
-----

And we realize that we no longer have no proper value set to  
"socket address" parameter with "nmbd bind explicit broadcast = yes".

I propose that 

 (1) If "nmbd bind explicit broadcast = yes", then
    nmbd binds each broadcast addresses explicitly and does not bind to
    0.0.0.0 (and binds to the interface address).

 (2) If "nmbd bind explicit broadcast = no", then
    nmbd binds to 0.0.0.0 only (and binds to the interface address).

Comment 27 Stefan Metzmacher 2011-07-30 09:27:09 UTC
Removing "socket address" might something for the future...

It seems this won't make it into 3.4 anymore, so closing the bug report
as it's already fixed in 3.5.