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.
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
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
Created attachment 7313 [details] Proposed Patch As per comment#1 and comment#2, please find attached the proposed patch as a fix.
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.
(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?
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.
This patch looks very sensible to me. Jeremy.
(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.
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?
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).
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.
(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);
Created attachment 7322 [details] Patch for master Updated patch
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
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.
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?
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.
Created attachment 7326 [details] Patch for 3.6.1 Attaching patch for 3.6.1. Please review and let me know.
Created attachment 7328 [details] git-am fix for 3.6.next Back-ported fix as a git-am change for 3.6.next.
Pushed to v3-6-test. Closing out bug report. Thanks!