[linux-cifs-client] Re: new umount race fix patch

Steve French smfrench at gmail.com
Wed Nov 12 14:54:03 GMT 2008


Other than the required indentation changes (in cifs_debug.c) the
current patch is reasonably small (about 150 lines changed).

A quick review for those interested in locking:

There are nested lists for sockets, SMB session and tree connections.
Adding or deleting from these lists, and updates to sensitive fields
within them such as the inUse counters requires a write lock on
GlobalSMBSesLock.   Walking any of these lists requires a read lock on
GlobalSMBSesLock.  If you grab both GlobalSMBSesLock and the
GlobalMidLock you must grab the GlobalSMBSesLock first to avoid
deadlock possibilities.

There have been various changes made to the locking of these
structures to fix mount/umount races.  At the moment I am still
checking how to avoid races in find_tcp_session while changing the
least amount of code.

A key problem which had to be addressed is when multiply mounting the
same server or share, how not to
return an entry in find_unc and find_tcp_session which is about to be freed.

The locking rules are more straightforward when described explicitly

A) The list of cifs tcp structures (struct TCP_Server_Info) are linked
into global_cifs_sock_list
List Initialized in: init_cifs
Added in: cifs_mount while holding SMBSesLock
Deleted in: cifs_demultiplex_thread as we are exiting while holding
SMBSesLock and MidLock
RefCount: cifs_sock->socketUseCount, used in six places,
          incremented in cifs_mount without locking, needs fixing
	  decremented in CIFSSMBLogoff while holding both locks and ses->sem
Status: cifs_sock->tcpStatus used in 53 places, but only updated in a few,
        protected by Midlock (and in case in which socketUseCount goes to
        zero and we set to exiting is protected by SMB ses lock as well)

B) the list of cifsSesInfo structs are linked into cifs_sock->smb_ses_list
Initialized: cifs_mount
Added: sesInfoAlloc while holding SMBSeslock
Deleted: sesInfoFree while holding SMBSeslock
RefCount: ses->inUse used in 4 places
          incremented in tconInfoAlloc while holding SMBSeslock
             but races with tcp_find_session need to be fixed
	  decremented in CIFSSMBLogoff while holding both locks and ses->sem
	  error paths when session is dead need to be checked
         (we try to send uLogoff even when socket is dead)
Status: ses->status used in 15 places

C) the list of cifsTconInfo structs are linked into ses->tcon_list
Initialized: sesInfoAlloc
Added:  tconInfoAlloc
Deleted: tconInfoFree
RefCount: tcon->useCount used in 5 places
          Incremented in find_unc and in cifs_mount
	  error paths in cifs_mount need to be checked when racing with find_unc
          Decremented in CIFSSMBLogoff while holding SMBSeslock and sesSem
Status: tcon->tidStatus used in six places, 4 while holding SMBSeslock


There are two additional lists related to the above which do not
appear to be a problem:
A list of cifsFileInfo structures hangs off tcon->openFileList
Initialized: tconInfoAlloc
Added: cifs_open while holding SMBSesLock
Deleted: cifs_close while holding SMBSesLock

A list of mid_q_entries hangs off cifs_sock->pending_mid_q
Initialized: cifs_mount
Added: AllocMidQEntry while holding MidLock
Deleted: DeleteMidQEntry while holding MidLock





On Wed, Nov 12, 2008 at 8:04 AM, Jeff Layton <jlayton at redhat.com> wrote:
> On Wed, 12 Nov 2008 07:35:02 -0600
> "Steve French" <smfrench at gmail.com> wrote:
>
>> I agree that the refcounting isn't finished for the socket and ses in
>> use counts - and I was looking at moving some of that within the code
>> which adds it on the linked list which is not that different from what
>> you were doing.
>>
>
> Cool. Can you also change the tcon refcounting code to use similar
> interfaces? While it looks like your patch so far closes the races with
> the tcon, it's very hard to follow.
>
> --
> Jeff Layton <jlayton at redhat.com>
>



-- 
Thanks,

Steve


More information about the linux-cifs-client mailing list