From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Sagi Grimberg <sagi@grimberg.me>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/mlx5: Fix possible NULL deref in RX path
Date: Mon, 1 Aug 2016 18:43:42 +0200 [thread overview]
Message-ID: <20160801164342.GL9044@6wind.com> (raw)
In-Reply-To: <1470041061-8059-1-git-send-email-sagi@grimberg.me>
Hi Sagi,
On Mon, Aug 01, 2016 at 11:44:21AM +0300, Sagi Grimberg wrote:
> The user is allowed to call ->rx_pkt_burst() even without free
> mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
> on the first iteration (where pkt is still NULL). This would cause us
> to deref a NULL pkt (reset refcount and free).
>
> Fix this by checking the pkt before freeing it.
Just to be sure, did you get an actual NULL deref crash here or is that an
assumed possibility?
I'm asking because this problem was supposed to be addressed by:
a1bdb71a32da ("net/mlx5: fix crash in Rx")
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/net/mlx5/mlx5_rxtx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index fce3381ae87a..a07cc4794023 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -1572,7 +1572,7 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
> rte_prefetch0(wqe);
> rep = rte_mbuf_raw_alloc(rxq->mp);
> if (unlikely(rep == NULL)) {
> - while (pkt != seg) {
> + while (pkt && pkt != seg) {
> assert(pkt != (*rxq->elts)[idx]);
> seg = NEXT(pkt);
> rte_mbuf_refcnt_set(pkt, 0);
> --
> 1.9.1
I've reviewed your patch and it indeed seems to address an issue, please
confirm my analysis below.
When rep cannot be allocated and is thus NULL, either pkt is still NULL
because the first packet segment has not been seen yet or points to the
first segment.
Either way at this point, seg points to current segment to process in the
queue and is never NULL.
Thus when pkt is still NULL (first segment) and rep cannot be allocated, the
comparison (pkt != seg) between a valid pointer (seg) and NULL (pkt)
succeeds. This case is not handled by the assert() statement and a crash
occurs.
We really want to avoid useless code in the data path, particularly inside
loops. The extra check you added is performed for each iteration, so what
about modifying your patch by adding the following if statement instead?
if (!pkt)
pkt = seg;
while (pkt != seg) {
...
}
I guess you could add "Fixes: a1bdb71a32da ("net/mlx5: fix crash in Rx")"
line to your commit log as well because the original patch only solved half
of the issue.
Thanks.
--
Adrien Mazarguil
6WIND
next parent reply other threads:[~2016-08-01 16:43 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1470041061-8059-1-git-send-email-sagi@grimberg.me>
2016-08-01 16:43 ` Adrien Mazarguil [this message]
2016-08-02 9:31 ` Sagi Grimberg
2016-08-02 9:58 ` Adrien Mazarguil
2016-08-02 10:47 ` Sagi Grimberg
2016-08-02 11:31 ` Adrien Mazarguil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160801164342.GL9044@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=sagi@grimberg.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).