[PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix

[PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix

[Heng Qi]

This patch 2e0de6366ac16 enables napi of the peer veth automatically when the
veth loads the xdp, but it breaks down as reported by Paolo and John. So reverting
it and its fix, we will rework the patch and make it more robust based on comments.

Heng Qi (2):
  Revert "bpf: veth driver panics when xdp prog attached before
    veth_open"
  Revert "veth: Avoid drop packets when xdp_redirect performs"

 drivers/net/veth.c | 88 +++++++---------------------------------------
 1 file changed, 12 insertions(+), 76 deletions(-)

-- 
Previous discussion in
https://lore.kernel.org/all/ce5fe56b-bf1b-71c0-1e96-cff6fde40ff3@linux.alibaba.com/

2.19.1.6.gb485710b

[PATCH 1/2] Revert "bpf: veth driver panics when xdp prog attached before veth_open"

[Heng Qi]

This reverts commit 5e5dc33d5dacb34b0165061bc5a10efd2fd3b66f.

This patch fixes the panic maked by 2e0de6366ac16. Now Paolo
and Toke suggest reverting the patch 2e0de6366ac16 and making
it stronger, so do this first.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/veth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 2a4592780141..b1ed5a93b6c5 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1125,7 +1125,7 @@ static int veth_enable_xdp(struct net_device *dev)
 	int err, i;
 
 	rq = &priv->rq[0];
-	napi_already_on = rcu_access_pointer(rq->napi);
+	napi_already_on = (dev->flags & IFF_UP) && rcu_access_pointer(rq->napi);
 
 	if (!xdp_rxq_info_is_reg(&priv->rq[0].xdp_rxq)) {
 		err = veth_enable_xdp_range(dev, 0, dev->real_num_rx_queues, napi_already_on);
-- 
2.19.1.6.gb485710b

[PATCH 2/2] Revert "veth: Avoid drop packets when xdp_redirect performs"

[Heng Qi]

This reverts commit 2e0de6366ac16ab4d0abb2aaddbc8a1eba216d11.

Based on the issues reported by John and Paolo and their comments,
this patch and the corresponding fix 5e5dc33d5da are reverted, and
we'll remake it.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/veth.c | 88 +++++++---------------------------------------
 1 file changed, 12 insertions(+), 76 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index b1ed5a93b6c5..ac7c0653695f 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1119,14 +1119,10 @@ static void veth_disable_xdp_range(struct net_device *dev, int start, int end,
 
 static int veth_enable_xdp(struct net_device *dev)
 {
+	bool napi_already_on = veth_gro_requested(dev) && (dev->flags & IFF_UP);
 	struct veth_priv *priv = netdev_priv(dev);
-	bool napi_already_on;
-	struct veth_rq *rq;
 	int err, i;
 
-	rq = &priv->rq[0];
-	napi_already_on = (dev->flags & IFF_UP) && rcu_access_pointer(rq->napi);
-
 	if (!xdp_rxq_info_is_reg(&priv->rq[0].xdp_rxq)) {
 		err = veth_enable_xdp_range(dev, 0, dev->real_num_rx_queues, napi_already_on);
 		if (err)
@@ -1327,28 +1323,18 @@ static int veth_set_channels(struct net_device *dev,
 
 static int veth_open(struct net_device *dev)
 {
-	struct veth_priv *peer_priv, *priv = netdev_priv(dev);
+	struct veth_priv *priv = netdev_priv(dev);
 	struct net_device *peer = rtnl_dereference(priv->peer);
-	struct veth_rq *peer_rq;
 	int err;
 
 	if (!peer)
 		return -ENOTCONN;
 
-	peer_priv = netdev_priv(peer);
-	peer_rq = &peer_priv->rq[0];
-
 	if (priv->_xdp_prog) {
 		err = veth_enable_xdp(dev);
 		if (err)
 			return err;
-		/* refer to the logic in veth_xdp_set() */
-		if (!rtnl_dereference(peer_rq->napi)) {
-			err = veth_napi_enable(peer);
-			if (err)
-				return err;
-		}
-	} else if (veth_gro_requested(dev) || peer_priv->_xdp_prog) {
+	} else if (veth_gro_requested(dev)) {
 		err = veth_napi_enable(dev);
 		if (err)
 			return err;
@@ -1364,29 +1350,17 @@ static int veth_open(struct net_device *dev)
 
 static int veth_close(struct net_device *dev)
 {
-	struct veth_priv *peer_priv, *priv = netdev_priv(dev);
+	struct veth_priv *priv = netdev_priv(dev);
 	struct net_device *peer = rtnl_dereference(priv->peer);
-	struct veth_rq *peer_rq;
 
 	netif_carrier_off(dev);
-	if (peer) {
-		peer_priv = netdev_priv(peer);
-		peer_rq = &peer_priv->rq[0];
-	}
+	if (peer)
+		netif_carrier_off(peer);
 
-	if (priv->_xdp_prog) {
+	if (priv->_xdp_prog)
 		veth_disable_xdp(dev);
-		/* refer to the logic in veth_xdp_set */
-		if (peer && rtnl_dereference(peer_rq->napi)) {
-			if (!veth_gro_requested(peer) && !peer_priv->_xdp_prog)
-				veth_napi_del(peer);
-		}
-	} else if (veth_gro_requested(dev) || (peer && peer_priv->_xdp_prog)) {
+	else if (veth_gro_requested(dev))
 		veth_napi_del(dev);
-	}
-
-	if (peer)
-		netif_carrier_off(peer);
 
 	return 0;
 }
@@ -1496,21 +1470,17 @@ static int veth_set_features(struct net_device *dev,
 {
 	netdev_features_t changed = features ^ dev->features;
 	struct veth_priv *priv = netdev_priv(dev);
-	struct veth_rq *rq = &priv->rq[0];
 	int err;
 
 	if (!(changed & NETIF_F_GRO) || !(dev->flags & IFF_UP) || priv->_xdp_prog)
 		return 0;
 
 	if (features & NETIF_F_GRO) {
-		if (!rtnl_dereference(rq->napi)) {
-			err = veth_napi_enable(dev);
-			if (err)
-				return err;
-		}
+		err = veth_napi_enable(dev);
+		if (err)
+			return err;
 	} else {
-		if (rtnl_dereference(rq->napi))
-			veth_napi_del(dev);
+		veth_napi_del(dev);
 	}
 	return 0;
 }
@@ -1542,19 +1512,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			struct netlink_ext_ack *extack)
 {
 	struct veth_priv *priv = netdev_priv(dev);
-	struct veth_priv *peer_priv;
 	struct bpf_prog *old_prog;
-	struct veth_rq *peer_rq;
 	struct net_device *peer;
-	bool napi_already_off;
 	unsigned int max_mtu;
-	bool noreq_napi;
 	int err;
 
 	old_prog = priv->_xdp_prog;
 	priv->_xdp_prog = prog;
 	peer = rtnl_dereference(priv->peer);
-	peer_priv = netdev_priv(peer);
 
 	if (prog) {
 		if (!peer) {
@@ -1591,24 +1556,6 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			}
 		}
 
-		if (peer && (peer->flags & IFF_UP)) {
-			peer_rq = &peer_priv->rq[0];
-
-			/* If the peer hasn't enabled GRO and loaded xdp,
-			 * then we enable napi automatically if its napi
-			 * is not ready.
-			 */
-			napi_already_off = !rtnl_dereference(peer_rq->napi);
-			if (napi_already_off) {
-				err = veth_napi_enable(peer);
-				if (err) {
-					NL_SET_ERR_MSG_MOD(extack,
-							   "Failed to automatically enable napi of peer");
-					goto err;
-				}
-			}
-		}
-
 		if (!old_prog) {
 			peer->hw_features &= ~NETIF_F_GSO_SOFTWARE;
 			peer->max_mtu = max_mtu;
@@ -1623,17 +1570,6 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			if (peer) {
 				peer->hw_features |= NETIF_F_GSO_SOFTWARE;
 				peer->max_mtu = ETH_MAX_MTU;
-				peer_rq = &peer_priv->rq[0];
-
-				/* If the peer doesn't has its xdp and enabled
-				 * GRO, then we disable napi if its napi is ready;
-				 */
-				if (rtnl_dereference(peer_rq->napi)) {
-					noreq_napi = !veth_gro_requested(peer) &&
-						     !peer_priv->_xdp_prog;
-					if (noreq_napi && (peer->flags & IFF_UP))
-						veth_napi_del(peer);
-				}
 			}
 		}
 		bpf_prog_put(old_prog);
-- 
2.19.1.6.gb485710b

Re: [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix

[Jakub Kicinski]

On Tue, 22 Nov 2022 11:50:13 +0800 Heng Qi wrote:
> This patch 2e0de6366ac16 enables napi of the peer veth automatically when the
> veth loads the xdp, but it breaks down as reported by Paolo and John. So reverting
> it and its fix, we will rework the patch and make it more robust based on comments.

Did anything change since the previous posting?

Re: [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix

[Heng Qi]

在 2022/11/22 下午12:35, Jakub Kicinski 写道:
> On Tue, 22 Nov 2022 11:50:13 +0800 Heng Qi wrote:
>> This patch 2e0de6366ac16 enables napi of the peer veth automatically when the
>> veth loads the xdp, but it breaks down as reported by Paolo and John. So reverting
>> it and its fix, we will rework the patch and make it more robust based on comments.
> Did anything change since the previous posting?

Do you mean this positing?
https://lore.kernel.org/all/20221121112848.51388-1-hengqi@linux.alibaba.com/

If yes, there is no difference between this posting and the posting 
posted by the link. This posting is to make it easier to merge.

Re: [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 22 Nov 2022 11:50:13 +0800 you wrote:
> This patch 2e0de6366ac16 enables napi of the peer veth automatically when the
> veth loads the xdp, but it breaks down as reported by Paolo and John. So reverting
> it and its fix, we will rework the patch and make it more robust based on comments.
> 
> Heng Qi (2):
>   Revert "bpf: veth driver panics when xdp prog attached before
>     veth_open"
>   Revert "veth: Avoid drop packets when xdp_redirect performs"
> 
> [...]

Here is the summary with links:
  - [1/2] Revert "bpf: veth driver panics when xdp prog attached before veth_open"
    https://git.kernel.org/netdev/net-next/c/b535d681adda
  - [2/2] Revert "veth: Avoid drop packets when xdp_redirect performs"
    https://git.kernel.org/netdev/net-next/c/5e8d3dc73e80

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/2] veth: a couple of fixes

[Paolo Abeni]

On Mon, 2022-11-21 at 11:55 +0800, Heng Qi wrote:
> 在 2022/11/21 上午11:33, Xuan Zhuo 写道:
> > On Fri, 18 Nov 2022 09:41:05 +0100, Paolo Abeni <pabeni@redhat.com> wrote:
> > > On Fri, 2022-11-18 at 00:33 +0100, Paolo Abeni wrote:
> > > > Recent changes in the veth driver caused a few regressions
> > > > this series addresses a couple of them, causing oops.
> > > > 
> > > > Paolo Abeni (2):
> > > >    veth: fix uninitialized napi disable
> > > >    veth: fix double napi enable
> > > > 
> > > >   drivers/net/veth.c | 6 ++++--
> > > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > @Xuan Zhuo: another option would be reverting 2e0de6366ac1 ("veth:
> > > Avoid drop packets when xdp_redirect performs") and its follow-up
> > > 5e5dc33d5dac ("bpf: veth driver panics when xdp prog attached before
> > > veth_open").
> > >  option would be possibly safer, because I feel there are other
> > > issues with 2e0de6366ac1, and would offer the opportunity to refactor
> > > its logic a bit: the napi enable/disable condition is quite complex and
> > > not used consistently mixing and alternating the gro/xdp/peer xdp check
> > > with the napi ptr dereference.
> > > 
> > > Ideally it would be better to have an helper alike
> > > napi_should_be_enabled(), use it everywhere, and pair the new code with
> > > some selftests, extending the existing ones.
> > > 
> > > WDYT?
> > I take your point.
> 
> I'll rewrite a patch as soon as possible and resubmit it.

Could you please first send the revert?

Thanks!

Paolo

[PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix

[Heng Qi]

This patch 2e0de6366ac16 enables napi of the peer veth automatically when the
veth loads the xdp, but it breaks down as reported by Paolo and John. So reverting
it and its fix, we will rework the patch and make it more robust based on comments.

Heng Qi (2):
  Revert "bpf: veth driver panics when xdp prog attached before
    veth_open"
  Revert "veth: Avoid drop packets when xdp_redirect performs"

 drivers/net/veth.c | 88 +++++++---------------------------------------
 1 file changed, 12 insertions(+), 76 deletions(-)

-- 
2.19.1.6.gb485710b