| 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
| ||
|
Date: Wed, 27 Dec 2017 12:58:24 +0100
From: Jesper Dangaard Brouer <brouer@...hat.com>
To: "Chopra, Manish" <Manish.Chopra@...ium.com>
Cc: Daniel Borkmann <borkmann@...earbox.net>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
"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>,
brouer@...hat.com
Subject: Re: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and
intro xdp_rxq_info_is_reg
On Wed, 27 Dec 2017 10:37:10 +0000
"Chopra, Manish" <Manish.Chopra@...ium.com> wrote:
> > -----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);
No, it is not sufficient.
Your driver have qede_alloc_fp_array() which for_each_queue (kz)alloc
memory. If one of the memory allocations fail, then memory is
free'ed/"rolled-back" in qede_free_fp_array(). Thus, some of the
fp->rxq pointers will be allocated, but xdp_rxq_info_reg() have not
"registered" it yet. Thus, this _is_ needed to correctly handle memory
failures.
> 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() ?
The xdp_rxq_info_is_reg() API is needed in your driver, and I also found
a need for this in the Broadcom bnxt_en driver.
> > 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.
With the current implementation in xdp.c, I agree that this case will
never fail in your driver. There is a clear expectation, that this API
will be extended (to support per RX-ring memory "return/free" API) and
that "will-never-fail" assumption will likely change...
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
Powered by blists - more mailing lists