On 10/07/2010 04:37 PM, Jeff Layton wrote:
> On Thu, 07 Oct 2010 12:37:19 +0530
> Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
>
>> On 10/07/2010 01:24 AM, Jeff Layton wrote:
>>> Now that it's feasible for a cifsFileInfo to outlive the filp under
>>> which it was created, move the close processing into cifsFileInfo_put.
>>>
>>> This means that the last user of the filehandle always does the actual
>>> on the wire close call. This also allows us to get rid of the closePend
>>> flag from cifsFileInfo. If we have an active reference to the file
>>> then it's never going to have a close pending.
>>>
>>> cifs_close is converted to simply put the filehandle.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>> ---
>>> fs/cifs/cifsglob.h | 1 -
>>> fs/cifs/file.c | 187 +++++++++++++++++-----------------------------------
>>> fs/cifs/misc.c | 10 ---
>>> 3 files changed, 60 insertions(+), 138 deletions(-)
>>>
>>> int cifs_open(struct inode *inode, struct file *file)
>>> @@ -542,79 +582,11 @@ reopen_error_exit:
>>>
>>> int cifs_close(struct inode *inode, struct file *file)
>>> {
>>> - int rc = 0;
>>> - int xid, timeout;
>>> - struct cifs_sb_info *cifs_sb;
>>> - struct cifsTconInfo *pTcon;
>>> - struct cifsFileInfo *pSMBFile = file->private_data;
>>> -
>>> - xid = GetXid();
>>> -
>>> - cifs_sb = CIFS_SB(inode->i_sb);
>>> - pTcon = tlink_tcon(pSMBFile->tlink);
>>> - if (pSMBFile) {
>>> - struct cifsLockInfo *li, *tmp;
>>> - write_lock(&cifs_file_list_lock);
>>> - pSMBFile->closePend = true;
>>> - if (pTcon) {
>>> - /* no sense reconnecting to close a file that is
>>> - already closed */
>>> - if (!pTcon->need_reconnect) {
>>> - write_unlock(&cifs_file_list_lock);
>>> - timeout = 2;
>>> - while ((atomic_read(&pSMBFile->count) != 1)
>>
>> This portion of code which induces manual delay seems missing. While I'm
>> not sure about the practical significance of this code, explaining why
>> we do not need this in the change log would help?
>>
>
> This is the part of the code that I was particularly keen to eliminate.
>
> To my understanding the idea was that the code would set the closePend
> flag and then wait a "little while" until the count had gone to 0. The
> closePend flag would prevent the find_*_file interfaces from picking
> that filehandle, and the "little while" would help ensure that the
> current users of it would finish with it. Eventually though (in 2s) the
> code gives up and just destroys the cifsFileInfo anyway even though the
> refcount is still high.
>
> Needless to say, that's not a particularly robust scheme. It's possible
> the refcount went high just before the server went offline but before
> closePend was set. 2s may not be enough to prevent a use-after-free.
>
> It also had the problem that the actual users of the filehandle had to
> deal with the fact that the closePend flag could be set while they were
> using it. Then what do they do?
>
> The patch I'm proposing moves this to a real refcounted scheme where
> the last user closes out the filehandle on the server. Far simpler and
> more certain not to cause these sorts of problems.
Sounds good and makes sense to me.
--
Suresh Jayaraman
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
