rte_ethdev: fix unsafe memory access by calling RX callback.
Checks
Commit Message
When compiling with -O0,
the compiler does not optimize two memory accesses into one.
Leads to accessing a null pointer when calling the RX callback.
The way to access the TX callback is correct.
Signed-off-by: Tencent TGW team <tgw_team@tencent.com>
---
lib/librte_ethdev/rte_ethdev.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
Comments
On 3/4/20 5:05 PM, Tencent TGW team wrote:
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when calling the RX callback.
> The way to access the TX callback is correct.
It looks like the patch is not passed through check-git-log.sh.
RX -> Rx, TX -> Tx
>
> Signed-off-by: Tencent TGW team <tgw_team@tencent.com>
If I'm not mistaken, it must be a person here, not team.
> ---
> lib/librte_ethdev/rte_ethdev.h | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..35eb580ff 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> rx_pkts, nb_pkts);
>
> #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> - if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> - struct rte_eth_rxtx_callback *cb =
> - dev->post_rx_burst_cbs[queue_id];
> -
> + struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> + if (unlikely(cb != NULL)) {
> do {
> nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> nb_pkts, cb->param);
>
Sorry, but I don't understand. I don't see the difference in
potential NULL pointer deference above. What is the compiler? Version?
Or is it a race condition with queue post Rx burst callback
removal while traffic is running?
On Wed, 4 Mar 2020 22:05:43 +0800
Tencent TGW team <quzeyao@gmail.com> wrote:
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when calling the RX callback.
> The way to access the TX callback is correct.
>
> Signed-off-by: Tencent TGW team <tgw_team@tencent.com>
> ---
> lib/librte_ethdev/rte_ethdev.h | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..35eb580ff 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> rx_pkts, nb_pkts);
>
> #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> - if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> - struct rte_eth_rxtx_callback *cb =
> - dev->post_rx_burst_cbs[queue_id];
> -
> + struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> + if (unlikely(cb != NULL)) {
> do {
> nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> nb_pkts, cb->param);
To fix TOCTOU like this you need to use something like READ_ONCE()
which does cast to volatile.
For Developer Certificate of Origin (Signed-off-by) you must use a person
not a team. It is a legal requirement. If you don't know what DCO is you should
read what it means.
Developer's Certificate of Origin 1.1
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
then you just add a line saying::
Signed-off-by: Random J Developer <random@developer.example.org>
using your real name (sorry, no pseudonyms or anonymous contributions.)
Sorry, I`ll use a real name in patch v2.
I don't think this is a TOCTOU question.
The original code works fine when compiled with the -O3 option.
At this point the compiler will optimize to one memory access.
But when compiled with -O0, there will be two memory accesses, which is wrong.
This change was modified with reference to the rte_eth_tx_burst function.
On Wed, 4 Mar 2020 22:05:43 +0800
Tencent TGW team <quzeyao@gmail.com> wrote:
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when calling the RX callback.
> The way to access the TX callback is correct.
>
> Signed-off-by: Tencent TGW team <tgw_team@tencent.com>
> ---
> lib/librte_ethdev/rte_ethdev.h | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..35eb580ff 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> rx_pkts, nb_pkts);
>
> #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> - if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> - struct rte_eth_rxtx_callback *cb =
> - dev->post_rx_burst_cbs[queue_id];
> -
> + struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> + if (unlikely(cb != NULL)) {
> do {
> nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> nb_pkts, cb->param);
To fix TOCTOU like this you need to use something like READ_ONCE()
which does cast to volatile.
For Developer Certificate of Origin (Signed-off-by) you must use a person
not a team. It is a legal requirement. If you don't know what DCO is you should
read what it means.
Developer's Certificate of Origin 1.1
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
then you just add a line saying::
Signed-off-by: Random J Developer <random@developer.example.org>
using your real name (sorry, no pseudonyms or anonymous contributions.)
On Wed, 4 Mar 2020 16:38:13 +0000
tgw_team(腾讯网关团队) <tgw_team@tencent.com> wrote:
> Sorry, I`ll use a real name in patch v2.
>
> I don't think this is a TOCTOU question.
> The original code works fine when compiled with the -O3 option.
> At this point the compiler will optimize to one memory access.
> But when compiled with -O0, there will be two memory accesses, which is wrong.
> This change was modified with reference to the rte_eth_tx_burst function.
There is nothing C standard that says compiler has to do it either way.
The optimizer may decide to do two memory accesses or one.
Depending on that in anyway is bad practice.
So in my patch it was changed to only one memory access.
@@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
rx_pkts, nb_pkts);
#ifdef RTE_ETHDEV_RXTX_CALLBACKS
- if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
- struct rte_eth_rxtx_callback *cb =
- dev->post_rx_burst_cbs[queue_id];
-
+ struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
+ if (unlikely(cb != NULL)) {
do {
nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
nb_pkts, cb->param);