patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor
@ 2017-09-20  9:59 Kuba Kozak
  2017-09-20 10:15 ` Burakov, Anatoly
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Kuba Kozak @ 2017-09-20  9:59 UTC (permalink / raw)
  To: anatoly.burakov; +Cc: dev, Kuba Kozak, patrick, stable

Add file descriptor value check before calling close() function.

Coverity issue: 141297
Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
Cc: patrick@patrickmacarthur.net
Cc: stable@dpdk.org

Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
index 7e8095c..c04f548 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
@@ -301,7 +301,8 @@ vfio_mp_sync_thread(void __rte_unused * arg)
 				vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
 			else
 				vfio_mp_sync_send_fd(conn_sock, fd);
-			close(fd);
+			if (fd != -1)
+				close(fd);
 			break;
 		case SOCKET_REQ_GROUP:
 			/* wait for group number */
-- 
2.7.4

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

* Re: [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor
  2017-09-20  9:59 [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor Kuba Kozak
@ 2017-09-20 10:15 ` Burakov, Anatoly
  2017-09-20 14:34 ` Patrick MacArthur
  2017-09-21 13:49 ` [dpdk-stable] [PATCH v2] " Michal Jastrzebski
  2 siblings, 0 replies; 9+ messages in thread
From: Burakov, Anatoly @ 2017-09-20 10:15 UTC (permalink / raw)
  To: Kuba Kozak; +Cc: dev, patrick, stable

On 20-Sep-17 10:59 AM, Kuba Kozak wrote:
> Add file descriptor value check before calling close() function.
> 
> Coverity issue: 141297
> Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
> Cc: patrick@patrickmacarthur.net
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
> ---

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

-- 
Thanks,
Anatoly

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

