[PATCH net 0/2] nfp: fix retcode and disable netpoll on representors

[PATCH net 0/2] nfp: fix retcode and disable netpoll on representors

[Jakub Kicinski]

Hi!

This series avoids a potential crash on nfp representor devices
when netpoll is in use.  If transmitting the frame through underlying
vNIC fails we'd return an error code (by passing on error code from
__dev_queue_xmit()) and cause double free in netpoll code.

Fix the error code and disable netpoll on reprs altogether.
IRQ-safety of locking the queues and calling __dev_queue_xmit()
is questionable.

Big thanks to John Hurley for debugging and narrowing down
the trace log after I gave up! :)

Jakub Kicinski (2):
  nfp: validate the return code from dev_queue_xmit()
  nfp: disable netpoll on representors

 drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.19.2

[PATCH net 1/2] nfp: validate the return code from dev_queue_xmit()

[Jakub Kicinski]

dev_queue_xmit() may return error codes as well as netdev_tx_t,
and it always consumes the skb.  Make sure we always return a
correct netdev_tx_t value.

Fixes: eadfa4c3be99 ("nfp: add stats and xmit helpers for representors")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index d2c803bb4e56..7b46fce2e81e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -195,7 +195,7 @@ static netdev_tx_t nfp_repr_xmit(struct sk_buff *skb, struct net_device *netdev)
 	ret = dev_queue_xmit(skb);
 	nfp_repr_inc_tx_stats(netdev, len, ret);
 
-	return ret;
+	return NETDEV_TX_OK;
 }
 
 static int nfp_repr_stop(struct net_device *netdev)
-- 
2.21.0

[PATCH net 2/2] nfp: disable netpoll on representors

[Jakub Kicinski]

NFP reprs are software device on top of the PF's vNIC.
The comment above __dev_queue_xmit() sayeth:

 When calling this method, interrupts MUST be enabled.  This is because
 the BH enable code must have IRQs enabled so that it will not deadlock.

For netconsole we can't guarantee IRQ state, let's just
disable netpoll on representors to be on the safe side.

When the initial implementation of NFP reprs was added by the
commit 5de73ee46704 ("nfp: general representor implementation")
.ndo_poll_controller was required for netpoll to be enabled.

Fixes: ac3d9dd034e5 ("netpoll: make ndo_poll_controller() optional")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index 7b46fce2e81e..94d228c04496 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -383,7 +383,7 @@ int nfp_repr_init(struct nfp_app *app, struct net_device *netdev,
 	netdev->features &= ~(NETIF_F_TSO | NETIF_F_TSO6);
 	netdev->gso_max_segs = NFP_NET_LSO_MAX_SEGS;
 
-	netdev->priv_flags |= IFF_NO_QUEUE;
+	netdev->priv_flags |= IFF_NO_QUEUE | IFF_DISABLE_NETPOLL;
 	netdev->features |= NETIF_F_LLTX;
 
 	if (nfp_app_has_tc(app)) {
-- 
2.21.0

Re: [PATCH net 0/2] nfp: fix retcode and disable netpoll on representors

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Wed, 27 Mar 2019 11:38:37 -0700

> This series avoids a potential crash on nfp representor devices
> when netpoll is in use.  If transmitting the frame through underlying
> vNIC fails we'd return an error code (by passing on error code from
> __dev_queue_xmit()) and cause double free in netpoll code.
> 
> Fix the error code and disable netpoll on reprs altogether.
> IRQ-safety of locking the queues and calling __dev_queue_xmit()
> is questionable.
> 
> Big thanks to John Hurley for debugging and narrowing down
> the trace log after I gave up! :)

Series applied, thanks Jakub.

Want me to queue these up for -stable?

Re: [PATCH net 0/2] nfp: fix retcode and disable netpoll on representors

[Jakub Kicinski]

On Thu, 28 Mar 2019 17:04:53 -0700 (PDT), David Miller wrote:
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Wed, 27 Mar 2019 11:38:37 -0700
> 
> > This series avoids a potential crash on nfp representor devices
> > when netpoll is in use.  If transmitting the frame through underlying
> > vNIC fails we'd return an error code (by passing on error code from
> > __dev_queue_xmit()) and cause double free in netpoll code.
> > 
> > Fix the error code and disable netpoll on reprs altogether.
> > IRQ-safety of locking the queues and calling __dev_queue_xmit()
> > is questionable.
> > 
> > Big thanks to John Hurley for debugging and narrowing down
> > the trace log after I gave up! :)  
> 
> Series applied, thanks Jakub.
> 
> Want me to queue these up for -stable?

Mm.. yes, I think so, patch 1 can lead to a hard to catch double free.

Thanks!

Re: [PATCH net 0/2] nfp: fix retcode and disable netpoll on representors

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu, 28 Mar 2019 17:27:50 -0700

> On Thu, 28 Mar 2019 17:04:53 -0700 (PDT), David Miller wrote:
>> From: Jakub Kicinski <jakub.kicinski@netronome.com>
>> Date: Wed, 27 Mar 2019 11:38:37 -0700
>> 
>> > This series avoids a potential crash on nfp representor devices
>> > when netpoll is in use.  If transmitting the frame through underlying
>> > vNIC fails we'd return an error code (by passing on error code from
>> > __dev_queue_xmit()) and cause double free in netpoll code.
>> > 
>> > Fix the error code and disable netpoll on reprs altogether.
>> > IRQ-safety of locking the queues and calling __dev_queue_xmit()
>> > is questionable.
>> > 
>> > Big thanks to John Hurley for debugging and narrowing down
>> > the trace log after I gave up! :)  
>> 
>> Series applied, thanks Jakub.
>> 
>> Want me to queue these up for -stable?
> 
> Mm.. yes, I think so, patch 1 can lead to a hard to catch double free.

Ok, queued up.