Bug 8760 - Smbd wipes out the share mode lock entries of other file servers which are unregistered
Smbd wipes out the share mode lock entries of other file servers which are un...
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.1
All Linux
: P5 major
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-13 08:36 UTC by Manoj Dahal
Modified: 2012-02-21 19:47 UTC (History)
3 users (show)

See Also:


Attachments
Proposed Patch (735 bytes, patch)
2012-02-13 11:22 UTC, Manoj Dahal
no flags Details
Patch with the changes (2.32 KB, text/plain)
2012-02-14 10:57 UTC, Manoj Dahal
no flags Details
Patch for master (2.91 KB, patch)
2012-02-15 08:35 UTC, Volker Lendecke
no flags Details
Patch for master (3.98 KB, patch)
2012-02-15 10:24 UTC, Volker Lendecke
no flags Details
Patch for 3.6.1 (2.41 KB, text/plain)
2012-02-16 11:05 UTC, Manoj Dahal
mdahal: review+
Details
git-am fix for 3.6.next (3.00 KB, patch)
2012-02-16 23:47 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Manoj Dahal 2012-02-13 08:36:52 UTC
Smbd server wipes out the share mode lock entries which are added by other file servers (e.g. NCP and AFP) and which are not registered using register_serverid() API.

Environment: SLES-11-SP2 box + OES11-SP1 Preview 8 build

The following are the steps to reproduce the problem:

1) Create a common share between Samba and NCP Servers

2) Now create and open some ms office files using Novell Client

3) Use 'smbstatus' to see the share mode entries

4) Now open the same file using SMB client (e.g. Windows box)

5) Again use 'smbstatus' to see the entries. The share mode entries created
 by NCP Server are wiped out.


Expected Result: The smb server should not wipe out the share mode lock entries created by other file servers.
Comment 1 Manoj Dahal 2012-02-13 10:05:14 UTC
SLES 11 SP2 has newer version of Samba i.e. 3.6.1 where it has been added
one  new field called 'unique_id' in the server_id structure which is passed to
libsmbsharemode while creating a lock entry. The newer SMB has one feature called server registration and de-registration which uses the 'unique_id'. SMB cleans the entries of non-existent PID, but what if after a process dies and that process's pid is recycled immediately; then  it may not able to clean up the entries of process which died prematurely. To fix this, the PID of a process is augmented with a random 64-bit number and stored in serverid.tdb. With this the comparison is made with both pid and the random number to find if the server still exists. 

Right now the proposed solution is to:


For the time being the plan is: 

i) To add a 'don't verify unique_id' value which will be passed by other file servers while adding a samba share mode lock entry.

ii) When samba sees the entries with 'don't verify unique_id' then it will not
verify if this server id is registered in it's server.tdb.

iii) Other file servers don't have to register/de-register with a random
unique_id
Comment 2 Manoj Dahal 2012-02-13 10:07:16 UTC
Pasting the mail exchange here:


>>> Volker Lendecke <Volker.Lendecke@SerNet.DE> 1/25/2012 8:21 PM >>>
On Wed, Jan 25, 2012 at 03:47:58PM +0100, Stefan (metze) Metzmacher wrote:
> Hi Manoj,
> 
> > This is regarding your fix on recycled PIDs. I am an NCP developer from Novell and we use libsmbsharemodes library
> > from Samba for Cross Protocols Locks between NCP, Samba and others. I have few queries regarding your fix.
> >  
> > In your fix, you have added a new field called 'unique_id' in the server_id structure 
> > and we need to pass this in our call to samba share mode APIs  e.g. create_share_mode_entry().
> >  
> > Also, you have introduced server registration/de-registration which is associated with 'unqiue_id'. If we use these new APIs 
> > then in which library from Samba do we need to link to?  Or without calling serverid_register() can we directly pass any 'unique_id'
> > while calling to create_share_mode_entry() and in that case whether this and other share mode APIs will work properly?
> >  
> >  
> > Looking forward to your answer,
> 
> Maybe we could invent a special value e.g. UINT64_MAX as "don't verify
> this unique id".

Good idea. Did not think about that. This would assume that
the ncp server process never dies...

Volker
Comment 3 Manoj Dahal 2012-02-13 11:22:24 UTC
Created attachment 7313 [details]
Proposed Patch

As per comment#1 and comment#2, please find attached the proposed patch as a fix.
Comment 4 Stefan Metzmacher 2012-02-13 12:36:10 UTC
I think you should make sure that samba never uses the invalid unique_id.