* Re: [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor
  2017-09-20  9:59 [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor Kuba Kozak
  2017-09-20 10:15 ` Burakov, Anatoly
@ 2017-09-20 14:34 ` Patrick MacArthur
  2017-09-20 14:39   ` Burakov, Anatoly
  2017-09-21 13:49 ` [dpdk-stable] [PATCH v2] " Michal Jastrzebski
  2 siblings, 1 reply; 9+ messages in thread
From: Patrick MacArthur @ 2017-09-20 14:34 UTC (permalink / raw)
  To: Kuba Kozak, anatoly.burakov; +Cc: dev, stable

On 09/20/2017 05:59 AM, Kuba Kozak wrote:
> Add file descriptor value check before calling close() function.
> 
> Coverity issue: 141297
> Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
> Cc: patrick@patrickmacarthur.net
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
> ---
>   lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
> index 7e8095c..c04f548 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
> @@ -301,7 +301,8 @@ vfio_mp_sync_thread(void __rte_unused * arg)
>   				vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
>   			else
>   				vfio_mp_sync_send_fd(conn_sock, fd);
> -			close(fd);
> +			if (fd != -1)
> +				close(fd);

IMHO this should be:

         if (fd >= 0)

What specifically is Coverity complaining about here? Is there a 
specific code path that leads to fd being -1 here?

Thanks,
Patrick MacArthur

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

* Re: [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor
  2017-09-20 14:34 ` Patrick MacArthur
@ 2017-09-20 14:39   ` Burakov, Anatoly
  2017-09-21  2:28     ` Patrick MacArthur
  0 siblings, 1 reply; 9+ messages in thread
From: Burakov, Anatoly @ 2017-09-20 14:39 UTC (permalink / raw)
  To: Patrick MacArthur, Kuba Kozak; +Cc: dev, stable

On 20-Sep-17 3:34 PM, Patrick MacArthur wrote:
> On 09/20/2017 05:59 AM, Kuba Kozak wrote:
>> Add file descriptor value check before calling close() function.
>>
>> Coverity issue: 141297
>> Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
>> Cc: patrick@patrickmacarthur.net
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c 
>> b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
>> index 7e8095c..c04f548 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
>> @@ -301,7 +301,8 @@ vfio_mp_sync_thread(void __rte_unused * arg)
>>                   vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
>>               else
>>                   vfio_mp_sync_send_fd(conn_sock, fd);
>> -            close(fd);
>> +            if (fd != -1)
>> +                close(fd);
> 
> IMHO this should be:
> 
>          if (fd >= 0)
> 
> What specifically is Coverity complaining about here? Is there a 
> specific code path that leads to fd being -1 here?
> 
Hi Patrick,

There's no way the fd will be 0 - the function we get the value from 
returns a valid fd, or a -1 in case of error. In this particular case, 
the "specific code path that leads to fd being -1" is when we can't get 
a container fd for some reason. I believe this is a very remote 
possibility as by the time we're spinning up the socket listening thread 
we're pretty sure we have a working VFIO container, but this is a valid 
fix nevertheless. Maybe having it >= 0 (or > 0, to be precise) would be 
cleaner, but it really makes no difference here.

-- 
Thanks,
Anatoly

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

* Re: [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor
  2017-09-20 14:39   ` Burakov, Anatoly
@ 2017-09-21  2:28     ` Patrick MacArthur
  2017-09-21  9:49       ` Kozak, KubaX
  0 siblings, 1 reply; 9+ messages in thread
From: Patrick MacArthur @ 2017-09-21  2:28 UTC (permalink / raw)
  To: Burakov, Anatoly, Kuba Kozak; +Cc: dev, stable

On 09/20/2017 10:39 AM, Burakov, Anatoly wrote:
> On 20-Sep-17 3:34 PM, Patrick MacArthur wrote:
>> On 09/20/2017 05:59 AM, Kuba Kozak wrote:
>>> Add file descriptor value check before calling close() function.
>>>
>>> Coverity issue: 141297
>>> Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
>>> Cc: patrick@patrickmacarthur.net
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
>>> ---
>>>   lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c 
>>> b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
>>> index 7e8095c..c04f548 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
>>> @@ -301,7 +301,8 @@ vfio_mp_sync_thread(void __rte_unused * arg)
>>>                   vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
>>>               else
>>>                   vfio_mp_sync_send_fd(conn_sock, fd);
>>> -            close(fd);
>>> +            if (fd != -1)
>>> +                close(fd);
>>
>> IMHO this should be:
>>
>>          if (fd >= 0)
>>
>> What specifically is Coverity complaining about here? Is there a 
>> specific code path that leads to fd being -1 here?
>>
> Hi Patrick,
> 
> There's no way the fd will be 0 - the function we get the value from 
> returns a valid fd, or a -1 in case of error. In this particular case, 
> the "specific code path that leads to fd being -1" is when we can't get 
> a container fd for some reason. I believe this is a very remote 
> possibility as by the time we're spinning up the socket listening thread 
> we're pretty sure we have a working VFIO container, but this is a valid 
> fix nevertheless. Maybe having it >= 0 (or > 0, to be precise) would be 
> cleaner, but it really makes no difference here.

The point of my suggestion is that it would catch *any* negative value 
for fd as opposed to just -1.

I agree 0 should never happen since it is stdin but it is technically a 
valid fd that could occur if the user program did close(STDIN_FILENO) 
for some reason.

I don't feel too strongly about it but feel like if we are going to fix 
what amounts to close() possibly returning EBADF we might as well fix it 
for all cases.

Thanks,
Patrick

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

* Re: [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor
  2017-09-21  2:28     ` Patrick MacArthur
@ 2017-09-21  9:49       ` Kozak, KubaX
  0 siblings, 0 replies; 9+ messages in thread
From: Kozak, KubaX @ 2017-09-21  9:49 UTC (permalink / raw)
  To: Patrick MacArthur, Burakov, Anatoly; +Cc: dev, stable

Hi,

Referring to Patrick suggestion I'll prepare patch with  (fd >= 0) condition.

Thanks,
Kuba

> -----Original Message-----
> From: Patrick MacArthur [mailto:patrick@patrickmacarthur.net]
> Sent: Thursday, September 21, 2017 04:28
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; Kozak, KubaX <kubax.kozak@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] vfio: fix close unchecked file descriptor
> 
> On 09/20/2017 10:39 AM, Burakov, Anatoly wrote:
> > On 20-Sep-17 3:34 PM, Patrick MacArthur wrote:
> >> On 09/20/2017 05:59 AM, Kuba Kozak wrote:
> >>> Add file descriptor value check before calling close() function.
> >>>
> >>> Coverity issue: 141297
> >>> Fixes: 811b6b25060f ("vfio: fix file descriptor leak in
> >>> multi-process")
> >>> Cc: patrick@patrickmacarthur.net
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
> >>> ---
> >>>   lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 3 ++-
> >>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
> >>> b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
> >>> index 7e8095c..c04f548 100644
> >>> --- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
> >>> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
> >>> @@ -301,7 +301,8 @@ vfio_mp_sync_thread(void __rte_unused * arg)
> >>>                   vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
> >>>               else
> >>>                   vfio_mp_sync_send_fd(conn_sock, fd);
> >>> -            close(fd);
> >>> +            if (fd != -1)
> >>> +                close(fd);
> >>
> >> IMHO this should be:
> >>
> >>          if (fd >= 0)
> >>
> >> What specifically is Coverity complaining about here? Is there a
> >> specific code path that leads to fd being -1 here?
> >>
> > Hi Patrick,
> >
> > There's no way the fd will be 0 - the function we get the value from
> > returns a valid fd, or a -1 in case of error. In this particular case,
> > the "specific code path that leads to fd being -1" is when we can't
> > get a container fd for some reason. I believe this is a very remote
> > possibility as by the time we're spinning up the socket listening
> > thread we're pretty sure we have a working VFIO container, but this is
> > a valid fix nevertheless. Maybe having it >= 0 (or > 0, to be precise)
> > would be cleaner, but it really makes no difference here.
> 
> The point of my suggestion is that it would catch *any* negative value for fd as opposed to just -1.
> 
> I agree 0 should never happen since it is stdin but it is technically a valid fd that could occur if the user
> program did close(STDIN_FILENO) for some reason.
> 
> I don't feel too strongly about it but feel like if we are going to fix what amounts to close() possibly
> returning EBADF we might as well fix it for all cases.
> 
> Thanks,
> Patrick

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

* [dpdk-stable] [PATCH v2] vfio: fix close unchecked file descriptor
  2017-09-20  9:59 [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor Kuba Kozak
  2017-09-20 10:15 ` Burakov, Anatoly
  2017-09-20 14:34 ` Patrick MacArthur
@ 2017-09-21 13:49 ` Michal Jastrzebski
  2017-09-21 16:07   ` Patrick MacArthur
  2 siblings, 1 reply; 9+ messages in thread
From: Michal Jastrzebski @ 2017-09-21 13:49 UTC (permalink / raw)
  To: anatoly.burakov; +Cc: dev, deepak.k.jain, Kuba Kozak, patrick, stable

From: Kuba Kozak <kubax.kozak@intel.com>

Add file descriptor value check before calling close() function.

Coverity issue: 141297
Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
Cc: patrick@patrickmacarthur.net
Cc: stable@dpdk.org

Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

---
v2:
	Change check condition
---
 lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
index 7e8095c..537beeb 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c
@@ -301,7 +301,8 @@ vfio_mp_sync_thread(void __rte_unused * arg)
 				vfio_mp_sync_send_request(conn_sock, SOCKET_ERR);
 			else
 				vfio_mp_sync_send_fd(conn_sock, fd);
-			close(fd);
+			if (fd >= 0)
+				close(fd);
 			break;
 		case SOCKET_REQ_GROUP:
 			/* wait for group number */
-- 
2.7.4

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

* Re: [dpdk-stable] [PATCH v2] vfio: fix close unchecked file descriptor
  2017-09-21 13:49 ` [dpdk-stable] [PATCH v2] " Michal Jastrzebski
@ 2017-09-21 16:07   ` Patrick MacArthur
  2017-10-05 21:42     ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
  0 siblings, 1 reply; 9+ messages in thread
From: Patrick MacArthur @ 2017-09-21 16:07 UTC (permalink / raw)
  To: Michal Jastrzebski, anatoly.burakov
  Cc: dev, deepak.k.jain, Kuba Kozak, stable

On 09/21/2017 09:49 AM, Michal Jastrzebski wrote:
> From: Kuba Kozak <kubax.kozak@intel.com>
> 
> Add file descriptor value check before calling close() function.
> 
> Coverity issue: 141297
> Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
> Cc: patrick@patrickmacarthur.net
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

FWIW:

Acked-by: Patrick MacArthur <patrick@patrickmacarthur.net>

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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] vfio: fix close unchecked file descriptor
  2017-09-21 16:07   ` Patrick MacArthur
@ 2017-10-05 21:42     ` Thomas Monjalon
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2017-10-05 21:42 UTC (permalink / raw)
  To: Kuba Kozak
  Cc: dev, Patrick MacArthur, Michal Jastrzebski, anatoly.burakov,
	deepak.k.jain, stable

21/09/2017 18:07, Patrick MacArthur:
> On 09/21/2017 09:49 AM, Michal Jastrzebski wrote:
> > From: Kuba Kozak <kubax.kozak@intel.com>
> > 
> > Add file descriptor value check before calling close() function.
> > 
> > Coverity issue: 141297
> > Fixes: 811b6b25060f ("vfio: fix file descriptor leak in multi-process")
> > Cc: patrick@patrickmacarthur.net
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Kuba Kozak <kubax.kozak@intel.com>
> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> FWIW:
> 
> Acked-by: Patrick MacArthur <patrick@patrickmacarthur.net>

Applied, thanks

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

end of thread, other threads:[~2017-10-05 21:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-20  9:59 [dpdk-stable] [PATCH] vfio: fix close unchecked file descriptor Kuba Kozak
2017-09-20 10:15 ` Burakov, Anatoly
2017-09-20 14:34 ` Patrick MacArthur
2017-09-20 14:39   ` Burakov, Anatoly
2017-09-21  2:28     ` Patrick MacArthur
2017-09-21  9:49       ` Kozak, KubaX
2017-09-21 13:49 ` [dpdk-stable] [PATCH v2] " Michal Jastrzebski
2017-09-21 16:07   ` Patrick MacArthur
2017-10-05 21:42     ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).