| Message ID | 20190418171923.570-1-herakliusz.lipiec@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Ferruh Yigit |
| Headers | show |
| Series | net/tap: ipc add check for number of messages received | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | fail | Compilation issues |
| ci/Performance-Testing | fail | build patch failure |
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?
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.
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
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;
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(-)