[dpdk-dev] [PATCH] net/tap: ipc add check for number of messages received

[dpdk-dev] [PATCH] net/tap: ipc add check for number of messages received

[Herakliusz Lipiec]

A sucessfull call to rte_mp_request_sync does not guarantee that there
are valid messages in the buffer, and this should be checked for before
accessing data in the message.

Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..a619a8850 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	request.len_param = sizeof(*request_param);
 	/* Send request and receive reply */
 	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0) {
+	if (ret < 0 || replies.n_receieved != 1) {
 		TAP_LOG(ERR, "Failed to request queues from primary: %d",
 			rte_errno);
 		return -1;
-- 
2.17.2

[dpdk-dev] [PATCH] net/tap: ipc add check for number of messages received

[Herakliusz Lipiec]

A sucessfull call to rte_mp_request_sync does not guarantee that there
are valid messages in the buffer, and this should be checked for before
accessing data in the message.

Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..a619a8850 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	request.len_param = sizeof(*request_param);
 	/* Send request and receive reply */
 	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0) {
+	if (ret < 0 || replies.n_receieved != 1) {
 		TAP_LOG(ERR, "Failed to request queues from primary: %d",
 			rte_errno);
 		return -1;
-- 
2.17.2

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/tap: ipc add check for number of messages received

[Ferruh Yigit]

On 4/18/2019 6:19 PM, Herakliusz Lipiec wrote:
> A sucessfull call to rte_mp_request_sync does not guarantee that there
> are valid messages in the buffer, and this should be checked for before
> accessing data in the message.
> 
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index e9fda8cf6..a619a8850 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
>  	request.len_param = sizeof(*request_param);
>  	/* Send request and receive reply */
>  	ret = rte_mp_request_sync(&request, &replies, &timeout);
> -	if (ret < 0) {
> +	if (ret < 0 || replies.n_receieved != 1) {

The API documentation says:

||   * @return



||   *  - On success, return 0.



||   *  - On failure, return -1, and the reason will be stored in rte_errno.

So if the API returns 0, why the reply is not valid, also if reply is not valid
how can you rely on a value in 'replies'

What do you think updating the 'rte_mp_request_sync()' API to return error
whenever the reply is not valid?

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/tap: ipc add check for number of messages received

[Ferruh Yigit]

On 4/18/2019 6:19 PM, Herakliusz Lipiec wrote:
> A sucessfull call to rte_mp_request_sync does not guarantee that there
> are valid messages in the buffer, and this should be checked for before
> accessing data in the message.
> 
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index e9fda8cf6..a619a8850 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
>  	request.len_param = sizeof(*request_param);
>  	/* Send request and receive reply */
>  	ret = rte_mp_request_sync(&request, &replies, &timeout);
> -	if (ret < 0) {
> +	if (ret < 0 || replies.n_receieved != 1) {

The API documentation says:

||   * @return



||   *  - On success, return 0.



||   *  - On failure, return -1, and the reason will be stored in rte_errno.

So if the API returns 0, why the reply is not valid, also if reply is not valid
how can you rely on a value in 'replies'

What do you think updating the 'rte_mp_request_sync()' API to return error
whenever the reply is not valid?

[dpdk-dev] [PATCH v2] net/tap: ipc add check for number of messages received

[Herakliusz Lipiec]

A sucessfull call to rte_mp_request_sync does not guarantee that there
are any messages in the buffer, and this should be checked for before
accessing data in the message. Buffer can be empty if IPC is disabled or
if we deciede to ignore replies.

Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..7f74b5dc9 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	request.len_param = sizeof(*request_param);
 	/* Send request and receive reply */
 	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0) {
+	if (ret < 0 || replies.nb_received != 1) {
 		TAP_LOG(ERR, "Failed to request queues from primary: %d",
 			rte_errno);
 		return -1;
-- 
2.17.2

[dpdk-dev] [PATCH v2] net/tap: ipc add check for number of messages received

[Herakliusz Lipiec]

A sucessfull call to rte_mp_request_sync does not guarantee that there
are any messages in the buffer, and this should be checked for before
accessing data in the message. Buffer can be empty if IPC is disabled or
if we deciede to ignore replies.

Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..7f74b5dc9 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	request.len_param = sizeof(*request_param);
 	/* Send request and receive reply */
 	ret = rte_mp_request_sync(&request, &replies, &timeout);
-	if (ret < 0) {
+	if (ret < 0 || replies.nb_received != 1) {
 		TAP_LOG(ERR, "Failed to request queues from primary: %d",
 			rte_errno);
 		return -1;
-- 
2.17.2

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/tap: ipc add check for number of messages received

[Lipiec, Herakliusz]

On 4/18/2019 7:13, Ferruh Yigit worte:
> On 4/18/2019 6:19 PM, Herakliusz Lipiec wrote:
> > A sucessfull call to rte_mp_request_sync does not guarantee that there
> > are valid messages in the buffer, and this should be checked for
> > before accessing data in the message.
> >
> > Fixes: c9aa56edec8e ("net/tap: access primary process queues from
> > secondary")
> > Cc: rasland@mellanox.com
> > Cc: stable@dpdk.org
> > Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> > b/drivers/net/tap/rte_eth_tap.c index e9fda8cf6..a619a8850 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name,
> struct rte_eth_dev *dev)
> >  	request.len_param = sizeof(*request_param);
> >  	/* Send request and receive reply */
> >  	ret = rte_mp_request_sync(&request, &replies, &timeout);
> > -	if (ret < 0) {
> > +	if (ret < 0 || replies.n_receieved != 1) {
> 
> The API documentation says:
> 
> ||   * @return
> 
> 
> 
> ||   *  - On success, return 0.
> 
> 
> 
> ||   *  - On failure, return -1, and the reason will be stored in rte_errno.
> 
> So if the API returns 0, why the reply is not valid, also if reply is not valid how
> can you rely on a value in 'replies'
> 
> What do you think updating the 'rte_mp_request_sync()' API to return error
> whenever the reply is not valid?
The reply is not valid, because there is no valid msg pointer in replies.msg (should be null)
replies.nb_received should be either 0 (if replies carries no message) or 1 (if there is a message).

There are two other code paths that can return a success, but have no (valid) message.
In rte_mp_request_sync there is a call to mp_request_sync which may return 0 with no message in case of:
- failure to send the message on behalf of remote
- the caller not caring about reply message.
I propose to add a check for nb_received to net/tap since this seems to be done in everywhere 
else when rte_mp_request_sync is called (this will do no harm), and also I think that return codes should be fixed,
but that can be done irrelevant of this. 

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/tap: ipc add check for number of messages received

[Lipiec, Herakliusz]

On 4/18/2019 7:13, Ferruh Yigit worte:
> On 4/18/2019 6:19 PM, Herakliusz Lipiec wrote:
> > A sucessfull call to rte_mp_request_sync does not guarantee that there
> > are valid messages in the buffer, and this should be checked for
> > before accessing data in the message.
> >
> > Fixes: c9aa56edec8e ("net/tap: access primary process queues from
> > secondary")
> > Cc: rasland@mellanox.com
> > Cc: stable@dpdk.org
> > Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> > b/drivers/net/tap/rte_eth_tap.c index e9fda8cf6..a619a8850 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name,
> struct rte_eth_dev *dev)
> >  	request.len_param = sizeof(*request_param);
> >  	/* Send request and receive reply */
> >  	ret = rte_mp_request_sync(&request, &replies, &timeout);
> > -	if (ret < 0) {
> > +	if (ret < 0 || replies.n_receieved != 1) {
> 
> The API documentation says:
> 
> ||   * @return
> 
> 
> 
> ||   *  - On success, return 0.
> 
> 
> 
> ||   *  - On failure, return -1, and the reason will be stored in rte_errno.
> 
> So if the API returns 0, why the reply is not valid, also if reply is not valid how
> can you rely on a value in 'replies'
> 
> What do you think updating the 'rte_mp_request_sync()' API to return error
> whenever the reply is not valid?
The reply is not valid, because there is no valid msg pointer in replies.msg (should be null)
replies.nb_received should be either 0 (if replies carries no message) or 1 (if there is a message).

There are two other code paths that can return a success, but have no (valid) message.
In rte_mp_request_sync there is a call to mp_request_sync which may return 0 with no message in case of:
- failure to send the message on behalf of remote
- the caller not caring about reply message.
I propose to add a check for nb_received to net/tap since this seems to be done in everywhere 
else when rte_mp_request_sync is called (this will do no harm), and also I think that return codes should be fixed,
but that can be done irrelevant of this. 

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/tap: ipc add check for number of messages received

[Ferruh Yigit]

On 4/19/2019 5:39 PM, Lipiec, Herakliusz wrote:
> On 4/18/2019 7:13, Ferruh Yigit worte:
>> On 4/18/2019 6:19 PM, Herakliusz Lipiec wrote:
>>> A sucessfull call to rte_mp_request_sync does not guarantee that there
>>> are valid messages in the buffer, and this should be checked for
>>> before accessing data in the message.
>>>
>>> Fixes: c9aa56edec8e ("net/tap: access primary process queues from
>>> secondary")
>>> Cc: rasland@mellanox.com
>>> Cc: stable@dpdk.org
>>> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
>>> ---
>>>  drivers/net/tap/rte_eth_tap.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>>> b/drivers/net/tap/rte_eth_tap.c index e9fda8cf6..a619a8850 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name,
>> struct rte_eth_dev *dev)
>>>  	request.len_param = sizeof(*request_param);
>>>  	/* Send request and receive reply */
>>>  	ret = rte_mp_request_sync(&request, &replies, &timeout);
>>> -	if (ret < 0) {
>>> +	if (ret < 0 || replies.n_receieved != 1) {
>>
>> The API documentation says:
>>
>> ||   * @return
>>
>>
>>
>> ||   *  - On success, return 0.
>>
>>
>>
>> ||   *  - On failure, return -1, and the reason will be stored in rte_errno.
>>
>> So if the API returns 0, why the reply is not valid, also if reply is not valid how
>> can you rely on a value in 'replies'
>>
>> What do you think updating the 'rte_mp_request_sync()' API to return error
>> whenever the reply is not valid?
> The reply is not valid, because there is no valid msg pointer in replies.msg (should be null)
> replies.nb_received should be either 0 (if replies carries no message) or 1 (if there is a message).
> 
> There are two other code paths that can return a success, but have no (valid) message.
> In rte_mp_request_sync there is a call to mp_request_sync which may return 0 with no message in case of:
> - failure to send the message on behalf of remote
> - the caller not caring about reply message.
> I propose to add a check for nb_received to net/tap since this seems to be done in everywhere 
> else when rte_mp_request_sync is called (this will do no harm), and also I think that return codes should be fixed,
> but that can be done irrelevant of this. 
> 

I see, "replies.nb_received" can be relied on since it is set in the begging of
the 'rte_mp_request_sync()'

And I can see you have created a defect for the API fix [1], it is OK to get tap
fix for rc2, but for next release it would be appreciated if you own the defect
you have submitted.

Thanks,
ferruh

[1]
https://bugs.dpdk.org/show_bug.cgi?id=257

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/tap: ipc add check for number of messages received

[Ferruh Yigit]

On 4/19/2019 5:39 PM, Lipiec, Herakliusz wrote:
> On 4/18/2019 7:13, Ferruh Yigit worte:
>> On 4/18/2019 6:19 PM, Herakliusz Lipiec wrote:
>>> A sucessfull call to rte_mp_request_sync does not guarantee that there
>>> are valid messages in the buffer, and this should be checked for
>>> before accessing data in the message.
>>>
>>> Fixes: c9aa56edec8e ("net/tap: access primary process queues from
>>> secondary")
>>> Cc: rasland@mellanox.com
>>> Cc: stable@dpdk.org
>>> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
>>> ---
>>>  drivers/net/tap/rte_eth_tap.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>>> b/drivers/net/tap/rte_eth_tap.c index e9fda8cf6..a619a8850 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -2101,7 +2101,7 @@ tap_mp_attach_queues(const char *port_name,
>> struct rte_eth_dev *dev)
>>>  	request.len_param = sizeof(*request_param);
>>>  	/* Send request and receive reply */
>>>  	ret = rte_mp_request_sync(&request, &replies, &timeout);
>>> -	if (ret < 0) {
>>> +	if (ret < 0 || replies.n_receieved != 1) {
>>
>> The API documentation says:
>>
>> ||   * @return
>>
>>
>>
>> ||   *  - On success, return 0.
>>
>>
>>
>> ||   *  - On failure, return -1, and the reason will be stored in rte_errno.
>>
>> So if the API returns 0, why the reply is not valid, also if reply is not valid how
>> can you rely on a value in 'replies'
>>
>> What do you think updating the 'rte_mp_request_sync()' API to return error
>> whenever the reply is not valid?
> The reply is not valid, because there is no valid msg pointer in replies.msg (should be null)
> replies.nb_received should be either 0 (if replies carries no message) or 1 (if there is a message).
> 
> There are two other code paths that can return a success, but have no (valid) message.
> In rte_mp_request_sync there is a call to mp_request_sync which may return 0 with no message in case of:
> - failure to send the message on behalf of remote
> - the caller not caring about reply message.
> I propose to add a check for nb_received to net/tap since this seems to be done in everywhere 
> else when rte_mp_request_sync is called (this will do no harm), and also I think that return codes should be fixed,
> but that can be done irrelevant of this. 
> 

I see, "replies.nb_received" can be relied on since it is set in the begging of
the 'rte_mp_request_sync()'

And I can see you have created a defect for the API fix [1], it is OK to get tap
fix for rc2, but for next release it would be appreciated if you own the defect
you have submitted.

Thanks,
ferruh

[1]
https://bugs.dpdk.org/show_bug.cgi?id=257

Re: [dpdk-dev] [PATCH v2] net/tap: ipc add check for number of messages received

[Ferruh Yigit]

On 4/19/2019 11:28 AM, Herakliusz Lipiec wrote:
> A sucessfull call to rte_mp_request_sync does not guarantee that there
> are any messages in the buffer, and this should be checked for before
> accessing data in the message. Buffer can be empty if IPC is disabled or
> if we deciede to ignore replies.
> 
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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


(https://bugs.dpdk.org/show_bug.cgi?id=257 created for the API)

Re: [dpdk-dev] [PATCH v2] net/tap: ipc add check for number of messages received

[Ferruh Yigit]

On 4/19/2019 11:28 AM, Herakliusz Lipiec wrote:
> A sucessfull call to rte_mp_request_sync does not guarantee that there
> are any messages in the buffer, and this should be checked for before
> accessing data in the message. Buffer can be empty if IPC is disabled or
> if we deciede to ignore replies.
> 
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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


(https://bugs.dpdk.org/show_bug.cgi?id=257 created for the API)