DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
* [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases
@ 2019-08-16 12:13 Jim Harris
  2019-08-20 13:13 ` Burakov, Anatoly
  0 siblings, 1 reply; 4+ messages in thread
From: Jim Harris @ 2019-08-16 12:13 UTC (permalink / raw)
  To: dev, anatoly.burakov

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);
 	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;
 }


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases
  2019-08-16 12:13 [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases Jim Harris
@ 2019-08-20 13:13 ` Burakov, Anatoly
  2019-08-20 13:16   ` Harris, James R
  0 siblings, 1 reply; 4+ messages in thread
From: Burakov, Anatoly @ 2019-08-20 13:13 UTC (permalink / raw)
  To: Jim Harris, dev

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.

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases
  2019-08-20 13:13 ` Burakov, Anatoly
@ 2019-08-20 13:16   ` Harris, James R
  2019-08-20 13:22     ` Burakov, Anatoly
  0 siblings, 1 reply; 4+ messages in thread
From: Harris, James R @ 2019-08-20 13:16 UTC (permalink / raw)
  To: Burakov, Anatoly; +Cc: dev



> 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases
  2019-08-20 13:16   ` Harris, James R
@ 2019-08-20 13:22     ` Burakov, Anatoly
  0 siblings, 0 replies; 4+ messages in thread
From: Burakov, Anatoly @ 2019-08-20 13:22 UTC (permalink / raw)
  To: Harris, James R; +Cc: dev

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>

-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16 12:13 [dpdk-dev] [PATCH] vfio: free mp_reply msgs in failure cases Jim Harris
2019-08-20 13:13 ` Burakov, Anatoly
2019-08-20 13:16   ` Harris, James R
2019-08-20 13:22     ` Burakov, Anatoly

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox