vfio: free mp_reply msgs in failure cases

Message ID 156595762238.18723.10089009448135563310.stgit@jrharri1-skx
State New
Delegated to: Thomas Monjalon
Headers show
Series
  • vfio: free mp_reply msgs in failure cases
Related show

Checks

Context Check Description
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/iol-Compile-Testing success Compile Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Jim Harris Aug. 16, 2019, 12:13 p.m.
The code checks both rte_mp_request_sync() return
code and that the number of messages in the reply
equals 1.  If rte_mp_request_sync() succeeds but
there was more than one message, those messages
would get leaked.

Found via code review by Anatoly Burakov of patches
that used the vhost code as a template for using
rte_mp_request_sync().

Signed-off-by: Jim Harris <james.r.harris@intel.com>
---
 lib/librte_eal/linux/eal/eal_vfio.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Burakov, Anatoly Aug. 20, 2019, 1:13 p.m. | #1
On 16-Aug-19 1:13 PM, Jim Harris wrote:
> The code checks both rte_mp_request_sync() return
> code and that the number of messages in the reply
> equals 1.  If rte_mp_request_sync() succeeds but
> there was more than one message, those messages
> would get leaked.
> 
> Found via code review by Anatoly Burakov of patches
> that used the vhost code as a template for using
> rte_mp_request_sync().
> 
> Signed-off-by: Jim Harris <james.r.harris@intel.com>
> ---
>   lib/librte_eal/linux/eal/eal_vfio.c |   16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
> index 501c74f23..d9541b122 100644
> --- a/lib/librte_eal/linux/eal/eal_vfio.c
> +++ b/lib/librte_eal/linux/eal/eal_vfio.c
> @@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
>   	int vfio_group_fd;
>   	char filename[PATH_MAX];
>   	struct rte_mp_msg mp_req, *mp_rep;
> -	struct rte_mp_reply mp_reply;
> +	struct rte_mp_reply mp_reply = {0};
>   	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
>   	struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
>   
> @@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
>   			RTE_LOG(ERR, EAL, "  bad VFIO group fd\n");
>   			vfio_group_fd = 0;
>   		}
> -		free(mp_reply.msgs);
>   	}
>   
> +	free(mp_reply.msgs);

That's not quite correct. This fixes the problem of missing free() when 
nb_received mismatches, but this /adds/ a problem of doing an 
unnecessary free() when rte_mp_request_sync() returns -1. Same for other 
places, i believe.
Jim Harris Aug. 20, 2019, 1:16 p.m. | #2
> On Aug 20, 2019, at 6:13 AM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
> 
>> On 16-Aug-19 1:13 PM, Jim Harris wrote:
>> The code checks both rte_mp_request_sync() return
>> code and that the number of messages in the reply
>> equals 1.  If rte_mp_request_sync() succeeds but
>> there was more than one message, those messages
>> would get leaked.
>> Found via code review by Anatoly Burakov of patches
>> that used the vhost code as a template for using
>> rte_mp_request_sync().
>> Signed-off-by: Jim Harris <james.r.harris@intel.com>
>> ---
>>  lib/librte_eal/linux/eal/eal_vfio.c |   16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>> diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
>> index 501c74f23..d9541b122 100644
>> --- a/lib/librte_eal/linux/eal/eal_vfio.c
>> +++ b/lib/librte_eal/linux/eal/eal_vfio.c
>> @@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
>>      int vfio_group_fd;
>>      char filename[PATH_MAX];
>>      struct rte_mp_msg mp_req, *mp_rep;
>> -    struct rte_mp_reply mp_reply;
>> +    struct rte_mp_reply mp_reply = {0};
>>      struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
>>      struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
>>  @@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
>>              RTE_LOG(ERR, EAL, "  bad VFIO group fd\n");
>>              vfio_group_fd = 0;
>>          }
>> -        free(mp_reply.msgs);
>>      }
>>  +    free(mp_reply.msgs);
> 
> That's not quite correct. This fixes the problem of missing free() when nb_received mismatches, but this /adds/ a problem of doing an unnecessary free() when rte_mp_request_sync() returns -1. Same for other places, i believe.

This would just resolve to free(NULL) in the -1 case.

Jim

