Re: [PATCH] net/mlx5: Fix possible NULL deref in RX path

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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