Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU

Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU

[Bruce Richardson]

On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
> Fix the time of check time of use warning in example code
> 
> Coverity issue: 350589 158663
> Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage sample")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jin Yu <jin.yu@intel.com>
> ---
>  examples/vhost_blk/vhost_blk.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/examples/vhost_blk/vhost_blk.c b/examples/vhost_blk/vhost_blk.c
> index 3182a488b..bcb4ebb0b 100644
> --- a/examples/vhost_blk/vhost_blk.c
> +++ b/examples/vhost_blk/vhost_blk.c
> @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
>  	}
>  	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path, ctrlr_name);
>  
> -	if (access(dev_pathname, F_OK) != -1) {
> -		if (unlink(dev_pathname) != 0)
> -			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> -				 dev_pathname);
> -	}
> +	unlink(dev_pathname);
>  

The original code did an exit if the delete failed, do you intend there to
be a behaviour change here? You can probably get the same behaviour if you
check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.

If not having the app exit on unlink failure is reasonable behaviour then
ignore this comment.

Regards,
/Bruce

[dpdk-stable] [PATCH] examples/vhost_blk: fix the TOCTOU

[Jin Yu]

Fix the time of check time of use warning in example code

Coverity issue: 350589 158663
Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage sample")
Cc: stable@dpdk.org

Signed-off-by: Jin Yu <jin.yu@intel.com>
---
 examples/vhost_blk/vhost_blk.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/examples/vhost_blk/vhost_blk.c b/examples/vhost_blk/vhost_blk.c
index 3182a488b..bcb4ebb0b 100644
--- a/examples/vhost_blk/vhost_blk.c
+++ b/examples/vhost_blk/vhost_blk.c
@@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
 	}
 	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path, ctrlr_name);
 
-	if (access(dev_pathname, F_OK) != -1) {
-		if (unlink(dev_pathname) != 0)
-			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
-				 dev_pathname);
-	}
+	unlink(dev_pathname);
 
 	if (rte_vhost_driver_register(dev_pathname, 0) != 0) {
 		fprintf(stderr, "socket %s already exists\n", dev_pathname);
@@ -1040,8 +1036,7 @@ signal_handler(__rte_unused int signum)
 {
 	struct vhost_blk_ctrlr *ctrlr;
 
-	if (access(dev_pathname, F_OK) == 0)
-		unlink(dev_pathname);
+	unlink(dev_pathname);
 
 	if (g_should_stop != -1) {
 		g_should_stop = 1;
-- 
2.17.2

Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU

[Yu, Jin]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, November 26, 2019 6:26 PM
> To: Yu, Jin <jin.yu@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
> 
> On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
> > Fix the time of check time of use warning in example code
> >
> > Coverity issue: 350589 158663
> > Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage
> > sample")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > ---
> >  examples/vhost_blk/vhost_blk.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/examples/vhost_blk/vhost_blk.c
> > b/examples/vhost_blk/vhost_blk.c index 3182a488b..bcb4ebb0b 100644
> > --- a/examples/vhost_blk/vhost_blk.c
> > +++ b/examples/vhost_blk/vhost_blk.c
> > @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
> >  	}
> >  	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
> > ctrlr_name);
> >
> > -	if (access(dev_pathname, F_OK) != -1) {
> > -		if (unlink(dev_pathname) != 0)
> > -			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> > -				 dev_pathname);
> > -	}
> > +	unlink(dev_pathname);
> >
> 
> The original code did an exit if the delete failed, do you intend there to be a
> behaviour change here? You can probably get the same behaviour if you
> check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
> 
> If not having the app exit on unlink failure is reasonable behaviour then
> ignore this comment.

I considered it. I think it's ok to ignore the errno of unlink failure. This code just want
to remove the file. There are two situations. The first one is that file doesn't exist the unlink
fails and it's ok to ignore. The second one is that unlink fails to remove file but the next bind() 
would fail too so I think it's ok to ignore too.
> 
> Regards,
> /Bruce

Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU

