Re: [RFC][PATCH] cifs: fix potential cifsFileInfo struct leaks

On Mon, May 10, 2010 at 9:10 AM, Suresh Jayaraman <[email protected]> wrote:
> On 05/10/2010 07:30 PM, Shirish Pargaonkar wrote:
>> On Mon, May 10, 2010 at 6:51 AM, Suresh Jayaraman <[email protected]> wrote:
>>> cifs_new_fileinfo() allocates and return a pointer to a struct cifsFileInfo
>>> based on which file->private_data is set. Apparently, we seem to be leaking
>>> cifsFileInfo structs when called from cifs_posix_open and cifs_create as we
>>> ignore the return value.
>>>
>>> IIUC, if we are unable to set file->private_data, then there seem to be little
>>> use in calling cifs_new_fileinfo(). Since the file pointer being passed is
>>> always NULL when called from both the callers, remove the call to
>>> cifs_new_fileinfo which seems to be of no use.
>>>
>>> This patch needs careful review.
>>>
>>> Signed-off-by: Suresh Jayaraman <[email protected]>
>>> ---
>>> �fs/cifs/dir.c | � 10 ++--------
>>> �1 files changed, 2 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>>> index d791d07..046f0a7 100644
>>> --- a/fs/cifs/dir.c
>>> +++ b/fs/cifs/dir.c
>>> @@ -251,9 +251,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>>> � � � � � � � �cifs_fattr_to_inode(*pinode, &fattr);
>>> � � � �}
>>>
>>> - � � � if (mnt)
>>> - � � � � � � � cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
>>> -
>>> �posix_open_ret:
>>> � � � �kfree(presp_data);
>>> � � � �return rc;
>>> @@ -462,13 +459,10 @@ cifs_create_set_dentry:
>>> � � � � � � � �cFYI(1, "Create worked, get_inode_info failed rc = %d", rc);
>>>
>>> � � � �/* nfsd case - nfs srv does not set nd */
>>> - � � � if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN))) {
>>> + � � � if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN)))
>>> � � � � � � � �/* mknod case - do not leave file open */
>>> � � � � � � � �CIFSSMBClose(xid, tcon, fileHandle);
>>> - � � � } else if (!(posix_create) && (newinode)) {
>>> - � � � � � � � � � � � cifs_new_fileinfo(newinode, fileHandle, NULL,
>>> - � � � � � � � � � � � � � � � � � � � � � � � nd->path.mnt, oflags);
>>> - � � � }
>>> +
>>> �cifs_create_out:
>>> � � � �kfree(buf);
>>> � � � �kfree(full_path);
>>> _______________________________________________
>>> linux-cifs-client mailing list
>>> [email protected]
>>> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>>
>>
>> We should not do this.
>> In case of cifs_create/cifs_lookup, even though the retrun value of
>> cifs_new_fileinfo is assigned directly,
>> because we do not have file structure at that point, pcifsFileInfo is
>> stuck on the openFileList and
>> pcifsFileInfo gets discovered in cifs_open is assigned to file's
>> private data in cifs_fill_filedata.
>>
>
> Yes, I see what you explained, quite hideous.. I'll roll up a doc patch
> explaining it a bit more clear..
>
> But I'm not sure whether there is a race window that exists, though. I'm
> trying to track a infamous "VFS: Busy inodes after unmount" error..
>
>
>
> Thanks,
>
> --
> Suresh Jayaraman
>

I am not sure busy inode means inode leak, I think these inodes get cleaned
up anyway, albeit with this warning.
_______________________________________________
linux-cifs-client mailing list
[email protected]
https://lists.samba.org/mailman/listinfo/linux-cifs-client