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

On Mon, May 10, 2010 at 9:00 AM, Shirish Pargaonkar
<[email protected]> 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.is assigned directly
> 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.
>
> Regards,
>
> Shirish
>

s/is assigned directly/is not assigned directly/