From: Longpeng <[email protected]>
The system will crash if we put an uninitialized iova_domain, this
could happen when an error occurs before initializing the iova_domain
in vdpasim_create().
BUG: kernel NULL pointer dereference, address: 0000000000000000
...
RIP: 0010:__cpuhp_state_remove_instance+0x96/0x1c0
...
Call Trace:
<TASK>
put_iova_domain+0x29/0x220
vdpasim_free+0xd1/0x120 [vdpa_sim]
vdpa_release_dev+0x21/0x40 [vdpa]
device_release+0x33/0x90
kobject_release+0x63/0x160
vdpasim_create+0x127/0x2a0 [vdpa_sim]
vdpasim_net_dev_add+0x7d/0xfe [vdpa_sim_net]
vdpa_nl_cmd_dev_add_set_doit+0xe1/0x1a0 [vdpa]
genl_family_rcv_msg_doit+0x112/0x140
genl_rcv_msg+0xdf/0x1d0
...
So we must make sure the iova_domain is already initialized before
put it.
In addition, we may get the following warning in this case:
WARNING: ... drivers/iommu/iova.c:344 iova_cache_put+0x58/0x70
So we must make sure the iova_cache_put() is invoked only if the
iova_cache_get() is already invoked. Let's fix it together.
Signed-off-by: Longpeng <[email protected]>
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 5f484fff8dbe..41b0cd17fcba 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -591,8 +591,11 @@ static void vdpasim_free(struct vdpa_device *vdpa)
vringh_kiov_cleanup(&vdpasim->vqs[i].in_iov);
}
- put_iova_domain(&vdpasim->iova);
- iova_cache_put();
+ if (vdpa_get_dma_dev(vdpa)) {
+ put_iova_domain(&vdpasim->iova);
+ iova_cache_put();
+ }
+
kvfree(vdpasim->buffer);
if (vdpasim->iommu)
vhost_iotlb_free(vdpasim->iommu);
--
2.27.0
On Mon, Nov 22, 2021 at 8:22 PM Longpeng(Mike) <[email protected]> wrote:
>
> From: Longpeng <[email protected]>
>
> The system will crash if we put an uninitialized iova_domain, this
> could happen when an error occurs before initializing the iova_domain
> in vdpasim_create().
>
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> ...
> RIP: 0010:__cpuhp_state_remove_instance+0x96/0x1c0
> ...
> Call Trace:
> <TASK>
> put_iova_domain+0x29/0x220
> vdpasim_free+0xd1/0x120 [vdpa_sim]
> vdpa_release_dev+0x21/0x40 [vdpa]
> device_release+0x33/0x90
> kobject_release+0x63/0x160
> vdpasim_create+0x127/0x2a0 [vdpa_sim]
> vdpasim_net_dev_add+0x7d/0xfe [vdpa_sim_net]
> vdpa_nl_cmd_dev_add_set_doit+0xe1/0x1a0 [vdpa]
> genl_family_rcv_msg_doit+0x112/0x140
> genl_rcv_msg+0xdf/0x1d0
> ...
>
> So we must make sure the iova_domain is already initialized before
> put it.
>
> In addition, we may get the following warning in this case:
> WARNING: ... drivers/iommu/iova.c:344 iova_cache_put+0x58/0x70
>
> So we must make sure the iova_cache_put() is invoked only if the
> iova_cache_get() is already invoked. Let's fix it together.
>
> Signed-off-by: Longpeng <[email protected]>
Acked-by: Jason Wang <[email protected]>
> ---
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index 5f484fff8dbe..41b0cd17fcba 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -591,8 +591,11 @@ static void vdpasim_free(struct vdpa_device *vdpa)
> vringh_kiov_cleanup(&vdpasim->vqs[i].in_iov);
> }
>
> - put_iova_domain(&vdpasim->iova);
> - iova_cache_put();
> + if (vdpa_get_dma_dev(vdpa)) {
> + put_iova_domain(&vdpasim->iova);
> + iova_cache_put();
> + }
> +
> kvfree(vdpasim->buffer);
> if (vdpasim->iommu)
> vhost_iotlb_free(vdpasim->iommu);
> --
> 2.27.0
>
> From: Longpeng(Mike) <[email protected]>
> Sent: Monday, November 22, 2021 5:52 PM
>
> From: Longpeng <[email protected]>
>
> The system will crash if we put an uninitialized iova_domain, this could
> happen when an error occurs before initializing the iova_domain in
> vdpasim_create().
>
> BUG: kernel NULL pointer dereference, address: 0000000000000000 ...
> RIP: 0010:__cpuhp_state_remove_instance+0x96/0x1c0
> ...
> Call Trace:
> <TASK>
> put_iova_domain+0x29/0x220
> vdpasim_free+0xd1/0x120 [vdpa_sim]
> vdpa_release_dev+0x21/0x40 [vdpa]
> device_release+0x33/0x90
> kobject_release+0x63/0x160
> vdpasim_create+0x127/0x2a0 [vdpa_sim]
> vdpasim_net_dev_add+0x7d/0xfe [vdpa_sim_net]
> vdpa_nl_cmd_dev_add_set_doit+0xe1/0x1a0 [vdpa]
> genl_family_rcv_msg_doit+0x112/0x140
> genl_rcv_msg+0xdf/0x1d0
> ...
>
> So we must make sure the iova_domain is already initialized before put it.
>
> In addition, we may get the following warning in this case:
> WARNING: ... drivers/iommu/iova.c:344 iova_cache_put+0x58/0x70
>
> So we must make sure the iova_cache_put() is invoked only if the
> iova_cache_get() is already invoked. Let's fix it together.
>
> Signed-off-by: Longpeng <[email protected]>
Can you please add the fixes tag here so that older kernels can take this fix?
On Tue, Nov 23, 2021 at 11:12 AM Parav Pandit <[email protected]> wrote:
>
>
>
> > From: Longpeng(Mike) <[email protected]>
> > Sent: Monday, November 22, 2021 5:52 PM
> >
> > From: Longpeng <[email protected]>
> >
> > The system will crash if we put an uninitialized iova_domain, this could
> > happen when an error occurs before initializing the iova_domain in
> > vdpasim_create().
> >
> > BUG: kernel NULL pointer dereference, address: 0000000000000000 ...
> > RIP: 0010:__cpuhp_state_remove_instance+0x96/0x1c0
> > ...
> > Call Trace:
> > <TASK>
> > put_iova_domain+0x29/0x220
> > vdpasim_free+0xd1/0x120 [vdpa_sim]
> > vdpa_release_dev+0x21/0x40 [vdpa]
> > device_release+0x33/0x90
> > kobject_release+0x63/0x160
> > vdpasim_create+0x127/0x2a0 [vdpa_sim]
> > vdpasim_net_dev_add+0x7d/0xfe [vdpa_sim_net]
> > vdpa_nl_cmd_dev_add_set_doit+0xe1/0x1a0 [vdpa]
> > genl_family_rcv_msg_doit+0x112/0x140
> > genl_rcv_msg+0xdf/0x1d0
> > ...
> >
> > So we must make sure the iova_domain is already initialized before put it.
> >
> > In addition, we may get the following warning in this case:
> > WARNING: ... drivers/iommu/iova.c:344 iova_cache_put+0x58/0x70
> >
> > So we must make sure the iova_cache_put() is invoked only if the
> > iova_cache_get() is already invoked. Let's fix it together.
> >
> > Signed-off-by: Longpeng <[email protected]>
>
> Can you please add the fixes tag here so that older kernels can take this fix?
>
I guess it's 4080fc106750 ("vdpa_sim: use iova module to allocate IOVA
addresses")
Thanks
> -----Original Message-----
> From: Jason Wang [mailto:[email protected]]
> Sent: Tuesday, November 23, 2021 12:13 PM
> To: Parav Pandit <[email protected]>
> Cc: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <[email protected]>; [email protected]; [email protected]; Max Gurtovoy
> <[email protected]>; [email protected];
> [email protected]; Gonglei (Arei) <[email protected]>
> Subject: Re: [PATCH] vdpa_sim: avoid putting an uninitialized iova_domain
>
> On Tue, Nov 23, 2021 at 11:12 AM Parav Pandit <[email protected]> wrote:
> >
> >
> >
> > > From: Longpeng(Mike) <[email protected]>
> > > Sent: Monday, November 22, 2021 5:52 PM
> > >
> > > From: Longpeng <[email protected]>
> > >
> > > The system will crash if we put an uninitialized iova_domain, this could
> > > happen when an error occurs before initializing the iova_domain in
> > > vdpasim_create().
> > >
> > > BUG: kernel NULL pointer dereference, address: 0000000000000000 ...
> > > RIP: 0010:__cpuhp_state_remove_instance+0x96/0x1c0
> > > ...
> > > Call Trace:
> > > <TASK>
> > > put_iova_domain+0x29/0x220
> > > vdpasim_free+0xd1/0x120 [vdpa_sim]
> > > vdpa_release_dev+0x21/0x40 [vdpa]
> > > device_release+0x33/0x90
> > > kobject_release+0x63/0x160
> > > vdpasim_create+0x127/0x2a0 [vdpa_sim]
> > > vdpasim_net_dev_add+0x7d/0xfe [vdpa_sim_net]
> > > vdpa_nl_cmd_dev_add_set_doit+0xe1/0x1a0 [vdpa]
> > > genl_family_rcv_msg_doit+0x112/0x140
> > > genl_rcv_msg+0xdf/0x1d0
> > > ...
> > >
> > > So we must make sure the iova_domain is already initialized before put it.
> > >
> > > In addition, we may get the following warning in this case:
> > > WARNING: ... drivers/iommu/iova.c:344 iova_cache_put+0x58/0x70
> > >
> > > So we must make sure the iova_cache_put() is invoked only if the
> > > iova_cache_get() is already invoked. Let's fix it together.
> > >
> > > Signed-off-by: Longpeng <[email protected]>
> >
> > Can you please add the fixes tag here so that older kernels can take this fix?
> >
>
> I guess it's 4080fc106750 ("vdpa_sim: use iova module to allocate IOVA
> addresses")
>
I think so. I'll add the fixes tag in V2, thanks.
> Thanks
On Mon, Nov 22, 2021 at 08:22:21PM +0800, Longpeng(Mike) wrote:
>From: Longpeng <[email protected]>
>
>The system will crash if we put an uninitialized iova_domain, this
>could happen when an error occurs before initializing the iova_domain
>in vdpasim_create().
>
>BUG: kernel NULL pointer dereference, address: 0000000000000000
>...
>RIP: 0010:__cpuhp_state_remove_instance+0x96/0x1c0
>...
>Call Trace:
> <TASK>
> put_iova_domain+0x29/0x220
> vdpasim_free+0xd1/0x120 [vdpa_sim]
> vdpa_release_dev+0x21/0x40 [vdpa]
> device_release+0x33/0x90
> kobject_release+0x63/0x160
> vdpasim_create+0x127/0x2a0 [vdpa_sim]
> vdpasim_net_dev_add+0x7d/0xfe [vdpa_sim_net]
> vdpa_nl_cmd_dev_add_set_doit+0xe1/0x1a0 [vdpa]
> genl_family_rcv_msg_doit+0x112/0x140
> genl_rcv_msg+0xdf/0x1d0
> ...
>
>So we must make sure the iova_domain is already initialized before
>put it.
>
>In addition, we may get the following warning in this case:
>WARNING: ... drivers/iommu/iova.c:344 iova_cache_put+0x58/0x70
>
>So we must make sure the iova_cache_put() is invoked only if the
>iova_cache_get() is already invoked. Let's fix it together.
>
>Signed-off-by: Longpeng <[email protected]>
>---
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Ooops, thanks for fixing this :-)
With the Fixes tag as suggested:
Reviewed-by: Stefano Garzarella <[email protected]>