[Bruce Richardson]

On Wed, Nov 27, 2019 at 02:36:01AM +0000, Yu, Jin wrote:
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Tuesday, November 26, 2019 6:26 PM
> > To: Yu, Jin <jin.yu@intel.com>
> > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
> > <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
> > dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
> > 
> > On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
> > > Fix the time of check time of use warning in example code
> > >
> > > Coverity issue: 350589 158663
> > > Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage
> > > sample")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Jin Yu <jin.yu@intel.com>
> > > ---
> > >  examples/vhost_blk/vhost_blk.c | 9 ++-------
> > >  1 file changed, 2 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/examples/vhost_blk/vhost_blk.c
> > > b/examples/vhost_blk/vhost_blk.c index 3182a488b..bcb4ebb0b 100644
> > > --- a/examples/vhost_blk/vhost_blk.c
> > > +++ b/examples/vhost_blk/vhost_blk.c
> > > @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
> > >  	}
> > >  	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
> > > ctrlr_name);
> > >
> > > -	if (access(dev_pathname, F_OK) != -1) {
> > > -		if (unlink(dev_pathname) != 0)
> > > -			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> > > -				 dev_pathname);
> > > -	}
> > > +	unlink(dev_pathname);
> > >
> > 
> > The original code did an exit if the delete failed, do you intend there to be a
> > behaviour change here? You can probably get the same behaviour if you
> > check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
> > 
> > If not having the app exit on unlink failure is reasonable behaviour then
> > ignore this comment.
> 
> I considered it. I think it's ok to ignore the errno of unlink failure. This code just want
> to remove the file. There are two situations. The first one is that file doesn't exist the unlink
> fails and it's ok to ignore. The second one is that unlink fails to remove file but the next bind() 
> would fail too so I think it's ok to ignore too.
> > 
Ok, thanks for clarifying.

Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU

[Maxime Coquelin]

On 11/27/19 3:36 AM, Yu, Jin wrote:
>> -----Original Message-----
>> From: Bruce Richardson <bruce.richardson@intel.com>
>> Sent: Tuesday, November 26, 2019 6:26 PM
>> To: Yu, Jin <jin.yu@intel.com>
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>;
>> dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
>>
>> On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
>>> Fix the time of check time of use warning in example code
>>>
>>> Coverity issue: 350589 158663
>>> Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage
>>> sample")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jin Yu <jin.yu@intel.com>
>>> ---
>>>  examples/vhost_blk/vhost_blk.c | 9 ++-------
>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/examples/vhost_blk/vhost_blk.c
>>> b/examples/vhost_blk/vhost_blk.c index 3182a488b..bcb4ebb0b 100644
>>> --- a/examples/vhost_blk/vhost_blk.c
>>> +++ b/examples/vhost_blk/vhost_blk.c
>>> @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
>>>  	}
>>>  	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
>>> ctrlr_name);
>>>
>>> -	if (access(dev_pathname, F_OK) != -1) {
>>> -		if (unlink(dev_pathname) != 0)
>>> -			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
>>> -				 dev_pathname);
>>> -	}
>>> +	unlink(dev_pathname);
>>>
>>
>> The original code did an exit if the delete failed, do you intend there to be a
>> behaviour change here? You can probably get the same behaviour if you
>> check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
>>
>> If not having the app exit on unlink failure is reasonable behaviour then
>> ignore this comment.
> 
> I considered it. I think it's ok to ignore the errno of unlink failure. This code just want
> to remove the file. There are two situations. The first one is that file doesn't exist the unlink
> fails and it's ok to ignore. The second one is that unlink fails to remove file but the next bind() 
> would fail too so I think it's ok to ignore too.

That's fine by me, but please could you mention it in the commit
message?

>> Regards,
>> /Bruce
> 

[dpdk-stable] [PATCH v2] examples/vhost_blk: fix the TOCTOU

[Jin Yu]

