[dpdk-dev] net/virtio: Fix crash in virtio_dev_free_mbufs

Message ID 20180203145523.28013-1-dharton@cisco.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

David Harton Feb. 3, 2018, 2:55 p.m. UTC
  virtio_dev_free_mbufs was recently modified to free the
virtqueues but failed to check whether the array was
allocated.  Added a check to ensure vqs was non-null.

Fixes: bdb32afbb610 ("net/virtio: rationalize queue flushing")
Cc: olivier.matz@6wind.com

Signed-off-by: David C Harton <dharton@cisco.com>
---
 drivers/net/virtio/virtio_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)
  

Comments

Olivier Matz Feb. 5, 2018, 8:17 a.m. UTC | #1
On Sat, Feb 03, 2018 at 09:55:23AM -0500, David C Harton wrote:
> virtio_dev_free_mbufs was recently modified to free the
> virtqueues but failed to check whether the array was
> allocated.  Added a check to ensure vqs was non-null.
> 
> Fixes: bdb32afbb610 ("net/virtio: rationalize queue flushing")
> Cc: olivier.matz@6wind.com
> 
> Signed-off-by: David C Harton <dharton@cisco.com>
> ---

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>

Thank you for this fix.
Out of curiosity, did you see the problem by code review or is there an
easy way to reproduce the crash?
  
Maxime Coquelin Feb. 5, 2018, 8:44 a.m. UTC | #2
On 02/03/2018 03:55 PM, David C Harton wrote:
> virtio_dev_free_mbufs was recently modified to free the
> virtqueues but failed to check whether the array was
> allocated.  Added a check to ensure vqs was non-null.
> 
> Fixes: bdb32afbb610 ("net/virtio: rationalize queue flushing")
> Cc: olivier.matz@6wind.com
> 
> Signed-off-by: David C Harton <dharton@cisco.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 2082d6a..884f74a 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1975,6 +1975,9 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
>   	struct rte_mbuf *buf;
>   	int queue_type;
>   
> +	if (hw->vqs == NULL)
> +		return;
> +
>   	for (i = 0; i < nr_vq; i++) {
>   		vq = hw->vqs[i];
>   		if (!vq)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
  
Ferruh Yigit Feb. 5, 2018, 6:52 p.m. UTC | #3
On 2/3/2018 2:55 PM, David C Harton wrote:
> virtio_dev_free_mbufs was recently modified to free the
> virtqueues but failed to check whether the array was
> allocated.  Added a check to ensure vqs was non-null.
> 
> Fixes: bdb32afbb610 ("net/virtio: rationalize queue flushing")
> Cc: olivier.matz@6wind.com
> 
> Signed-off-by: David C Harton <dharton@cisco.com>

[1] Signed-off-by: David Harton <dharton@cisco.com>

Reviewed-by: Olivier Matz <olivier.matz@6wind.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied to dpdk-next-net/master, thanks.


[1]
Hi David,

Since your previous commits has "David Harton" name, to prevent seen as a new
person, I kept same name, fyi.
  

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 2082d6a..884f74a 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1975,6 +1975,9 @@  static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 	struct rte_mbuf *buf;
 	int queue_type;
 
+	if (hw->vqs == NULL)
+		return;
+
 	for (i = 0; i < nr_vq; i++) {
 		vq = hw->vqs[i];
 		if (!vq)