[RFC] cifs: fix potential cifsFileInfo struct leaks

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@suse.de>
---
 fs/cifs/dir.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

On Mon, May 10, 2010 at 6:51 AM, Suresh Jayaraman <sjayaraman@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@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);
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> 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.

Regards,

Shirish

On Mon, May 10, 2010 at 9:00 AM, Shirish Pargaonkar
<shirishpargaonkar@gmail.com> wrote:
> On Mon, May 10, 2010 at 6:51 AM, Suresh Jayaraman <sjayaraman@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@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);
>> _______________________________________________
>> linux-cifs-client mailing list
>> linux-cifs-client@lists.samba.org
>> 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/

On 05/10/2010 07:30 PM, Shirish Pargaonkar wrote:
> On Mon, May 10, 2010 at 6:51 AM, Suresh Jayaraman <sjayaraman@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@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);
>> _______________________________________________
>> linux-cifs-client mailing list
>> linux-cifs-client@lists.samba.org
>> 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,

On Mon, May 10, 2010 at 9:10 AM, Suresh Jayaraman <sjayaraman@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@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@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);
>>> _______________________________________________
>>> linux-cifs-client mailing list
>>> linux-cifs-client@lists.samba.org
>>> 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.

On 05/10/2010 08:00 PM, Shirish Pargaonkar wrote:
> On Mon, May 10, 2010 at 9:10 AM, Suresh Jayaraman <sjayaraman@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@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@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,