During the stop copy phase of live migration, the driver allocates
a memory for the migrated data to save the data.
When an exception occurs when the driver reads device data, the driver
will report an error to qemu and exit the current migration state.
But this memory is not released, which will lead to a memory
leak problem.
So we need to add a memory release operation.
Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Longfang Liu <liulongfang@huawei.com>
---
drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index ea762e28c1cc..8fd68af2ed5f 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -828,15 +828,15 @@ hisi_acc_vf_stop_copy(struct hisi_acc_vf_core_device *hisi_acc_vdev)
return ERR_PTR(err);
}
- stream_open(migf->filp->f_inode, migf->filp);
- mutex_init(&migf->lock);
-
ret = vf_qm_state_save(hisi_acc_vdev, migf);
if (ret) {
- fput(migf->filp);
+ kfree(migf);
return ERR_PTR(ret);
}
+ stream_open(migf->filp->f_inode, migf->filp);
+ mutex_init(&migf->lock);
+
return migf;
}
--
2.33.0On Thu, 15 Sep 2022 09:31:53 +0800
Longfang Liu <liulongfang@huawei.com> wrote:
> During the stop copy phase of live migration, the driver allocates
> a memory for the migrated data to save the data.
>
> When an exception occurs when the driver reads device data, the driver
> will report an error to qemu and exit the current migration state.
> But this memory is not released, which will lead to a memory
> leak problem.
>
> So we need to add a memory release operation.
>
> Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Longfang Liu <liulongfang@huawei.com>
> ---
> drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index ea762e28c1cc..8fd68af2ed5f 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -828,15 +828,15 @@ hisi_acc_vf_stop_copy(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> return ERR_PTR(err);
> }
>
> - stream_open(migf->filp->f_inode, migf->filp);
> - mutex_init(&migf->lock);
> -
> ret = vf_qm_state_save(hisi_acc_vdev, migf);
> if (ret) {
> - fput(migf->filp);
Sorry, why did this fput() get removed? Thanks,
Alex
> + kfree(migf);
> return ERR_PTR(ret);
> }
>
> + stream_open(migf->filp->f_inode, migf->filp);
> + mutex_init(&migf->lock);
> +
> return migf;
> }
>
On Tue, Sep 20, 2022 at 10:34:43AM -0600, Alex Williamson wrote:
> On Thu, 15 Sep 2022 09:31:53 +0800
> Longfang Liu <liulongfang@huawei.com> wrote:
>
> > During the stop copy phase of live migration, the driver allocates
> > a memory for the migrated data to save the data.
> >
> > When an exception occurs when the driver reads device data, the driver
> > will report an error to qemu and exit the current migration state.
> > But this memory is not released, which will lead to a memory
> > leak problem.
Why isn't it released? The fput() releases it:
static int hisi_acc_vf_release_file(struct inode *inode, struct file *filp)
{
struct hisi_acc_vf_migration_file *migf = filp->private_data;
hisi_acc_vf_disable_fd(migf);
mutex_destroy(&migf->lock);
kfree(migf);
^^^^^^^^^^
This patch looks wrong to me.
Jason
> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgg@nvidia.com]
> Sent: 20 September 2022 17:38
> To: Alex Williamson <alex.williamson@redhat.com>
> Cc: liulongfang <liulongfang@huawei.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; cohuck@redhat.com;
> linux-kernel@vger.kernel.org; linuxarm@openeuler.org
> Subject: Re: [PATCH 1/5] hisi_acc_vfio_pci: Fixes a memory leak bug
>
> On Tue, Sep 20, 2022 at 10:34:43AM -0600, Alex Williamson wrote:
> > On Thu, 15 Sep 2022 09:31:53 +0800
> > Longfang Liu <liulongfang@huawei.com> wrote:
> >
> > > During the stop copy phase of live migration, the driver allocates a
> > > memory for the migrated data to save the data.
> > >
> > > When an exception occurs when the driver reads device data, the
> > > driver will report an error to qemu and exit the current migration state.
> > > But this memory is not released, which will lead to a memory leak
> > > problem.
>
> Why isn't it released? The fput() releases it:
>
> static int hisi_acc_vf_release_file(struct inode *inode, struct file *filp) {
> struct hisi_acc_vf_migration_file *migf = filp->private_data;
>
> hisi_acc_vf_disable_fd(migf);
> mutex_destroy(&migf->lock);
> kfree(migf);
> ^^^^^^^^^^
>
> This patch looks wrong to me.
That's right. Missed that. Sorry of the oversight.
Thanks,
Shameer
On 2022/9/21 1:03, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: Jason Gunthorpe [mailto:jgg@nvidia.com]
>> Sent: 20 September 2022 17:38
>> To: Alex Williamson <alex.williamson@redhat.com>
>> Cc: liulongfang <liulongfang@huawei.com>; Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; cohuck@redhat.com;
>> linux-kernel@vger.kernel.org; linuxarm@openeuler.org
>> Subject: Re: [PATCH 1/5] hisi_acc_vfio_pci: Fixes a memory leak bug
>>
>> On Tue, Sep 20, 2022 at 10:34:43AM -0600, Alex Williamson wrote:
>>> On Thu, 15 Sep 2022 09:31:53 +0800
>>> Longfang Liu <liulongfang@huawei.com> wrote:
>>>
>>>> During the stop copy phase of live migration, the driver allocates a
>>>> memory for the migrated data to save the data.
>>>>
>>>> When an exception occurs when the driver reads device data, the
>>>> driver will report an error to qemu and exit the current migration state.
>>>> But this memory is not released, which will lead to a memory leak
>>>> problem.
>>
>> Why isn't it released? The fput() releases it:
>>
>> static int hisi_acc_vf_release_file(struct inode *inode, struct file *filp) {
>> struct hisi_acc_vf_migration_file *migf = filp->private_data;
>>
>> hisi_acc_vf_disable_fd(migf);
>> mutex_destroy(&migf->lock);
>> kfree(migf);
>> ^^^^^^^^^^
>>
>> This patch looks wrong to me.
>
> That's right. Missed that. Sorry of the oversight.
>
Yes, fput will call release in ops of file, here will call hisi_acc_vf_release_file
to complete the release operation of migf, so this patch is unnecessary.
But there is another place that needs to be modified:
hisi_acc_vf_disable_fd in hisi_acc_vf_disable_fds is not needed,
because it will have an fput next. Is this correct?
> Thanks,
> Shameer
>
> .
Thanks,
Longfang.
>
> -----Original Message-----
> From: liulongfang
> Sent: 21 September 2022 04:13
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Jason Gunthorpe <jgg@nvidia.com>; Alex Williamson
> <alex.williamson@redhat.com>
> Cc: cohuck@redhat.com; linux-kernel@vger.kernel.org;
> linuxarm@openeuler.org
> Subject: Re: [PATCH 1/5] hisi_acc_vfio_pci: Fixes a memory leak bug
>
> On 2022/9/21 1:03, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jason Gunthorpe [mailto:jgg@nvidia.com]
> >> Sent: 20 September 2022 17:38
> >> To: Alex Williamson <alex.williamson@redhat.com>
> >> Cc: liulongfang <liulongfang@huawei.com>; Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@huawei.com>; cohuck@redhat.com;
> >> linux-kernel@vger.kernel.org; linuxarm@openeuler.org
> >> Subject: Re: [PATCH 1/5] hisi_acc_vfio_pci: Fixes a memory leak bug
> >>
> >> On Tue, Sep 20, 2022 at 10:34:43AM -0600, Alex Williamson wrote:
> >>> On Thu, 15 Sep 2022 09:31:53 +0800
> >>> Longfang Liu <liulongfang@huawei.com> wrote:
> >>>
> >>>> During the stop copy phase of live migration, the driver allocates a
> >>>> memory for the migrated data to save the data.
> >>>>
> >>>> When an exception occurs when the driver reads device data, the
> >>>> driver will report an error to qemu and exit the current migration state.
> >>>> But this memory is not released, which will lead to a memory leak
> >>>> problem.
> >>
> >> Why isn't it released? The fput() releases it:
> >>
> >> static int hisi_acc_vf_release_file(struct inode *inode, struct file *filp) {
> >> struct hisi_acc_vf_migration_file *migf = filp->private_data;
> >>
> >> hisi_acc_vf_disable_fd(migf);
> >> mutex_destroy(&migf->lock);
> >> kfree(migf);
> >> ^^^^^^^^^^
> >>
> >> This patch looks wrong to me.
> >
> > That's right. Missed that. Sorry of the oversight.
> >
> Yes, fput will call release in ops of file, here will call hisi_acc_vf_release_file
> to complete the release operation of migf, so this patch is unnecessary.
>
> But there is another place that needs to be modified:
> hisi_acc_vf_disable_fd in hisi_acc_vf_disable_fds is not needed,
> because it will have an fput next. Is this correct?
I don't think that is correct either. fput() decrements ref count and
will only call release() if the count is zero. We have an explicit get_file()
for the hisi_acc_vf_disable_fds(). Isn't it?
Thanks,
Shameer
> > Thanks,
> > Shameer
> >
> > .
> Thanks,
> Longfang.
> >
On 2022/9/21 15:27, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: liulongfang
>> Sent: 21 September 2022 04:13
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Jason Gunthorpe <jgg@nvidia.com>; Alex Williamson
>> <alex.williamson@redhat.com>
>> Cc: cohuck@redhat.com; linux-kernel@vger.kernel.org;
>> linuxarm@openeuler.org
>> Subject: Re: [PATCH 1/5] hisi_acc_vfio_pci: Fixes a memory leak bug
>>
>> On 2022/9/21 1:03, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jason Gunthorpe [mailto:jgg@nvidia.com]
>>>> Sent: 20 September 2022 17:38
>>>> To: Alex Williamson <alex.williamson@redhat.com>
>>>> Cc: liulongfang <liulongfang@huawei.com>; Shameerali Kolothum Thodi
>>>> <shameerali.kolothum.thodi@huawei.com>; cohuck@redhat.com;
>>>> linux-kernel@vger.kernel.org; linuxarm@openeuler.org
>>>> Subject: Re: [PATCH 1/5] hisi_acc_vfio_pci: Fixes a memory leak bug
>>>>
>>>> On Tue, Sep 20, 2022 at 10:34:43AM -0600, Alex Williamson wrote:
>>>>> On Thu, 15 Sep 2022 09:31:53 +0800
>>>>> Longfang Liu <liulongfang@huawei.com> wrote:
>>>>>
>>>>>> During the stop copy phase of live migration, the driver allocates a
>>>>>> memory for the migrated data to save the data.
>>>>>>
>>>>>> When an exception occurs when the driver reads device data, the
>>>>>> driver will report an error to qemu and exit the current migration state.
>>>>>> But this memory is not released, which will lead to a memory leak
>>>>>> problem.
>>>>
>>>> Why isn't it released? The fput() releases it:
>>>>
>>>> static int hisi_acc_vf_release_file(struct inode *inode, struct file *filp) {
>>>> struct hisi_acc_vf_migration_file *migf = filp->private_data;
>>>>
>>>> hisi_acc_vf_disable_fd(migf);
>>>> mutex_destroy(&migf->lock);
>>>> kfree(migf);
>>>> ^^^^^^^^^^
>>>>
>>>> This patch looks wrong to me.
>>>
>>> That's right. Missed that. Sorry of the oversight.
>>>
>> Yes, fput will call release in ops of file, here will call hisi_acc_vf_release_file
>> to complete the release operation of migf, so this patch is unnecessary.
>>
>> But there is another place that needs to be modified:
>> hisi_acc_vf_disable_fd in hisi_acc_vf_disable_fds is not needed,
>> because it will have an fput next. Is this correct?
>
> I don't think that is correct either. fput() decrements ref count and
> will only call release() if the count is zero. We have an explicit get_file()
> for the hisi_acc_vf_disable_fds(). Isn't it?
>
> Thanks,
> Shameer
>
>
OK! These are not necessary to be modified, so there is no need to add them to
the patchset. I am going to modify the patchset and send it out in the next
version.
>
>>> Thanks,
>>> Shameer
>>>
>>> .
>> Thanks,
>> Longfang.
>>>
> .
Thanks,
Longfang.
>
© 2016 - 2026 Red Hat, Inc.