netdev - RE: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and intro xdp_rxq_info_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 27 Dec 2017 10:37:10 +0000
From:   "Chopra, Manish" <Manish.Chopra@...ium.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     "bjorn.topel@...el.com" <bjorn.topel@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Elior, Ariel" <Ariel.Elior@...ium.com>,
        "dsahern@...il.com" <dsahern@...il.com>,
        "gospo@...adcom.com" <gospo@...adcom.com>,
        Dept-Eng Everest Linux L2 <Dept-EngEverestLinuxL2@...ium.com>,
        "michael.chan@...adcom.com" <michael.chan@...adcom.com>
Subject: RE: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and intro
 xdp_rxq_info_is_reg

> -----Original Message-----
> From: Jesper Dangaard Brouer [mailto:brouer@...hat.com]
> Sent: Friday, December 22, 2017 10:42 PM
> To: Daniel Borkmann <borkmann@...earbox.net>; Alexei Starovoitov
> <alexei.starovoitov@...il.com>
> Cc: bjorn.topel@...el.com; netdev@...r.kernel.org; Jesper Dangaard Brouer
> <brouer@...hat.com>; Elior, Ariel <Ariel.Elior@...ium.com>;
> dsahern@...il.com; gospo@...adcom.com; Dept-Eng Everest Linux L2 <Dept-
> EngEverestLinuxL2@...ium.com>; michael.chan@...adcom.com
> Subject: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and intro
> xdp_rxq_info_is_reg
> 
> The driver code qede_free_fp_array() depend on kfree() can be called with a
> NULL pointer. This stems from the qede_alloc_fp_array() function which either
> (kz)alloc memory for fp->txq or fp->rxq.
> This also simplifies error handling code in case of memory allocation failures,
> but xdp_rxq_info_unreg need to know the difference.
> 
> Introduce xdp_rxq_info_is_reg() to handle if a memory allocation fails and
> detect this is the failure path by seeing that xdp_rxq_info was not registred yet,
> which first happens after successful alloaction in qede_init_fp().
> 
> Driver hook points for xdp_rxq_info:
>  * reg  : qede_init_fp
>  * unreg: qede_free_fp_array
> 
> Tested on actual hardware with samples/bpf program.
> 
> V2: Driver have no proper error path for failed XDP RX-queue info reg, as
> qede_init_fp() is a void function.
> 
> Cc: everest-linux-l2@...ium.com
> Cc: Ariel Elior <Ariel.Elior@...ium.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>  drivers/net/ethernet/qlogic/qede/qede.h      |    2 ++
>  drivers/net/ethernet/qlogic/qede/qede_fp.c   |    1 +
>  drivers/net/ethernet/qlogic/qede/qede_main.c |   10 ++++++++++
>  include/net/xdp.h                            |    1 +
>  net/core/xdp.c                               |    6 ++++++
>  5 files changed, 20 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede.h
> b/drivers/net/ethernet/qlogic/qede/qede.h
> index a3a70ade411f..62f47736511b 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede.h
> +++ b/drivers/net/ethernet/qlogic/qede/qede.h
> @@ -40,6 +40,7 @@
>  #include <linux/kernel.h>
>  #include <linux/mutex.h>
>  #include <linux/bpf.h>
> +#include <net/xdp.h>
>  #include <linux/qed/qede_rdma.h>
>  #include <linux/io.h>
>  #ifdef CONFIG_RFS_ACCEL
> @@ -345,6 +346,7 @@ struct qede_rx_queue {
>  	u64 xdp_no_pass;
> 
>  	void *handle;
> +	struct xdp_rxq_info xdp_rxq;
>  };
> 
>  union db_prod {
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index 48ec4c56cddf..dafc079ab6b9 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> @@ -1006,6 +1006,7 @@ static bool qede_rx_xdp(struct qede_dev *edev,
>  	xdp.data = xdp.data_hard_start + *data_offset;
>  	xdp_set_data_meta_invalid(&xdp);
>  	xdp.data_end = xdp.data + *len;
> +	xdp.rxq = &rxq->xdp_rxq;
> 
>  	/* Queues always have a full reset currently, so for the time
>  	 * being until there's atomic program replace just mark read diff --git
> a/drivers/net/ethernet/qlogic/qede/qede_main.c
> b/drivers/net/ethernet/qlogic/qede/qede_main.c
> index 57332b3e5e64..24160f1fd0e5 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> @@ -762,6 +762,12 @@ static void qede_free_fp_array(struct qede_dev *edev)
>  			fp = &edev->fp_array[i];
> 
>  			kfree(fp->sb_info);
> +			/* Handle mem alloc failure case where qede_init_fp
> +			 * didn't register xdp_rxq_info yet.
> +			 * Implicit only (fp->type & QEDE_FASTPATH_RX)
> +			 */
> +			if (fp->rxq && xdp_rxq_info_is_reg(&fp->rxq->xdp_rxq))
> +				xdp_rxq_info_unreg(&fp->rxq->xdp_rxq);

Isn't this sufficient just ?
If (fp->rxq)
	xdp_rxq_info_unreg(&fp->rxq->xdp_rxq);

xdp_rxq_info_is_reg() API seems unnecessary to me, xdp_rxq_info_unreg() seems to be taking care of the case
if it's not registered then WARN already, I think instead of adding these checks in individual drivers, better handle it properly in xdp_rxq_info_unreg() ?

>  			kfree(fp->rxq);
>  			kfree(fp->xdp_tx);
>  			kfree(fp->txq);
> @@ -1498,6 +1504,10 @@ static void qede_init_fp(struct qede_dev *edev)
>  			else
>  				fp->rxq->data_direction =
> DMA_FROM_DEVICE;
>  			fp->rxq->dev = &edev->pdev->dev;
> +
> +			/* Driver have no error path from here */
> +			WARN_ON(xdp_rxq_info_reg(&fp->rxq->xdp_rxq, edev-
> >ndev,
> +						 fp->rxq->rxq_id) < 0);
>  		}

We will reach to this point only when fp->rxq and edev are valid
I don't think that xdp_rxq_info_reg() will fail ever in that case.