Fix the time of check time of use warning in example code.
Ignore the errno of unlink failure. There are two situations.
The first one is that file doesn't exist the unlink fails and
it's ok to ignore. The second one is that unlink fails to remove
file but the next bind() would fail too.

Coverity issue: 350589 158663
Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage sample")
Cc: stable@dpdk.org

Signed-off-by: Jin Yu <jin.yu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
V2 - complement the commit message.
---
 examples/vhost_blk/vhost_blk.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/examples/vhost_blk/vhost_blk.c b/examples/vhost_blk/vhost_blk.c
index 3182a488b..bcb4ebb0b 100644
--- a/examples/vhost_blk/vhost_blk.c
+++ b/examples/vhost_blk/vhost_blk.c
@@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
 	}
 	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path, ctrlr_name);
 
-	if (access(dev_pathname, F_OK) != -1) {
-		if (unlink(dev_pathname) != 0)
-			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
-				 dev_pathname);
-	}
+	unlink(dev_pathname);
 
 	if (rte_vhost_driver_register(dev_pathname, 0) != 0) {
 		fprintf(stderr, "socket %s already exists\n", dev_pathname);
@@ -1040,8 +1036,7 @@ signal_handler(__rte_unused int signum)
 {
 	struct vhost_blk_ctrlr *ctrlr;
 
-	if (access(dev_pathname, F_OK) == 0)
-		unlink(dev_pathname);
+	unlink(dev_pathname);
 
 	if (g_should_stop != -1) {
 		g_should_stop = 1;
-- 
2.17.2

[dpdk-stable] [PATCH v2] examples/vhost_blk: fix the TOCTOU

[Jin Yu]

Fix the time of check time of use warning in example code.
Ignore the errno of unlink failure. There are two situations.
The first one is that file doesn't exist the unlink fails and
it's ok to ignore. The second one is that unlink fails to remove
file but the next bind() would fail too.

Coverity issue: 350589 158663
Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage sample")
Cc: stable@dpdk.org

Signed-off-by: Jin Yu <jin.yu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
V2 - complement the commit message.
---
 examples/vhost_blk/vhost_blk.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/examples/vhost_blk/vhost_blk.c b/examples/vhost_blk/vhost_blk.c
index e1036bf3a..74c82a900 100644
--- a/examples/vhost_blk/vhost_blk.c
+++ b/examples/vhost_blk/vhost_blk.c
@@ -994,11 +994,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
 	}
 	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path, ctrlr_name);
 
-	if (access(dev_pathname, F_OK) != -1) {
-		if (unlink(dev_pathname) != 0)
-			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
-				 dev_pathname);
-	}
+	unlink(dev_pathname);
 
 	if (rte_vhost_driver_register(dev_pathname, 0) != 0) {
 		fprintf(stderr, "socket %s already exists\n", dev_pathname);
@@ -1041,8 +1037,7 @@ signal_handler(__rte_unused int signum)
 {
 	struct vhost_blk_ctrlr *ctrlr;
 
-	if (access(dev_pathname, F_OK) == 0)
-		unlink(dev_pathname);
+	unlink(dev_pathname);
 
 	if (g_should_stop != -1) {
 		g_should_stop = 1;
-- 
2.17.2

Re: [dpdk-stable] [PATCH v2] examples/vhost_blk: fix the TOCTOU

[Maxime Coquelin]

On 2/11/20 10:33 AM, Jin Yu wrote:
> Fix the time of check time of use warning in example code.
> Ignore the errno of unlink failure. There are two situations.
> The first one is that file doesn't exist the unlink fails and
> it's ok to ignore. The second one is that unlink fails to remove
> file but the next bind() would fail too.
> 
> Coverity issue: 350589 158663
> Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage sample")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jin Yu <jin.yu@intel.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> V2 - complement the commit message.
> ---
>  examples/vhost_blk/vhost_blk.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

Applied to dpdk-next-virtio/master

Thanks,
Maxime