[linux-cifs-client] [RFC][PATCH] cifs: fix potential cifsFileInfo struct leaks

[Suresh Jayaraman]

On 05/10/2010 08:00 PM, Shirish Pargaonkar wrote:
> On Mon, May 10, 2010 at 9:10 AM, Suresh Jayaraman <sjayaraman at suse.de> wrote:
>> On 05/10/2010 07:30 PM, Shirish Pargaonkar wrote:
>>> On Mon, May 10, 2010 at 6:51 AM, Suresh Jayaraman <sjayaraman at suse.de> 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 <sjayaraman at suse.de>
>>>> ---
>>>> �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);

>>>
>>> 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..
>>
> 
> I am not sure busy inode means inode leak, I think these inodes get cleaned
> up anyway, albeit with this warning.


To my knowledge, the "VFS: Busy inodes after unmount" usually means the
filesystem is unmounted and thus superblock is freed, but the inodes are
still hanging around.

There is a good chance that the system would crash when the kernel tries
to reference any of these inodes as it try to use the freed superblock
pointer. So, they could mean some serious issues.

However, the VFS Busy errors that we see are highly non-reproducible
(ran fsstress for 8 hours, no errors). I'm suspecting that it could be a
case where some application on cifs mounts caught an unexpected signal
or something like that..


Thanks,

-- 
Suresh Jayaraman