BTW: I'm currently working on a more general fix for master,
so that we can also mark a full server_id as do not check.
Comment 5 Manoj Dahal 2012-02-13 16:48:56 UTC
(In reply to comment #4)
> I think you should make sure that samba never uses the invalid unique_id.

Understand this, thanks for indicating it. So while Samba generates an unique_id, we need to make sure that it is not the one used by NCP & others, right?

> BTW: I'm currently working on a more general fix for master,
> so that we can also mark a full server_id as do not check.
When is it expected to submit? Will it overlap with my fix?
Comment 6 Manoj Dahal 2012-02-14 10:57:26 UTC
Created attachment 7319 [details]
Patch with the changes

I have made the changes as suggested by Stefan. Now smbd does not use the invalid unique id. Please send your comments.
Comment 7 Jeremy Allison 2012-02-14 23:16:48 UTC
This patch looks very sensible to me.
Jeremy.
Comment 8 Manoj Dahal 2012-02-15 06:10:03 UTC
(In reply to comment #7)
> This patch looks very sensible to me.
> Jeremy.

Volker/Stefan/Jeremy, would you please let me know whether Samba accepts this patch for the upstream? So that I can move ahead in submitting the patch to SUSE as well.
Comment 9 Volker Lendecke 2012-02-15 08:35:44 UTC
Created attachment 7320 [details]
Patch for master

Attached find a patch with some cosmetic changes. https://bugzilla.samba.org/attachment.cgi?id=7319 does not apply cleanly to master, which this one does. Then, I did some cosmetic formatting changes, and the real change is that I added a SERVERID_ prefix to everything. Would that be ok to you as well?
Comment 10 Stefan Metzmacher 2012-02-15 09:19:29 UTC
Both versions need 'void' in the prototypes of serverid_get_random_unique_id().

The master version also needs to cover the serverids_exist() codepath,
maybe including the ctdb_serverids_exist() optimization (I haven't looked into the code of that function).
Comment 11 Manoj Dahal 2012-02-15 09:31:32 UTC
Stefan,
serverid_get_random_unique_id() returns a uint64_t, so I don't understand why to make it void type. 

Volker,

One trivial and cosmetic correction is required.
In the prototype declaration of serverid_get_random_unique_id(), should not we correct the comments as well? By replacing "Get a random unique_id and make sure that it is not UNIQUE_ID_NOT_TO_VERIFY" to "Get a random unique_id and make sure that it is not SERVERID_UNIQUE_ID_NOT_TO_VERIFY".

Otherwise you changes look fine to me.
Comment 12 Stefan Metzmacher 2012-02-15 09:49:15 UTC
(In reply to comment #11)
> Stefan,
> serverid_get_random_unique_id() returns a uint64_t, so I don't understand why
> to make it void type. 

uint64_t serverid_get_random_unique_id();

is an invalid prototype in C, it should be:

uint64_t serverid_get_random_unique_id(void);
Comment 13 Volker Lendecke 2012-02-15 10:24:45 UTC
Created attachment 7322 [details]
Patch for master

Updated patch
Comment 14 Stefan Metzmacher 2012-02-15 11:12:07 UTC
Comment on attachment 7322 [details]
Patch for master

I don't fully understand the ctdb_serverids_exist() change, but I trust Volker to do the correct thing there...

Otherwise I'm happy with the patch for master
Comment 15 Manoj Dahal 2012-02-15 11:45:28 UTC
If understand correctly, ctdb_serverids_exist() changes makes sure that if the server's unqiue_id is SERVERID_UNIQUE_ID_NOT_TO_VERIFY then it skips the verification process.
Comment 16 Manoj Dahal 2012-02-16 05:37:55 UTC
Volker, the code looks good now. What is the next step? When can it be submitted to upstream and when the SUSE build service can pull it for subsequent build cycles?
Comment 17 Volker Lendecke 2012-02-16 07:00:57 UTC
The patch is in master as of

http://git.samba.org/?p=samba.git;a=commitdiff;h=dd5868d41eeaa304a471822d7783526d9f4c37f5

I have not yet checked whether it applies cleanly to 3.6 (I would expect not). I would appreciate you to port it to 3.6 and upload the 3.6 patch here. We will then review it and bless it for the next 3.6 release. If you don't find the time to port it to 3.6, eventually someone will have to do it or you can wait for the next major release of Samba, which will have the functionality available.
Comment 18 Manoj Dahal 2012-02-16 11:05:38 UTC
Created attachment 7326 [details]
Patch for 3.6.1

Attaching patch for 3.6.1. Please review and let me know.
Comment 19 Jeremy Allison 2012-02-16 23:47:54 UTC
Created attachment 7328 [details]
git-am fix for 3.6.next

Back-ported fix as a git-am change for 3.6.next.
Comment 20 Karolin Seeger 2012-02-21 19:47:43 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!