> 
> -- 
> Thanks,
> Anatoly
Burakov, Anatoly Aug. 20, 2019, 1:22 p.m. | #3
On 20-Aug-19 2:16 PM, Harris, James R wrote:
> 
> 
>> On Aug 20, 2019, at 6:13 AM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
>>
>>> On 16-Aug-19 1:13 PM, Jim Harris wrote:
>>> The code checks both rte_mp_request_sync() return
>>> code and that the number of messages in the reply
>>> equals 1.  If rte_mp_request_sync() succeeds but
>>> there was more than one message, those messages
>>> would get leaked.
>>> Found via code review by Anatoly Burakov of patches
>>> that used the vhost code as a template for using
>>> rte_mp_request_sync().
>>> Signed-off-by: Jim Harris <james.r.harris@intel.com>
>>> ---
>>>   lib/librte_eal/linux/eal/eal_vfio.c |   16 ++++++++--------
>>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>> diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
>>> index 501c74f23..d9541b122 100644
>>> --- a/lib/librte_eal/linux/eal/eal_vfio.c
>>> +++ b/lib/librte_eal/linux/eal/eal_vfio.c
>>> @@ -264,7 +264,7 @@ vfio_open_group_fd(int iommu_group_num)
>>>       int vfio_group_fd;
>>>       char filename[PATH_MAX];
>>>       struct rte_mp_msg mp_req, *mp_rep;
>>> -    struct rte_mp_reply mp_reply;
>>> +    struct rte_mp_reply mp_reply = {0};
>>>       struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
>>>       struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
>>>   @@ -320,9 +320,9 @@ vfio_open_group_fd(int iommu_group_num)
>>>               RTE_LOG(ERR, EAL, "  bad VFIO group fd\n");
>>>               vfio_group_fd = 0;
>>>           }
>>> -        free(mp_reply.msgs);
>>>       }
>>>   +    free(mp_reply.msgs);
>>
>> That's not quite correct. This fixes the problem of missing free() when nb_received mismatches, but this /adds/ a problem of doing an unnecessary free() when rte_mp_request_sync() returns -1. Same for other places, i believe.
> 
> This would just resolve to free(NULL) in the -1 case.
> 

Ah, you're right! We did fix that bug :)

With that in mind,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Patch

diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
index 501c74f23..d9541b122 100644
--- a/lib/librte_eal/linux/eal/eal_vfio.c
+++ b/lib/librte_eal/linux/eal/eal_vfio.c
@@ -264,7 +264,7 @@  vfio_open_group_fd(int iommu_group_num)
 	int vfio_group_fd;
 	char filename[PATH_MAX];
 	struct rte_mp_msg mp_req, *mp_rep;
-	struct rte_mp_reply mp_reply;
+	struct rte_mp_reply mp_reply = {0};
 	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
 	struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
 
@@ -320,9 +320,9 @@  vfio_open_group_fd(int iommu_group_num)
 			RTE_LOG(ERR, EAL, "  bad VFIO group fd\n");
 			vfio_group_fd = 0;
 		}
-		free(mp_reply.msgs);
 	}
 
+	free(mp_reply.msgs);
 	if (vfio_group_fd < 0)
 		RTE_LOG(ERR, EAL, "  cannot request group fd\n");
 	return vfio_group_fd;
@@ -554,7 +554,7 @@  static int
 vfio_sync_default_container(void)
 {
 	struct rte_mp_msg mp_req, *mp_rep;
-	struct rte_mp_reply mp_reply;
+	struct rte_mp_reply mp_reply = {0};
 	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
 	struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
 	int iommu_type_id;
@@ -584,8 +584,8 @@  vfio_sync_default_container(void)
 		p = (struct vfio_mp_param *)mp_rep->param;
 		if (p->result == SOCKET_OK)
 			iommu_type_id = p->iommu_type_id;
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 	if (iommu_type_id < 0) {
 		RTE_LOG(ERR, EAL, "Could not get IOMMU type for default container\n");
 		return -1;
@@ -1021,7 +1021,7 @@  int
 vfio_get_default_container_fd(void)
 {
 	struct rte_mp_msg mp_req, *mp_rep;
-	struct rte_mp_reply mp_reply;
+	struct rte_mp_reply mp_reply = {0};
 	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
 	struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
 
@@ -1049,9 +1049,9 @@  vfio_get_default_container_fd(void)
 			free(mp_reply.msgs);
 			return mp_rep->fds[0];
 		}
-		free(mp_reply.msgs);
 	}
 
+	free(mp_reply.msgs);
 	RTE_LOG(ERR, EAL, "  cannot request default container fd\n");
 	return -1;
 }
@@ -1127,7 +1127,7 @@  rte_vfio_get_container_fd(void)
 {
 	int ret, vfio_container_fd;
 	struct rte_mp_msg mp_req, *mp_rep;
-	struct rte_mp_reply mp_reply;
+	struct rte_mp_reply mp_reply = {0};
 	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
 	struct vfio_mp_param *p = (struct vfio_mp_param *)mp_req.param;
 
@@ -1181,9 +1181,9 @@  rte_vfio_get_container_fd(void)
 			free(mp_reply.msgs);
 			return vfio_container_fd;
 		}
-		free(mp_reply.msgs);
 	}
 
+	free(mp_reply.msgs);
 	RTE_LOG(ERR, EAL, "  cannot request container fd\n");
 	return -1;
 }