All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net/mlx5: Fix possible NULL deref in RX path
@ 2016-08-01  8:44 Sagi Grimberg
  2016-08-01 16:43 ` Adrien Mazarguil
  0 siblings, 1 reply; 6+ messages in thread
From: Sagi Grimberg @ 2016-08-01  8:44 UTC (permalink / raw)
  To: dev

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.

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/mlx5: Fix possible NULL deref in RX path
  2016-08-01  8:44 [PATCH] net/mlx5: Fix possible NULL deref in RX path Sagi Grimberg
@ 2016-08-01 16:43 ` Adrien Mazarguil
  2016-08-02  9:31   ` Sagi Grimberg
  0 siblings, 1 reply; 6+ messages in thread
From: Adrien Mazarguil @ 2016-08-01 16:43 UTC (permalink / raw)
  To: Sagi Grimberg; +Cc: dev

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/mlx5: Fix possible NULL deref in RX path
  2016-08-01 16:43 ` Adrien Mazarguil
@ 2016-08-02  9:31   ` Sagi Grimberg
  2016-08-02  9:58     ` Adrien Mazarguil
  0 siblings, 1 reply; 6+ messages in thread
From: Sagi Grimberg @ 2016-08-02  9:31 UTC (permalink / raw)
  To: Adrien Mazarguil; +Cc: dev



On 01/08/16 19:43, Adrien Mazarguil wrote:
> 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")

I actually got the NULL deref. This happens when the application doesn't
restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
will fail on the first iteration (pkt wasn't assigned) unlike the
condition handled in a1bdb71a32da.

With this applied, I didn't see the crash.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/mlx5: Fix possible NULL deref in RX path
  2016-08-02  9:31   ` Sagi Grimberg
@ 2016-08-02  9:58     ` Adrien Mazarguil
  2016-08-02 10:47       ` Sagi Grimberg
  0 siblings, 1 reply; 6+ messages in thread
From: Adrien Mazarguil @ 2016-08-02  9:58 UTC (permalink / raw)
  To: Sagi Grimberg; +Cc: dev

On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote:
> 
> 
> On 01/08/16 19:43, Adrien Mazarguil wrote:
> >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")
> 
> I actually got the NULL deref. This happens when the application doesn't
> restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
> will fail on the first iteration (pkt wasn't assigned) unlike the
> condition handled in a1bdb71a32da.
> 
> With this applied, I didn't see the crash.

Thanks for confirming this, now what about the different approach I
suggested in my previous message to avoid the extra check in the inner loop:

 if (!pkt)
     pkt = seg;
 while (pkt != seg) {
      ...
 }

Also the fixes line in your commit message?

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/mlx5: Fix possible NULL deref in RX path
  2016-08-02  9:58     ` Adrien Mazarguil
@ 2016-08-02 10:47       ` Sagi Grimberg
  2016-08-02 11:31         ` Adrien Mazarguil
  0 siblings, 1 reply; 6+ messages in thread
From: Sagi Grimberg @ 2016-08-02 10:47 UTC (permalink / raw)
  To: Adrien Mazarguil; +Cc: dev



On 02/08/16 12:58, Adrien Mazarguil wrote:
> On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote:
>>
>>
>> On 01/08/16 19:43, Adrien Mazarguil wrote:
>>> 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")
>>
>> I actually got the NULL deref. This happens when the application doesn't
>> restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
>> will fail on the first iteration (pkt wasn't assigned) unlike the
>> condition handled in a1bdb71a32da.
>>
>> With this applied, I didn't see the crash.
>
> Thanks for confirming this,

Hey Adrien, I just noticed that I missed the rest of
your response in the previous message (pre-coffee mail
browsing...)

You analysis was on spot.

> now what about the different approach I
> suggested in my previous message to avoid the extra check in the inner loop:
>
>  if (!pkt)
>      pkt = seg;
>  while (pkt != seg) {
>       ...
>  }

We can go this way, but it looks kinda confusing to set pkt = seg and
then iterate on pkt != seg.

How about a more explicit approach:
--
diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index fce3381ae87a..37573668e43e 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1572,6 +1572,14 @@ 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)) {
+                       ++rxq->stats.rx_nombuf;
+                       if (!pkt) {
+                               /*
+                                * no buffers before we even started,
+                                * bail out silently.
+                                */
+                               break;
+                       }
                         while (pkt != seg) {
                                 assert(pkt != (*rxq->elts)[idx]);
                                 seg = NEXT(pkt);
@@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf 
**pkts, uint16_t pkts_n)
                                 __rte_mbuf_raw_free(pkt);
                                 pkt = seg;
                         }
-                       ++rxq->stats.rx_nombuf;
                         break;
                 }
                 if (!pkt) {
--


> Also the fixes line in your commit message?

I'll add it in v2. Thanks.

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/mlx5: Fix possible NULL deref in RX path
  2016-08-02 10:47       ` Sagi Grimberg
@ 2016-08-02 11:31         ` Adrien Mazarguil
  0 siblings, 0 replies; 6+ messages in thread
From: Adrien Mazarguil @ 2016-08-02 11:31 UTC (permalink / raw)
  To: Sagi Grimberg; +Cc: dev

On Tue, Aug 02, 2016 at 01:47:55PM +0300, Sagi Grimberg wrote:
> 
> 
> On 02/08/16 12:58, Adrien Mazarguil wrote:
> >On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote:
> >>
> >>
> >>On 01/08/16 19:43, Adrien Mazarguil wrote:
> >>>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")
> >>
> >>I actually got the NULL deref. This happens when the application doesn't
> >>restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
> >>will fail on the first iteration (pkt wasn't assigned) unlike the
> >>condition handled in a1bdb71a32da.
> >>
> >>With this applied, I didn't see the crash.
> >
> >Thanks for confirming this,
> 
> Hey Adrien, I just noticed that I missed the rest of
> your response in the previous message (pre-coffee mail
> browsing...)
> 
> You analysis was on spot.
> 
> >now what about the different approach I
> >suggested in my previous message to avoid the extra check in the inner loop:
> >
> > if (!pkt)
> >     pkt = seg;
> > while (pkt != seg) {
> >      ...
> > }
> 
> We can go this way, but it looks kinda confusing to set pkt = seg and
> then iterate on pkt != seg.
> 
> How about a more explicit approach:
> --
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index fce3381ae87a..37573668e43e 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -1572,6 +1572,14 @@ 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)) {
> +                       ++rxq->stats.rx_nombuf;
> +                       if (!pkt) {
> +                               /*
> +                                * no buffers before we even started,
> +                                * bail out silently.
> +                                */
> +                               break;
> +                       }
>                         while (pkt != seg) {
>                                 assert(pkt != (*rxq->elts)[idx]);
>                                 seg = NEXT(pkt);
> @@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts,
> uint16_t pkts_n)
>                                 __rte_mbuf_raw_free(pkt);
>                                 pkt = seg;
>                         }
> -                       ++rxq->stats.rx_nombuf;
>                         break;
>                 }
>                 if (!pkt) {
> --

Yes, that's also fine.

> >Also the fixes line in your commit message?
> 
> I'll add it in v2. Thanks.

Go ahead, thanks!

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-08-02 11:31 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-01  8:44 [PATCH] net/mlx5: Fix possible NULL deref in RX path Sagi Grimberg
2016-08-01 16:43 ` Adrien Mazarguil
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.