| Message ID | 1553091059-15991-1-git-send-email-ruxandra.radulescu@nxp.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] dpaa2-eth: Fix possible access beyond end of array | expand |
From: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> Date: Wed, 20 Mar 2019 14:11:04 +0000 > Make sure we don't try to enqueue XDP_REDIRECT frames to an > inexistent FQ. > > While it is guaranteed not to have more than one queue per core, > having fewer queues than CPUs on an interface is a valid > configuration. > > Fixes: d678be1dc1ec ("dpaa2-eth: add XDP_REDIRECT support") > Reported-by: Jesper Dangaard Brouer <brouer@redhat.com> > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> Applied, thanks. I hope the modulus operation doesn't start showing up on profiles.
> -----Original Message----- > From: David Miller <davem@davemloft.net> > Sent: Thursday, March 21, 2019 1:29 AM > To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> > Cc: netdev@vger.kernel.org; Ioana Ciornei <ioana.ciornei@nxp.com>; > brouer@redhat.com > Subject: Re: [PATCH net] dpaa2-eth: Fix possible access beyond end of array > > From: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> > Date: Wed, 20 Mar 2019 14:11:04 +0000 > > > Make sure we don't try to enqueue XDP_REDIRECT frames to an > > inexistent FQ. > > > > While it is guaranteed not to have more than one queue per core, > > having fewer queues than CPUs on an interface is a valid > > configuration. > > > > Fixes: d678be1dc1ec ("dpaa2-eth: add XDP_REDIRECT support") > > Reported-by: Jesper Dangaard Brouer <brouer@redhat.com> > > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > > Applied, thanks. > > I hope the modulus operation doesn't start showing up on profiles. I haven't seen any performance impact on XDP_REDIRECT numbers. Also, I think this patch should be added to stable as well, the bug it fixes is quite ugly. Thanks, Ioana
On Wed, 20 Mar 2019 14:11:04 +0000 Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> wrote: > Make sure we don't try to enqueue XDP_REDIRECT frames to an > inexistent FQ. > > While it is guaranteed not to have more than one queue per core, > having fewer queues than CPUs on an interface is a valid > configuration. > > Fixes: d678be1dc1ec ("dpaa2-eth: add XDP_REDIRECT support") > Reported-by: Jesper Dangaard Brouer <brouer@redhat.com> > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 2ba49e9..1a68052 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -1817,7 +1817,7 @@ static int dpaa2_eth_xdp_xmit_frame(struct net_device *net_dev, > dpaa2_fd_set_format(&fd, dpaa2_fd_single); > dpaa2_fd_set_ctrl(&fd, FD_CTRL_PTA); > > - fq = &priv->fq[smp_processor_id()]; > + fq = &priv->fq[smp_processor_id() % dpaa2_eth_queue_count(priv)]; > for (i = 0; i < DPAA2_ETH_ENQUEUE_RETRIES; i++) { > err = priv->enqueue(priv, fq, &fd, 0); Are you sure this is correct? How do you guard/lock concurrent CPUs from accessing 'fq' ?
> -----Original Message----- > From: Jesper Dangaard Brouer <brouer@redhat.com> > Sent: Sunday, March 24, 2019 8:29 PM > To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> > Cc: netdev@vger.kernel.org; davem@davemloft.net; Ioana Ciornei > <ioana.ciornei@nxp.com>; brouer@redhat.com > Subject: Re: [PATCH net] dpaa2-eth: Fix possible access beyond end of array > > On Wed, 20 Mar 2019 14:11:04 +0000 > Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> wrote: > > > Make sure we don't try to enqueue XDP_REDIRECT frames to an > > inexistent FQ. > > > > While it is guaranteed not to have more than one queue per core, > > having fewer queues than CPUs on an interface is a valid > > configuration. > > > > Fixes: d678be1dc1ec ("dpaa2-eth: add XDP_REDIRECT support") > > Reported-by: Jesper Dangaard Brouer <brouer@redhat.com> > > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > > --- > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > index 2ba49e9..1a68052 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > @@ -1817,7 +1817,7 @@ static int dpaa2_eth_xdp_xmit_frame(struct > net_device *net_dev, > > dpaa2_fd_set_format(&fd, dpaa2_fd_single); > > dpaa2_fd_set_ctrl(&fd, FD_CTRL_PTA); > > > > - fq = &priv->fq[smp_processor_id()]; > > + fq = &priv->fq[smp_processor_id() % dpaa2_eth_queue_count(priv)]; > > for (i = 0; i < DPAA2_ETH_ENQUEUE_RETRIES; i++) { > > err = priv->enqueue(priv, fq, &fd, 0); > > Are you sure this is correct? > How do you guard/lock concurrent CPUs from accessing 'fq' ? We only use 'fq' in the enqueue command to hardware. Our hardware can handle concurrent enqueue commands, we might at most get a busy response from it. Thanks, Ioana
On Mon, 25 Mar 2019 09:42:22 +0000 Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> wrote: > > -----Original Message----- > > From: Jesper Dangaard Brouer <brouer@redhat.com> > > Sent: Sunday, March 24, 2019 8:29 PM > > To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> > > Cc: netdev@vger.kernel.org; davem@davemloft.net; Ioana Ciornei > > <ioana.ciornei@nxp.com>; brouer@redhat.com > > Subject: Re: [PATCH net] dpaa2-eth: Fix possible access beyond end of array > > > > On Wed, 20 Mar 2019 14:11:04 +0000 > > Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com> wrote: > > > > > Make sure we don't try to enqueue XDP_REDIRECT frames to an > > > inexistent FQ. > > > > > > While it is guaranteed not to have more than one queue per core, > > > having fewer queues than CPUs on an interface is a valid > > > configuration. > > > > > > Fixes: d678be1dc1ec ("dpaa2-eth: add XDP_REDIRECT support") > > > Reported-by: Jesper Dangaard Brouer <brouer@redhat.com> > > > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > > > --- > > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > index 2ba49e9..1a68052 100644 > > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > > @@ -1817,7 +1817,7 @@ static int dpaa2_eth_xdp_xmit_frame(struct > > net_device *net_dev, > > > dpaa2_fd_set_format(&fd, dpaa2_fd_single); > > > dpaa2_fd_set_ctrl(&fd, FD_CTRL_PTA); > > > > > > - fq = &priv->fq[smp_processor_id()]; > > > + fq = &priv->fq[smp_processor_id() % dpaa2_eth_queue_count(priv)]; > > > for (i = 0; i < DPAA2_ETH_ENQUEUE_RETRIES; i++) { > > > err = priv->enqueue(priv, fq, &fd, 0); > > > > Are you sure this is correct? > > How do you guard/lock concurrent CPUs from accessing 'fq' ? > > We only use 'fq' in the enqueue command to hardware. > Our hardware can handle concurrent enqueue commands, we > might at most get a busy response from it. Great, then this should be okay :-)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c index 2ba49e9..1a68052 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -1817,7 +1817,7 @@ static int dpaa2_eth_xdp_xmit_frame(struct net_device *net_dev, dpaa2_fd_set_format(&fd, dpaa2_fd_single); dpaa2_fd_set_ctrl(&fd, FD_CTRL_PTA); - fq = &priv->fq[smp_processor_id()]; + fq = &priv->fq[smp_processor_id() % dpaa2_eth_queue_count(priv)]; for (i = 0; i < DPAA2_ETH_ENQUEUE_RETRIES; i++) { err = priv->enqueue(priv, fq, &fd, 0); if (err != -EBUSY)
Make sure we don't try to enqueue XDP_REDIRECT frames to an inexistent FQ. While it is guaranteed not to have more than one queue per core, having fewer queues than CPUs on an interface is a valid configuration. Fixes: d678be1dc1ec ("dpaa2-eth: add XDP_REDIRECT support") Reported-by: Jesper Dangaard Brouer <brouer@redhat.com> Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)