[PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx

[PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx

[Toshiaki Makita]

While I'm looking into how to account standard tx counters on XDP tx
processing, I found several bugs around XDP tx and napi_tx.

Patch1: Fix oops on error path. Patch2 depends on this.
Patch2: Fix memory corruption on freeing xdp_frames with napi_tx enabled.
Patch3: Minor fix patch5 depends on.
Patch4: Fix memory corruption on processing xdp_frames when XDP is disabled.
  Also patch5 depends on this.
Patch5: Fix memory corruption on processing xdp_frames while XDP is being
  disabled.
Patch6: Minor fix patch7 depends on.
Patch7: Fix memory corruption on freeing sk_buff or xdp_frames when a normal
  queue is reused for XDP and vise versa.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Toshiaki Makita (7):
  virtio_net: Don't enable NAPI when interface is down
  virtio_net: Don't call free_old_xmit_skbs for xdp_frames
  virtio_net: Fix not restoring real_num_rx_queues
  virtio_net: Fix out of bounds access of sq
  virtio_net: Don't process redirected XDP frames when XDP is disabled
  virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
  virtio_net: Differentiate sk_buff and xdp_frame on freeing

 drivers/net/virtio_net.c | 154 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 109 insertions(+), 45 deletions(-)

-- 
1.8.3.1

[PATCH net 1/7] virtio_net: Don't enable NAPI when interface is down

[Toshiaki Makita]

Commit 4e09ff536284 ("virtio-net: disable NAPI only when enabled during
XDP set") tried to fix inappropriate NAPI enabling/disabling when
!netif_running(), but was not complete.

On error path virtio_net could enable NAPI even when !netif_running().
This can cause enabling NAPI twice on virtnet_open(), which would
trigger BUG_ON() in napi_enable().

Fixes: 4941d472bf95b ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0237250..a08da9e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2430,8 +2430,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	return 0;
 
 err:
-	for (i = 0; i < vi->max_queue_pairs; i++)
-		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+	if (netif_running(dev)) {
+		for (i = 0; i < vi->max_queue_pairs; i++)
+			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+	}
 	if (prog)
 		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
 	return err;
-- 
1.8.3.1

[PATCH net 1/7] virtio_net: Don't enable NAPI when interface is down

[Toshiaki Makita]

Commit 4e09ff536284 ("virtio-net: disable NAPI only when enabled during
XDP set") tried to fix inappropriate NAPI enabling/disabling when
!netif_running(), but was not complete.

On error path virtio_net could enable NAPI even when !netif_running().
This can cause enabling NAPI twice on virtnet_open(), which would
trigger BUG_ON() in napi_enable().

Fixes: 4941d472bf95b ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0237250..a08da9e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2430,8 +2430,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	return 0;
 
 err:
-	for (i = 0; i < vi->max_queue_pairs; i++)
-		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+	if (netif_running(dev)) {
+		for (i = 0; i < vi->max_queue_pairs; i++)
+			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+	}
 	if (prog)
 		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
 	return err;
-- 
1.8.3.1

Re: [PATCH net 1/7] virtio_net: Don't enable NAPI when interface is down

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> Commit 4e09ff536284 ("virtio-net: disable NAPI only when enabled during
> XDP set") tried to fix inappropriate NAPI enabling/disabling when
> !netif_running(), but was not complete.
>
> On error path virtio_net could enable NAPI even when !netif_running().
> This can cause enabling NAPI twice on virtnet_open(), which would
> trigger BUG_ON() in napi_enable().
>
> Fixes: 4941d472bf95b ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0237250..a08da9e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2430,8 +2430,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   	return 0;
>   
>   err:
> -	for (i = 0; i < vi->max_queue_pairs; i++)
> -		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +	if (netif_running(dev)) {
> +		for (i = 0; i < vi->max_queue_pairs; i++)
> +			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +	}
>   	if (prog)
>   		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
>   	return err;


Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH net 1/7] virtio_net: Don't enable NAPI when interface is down

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> Commit 4e09ff536284 ("virtio-net: disable NAPI only when enabled during
> XDP set") tried to fix inappropriate NAPI enabling/disabling when
> !netif_running(), but was not complete.
>
> On error path virtio_net could enable NAPI even when !netif_running().
> This can cause enabling NAPI twice on virtnet_open(), which would
> trigger BUG_ON() in napi_enable().
>
> Fixes: 4941d472bf95b ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0237250..a08da9e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2430,8 +2430,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   	return 0;
>   
>   err:
> -	for (i = 0; i < vi->max_queue_pairs; i++)
> -		virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +	if (netif_running(dev)) {
> +		for (i = 0; i < vi->max_queue_pairs; i++)
> +			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +	}
>   	if (prog)
>   		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
>   	return err;


Acked-by: Jason Wang <jasowang@redhat.com>


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Toshiaki Makita]

When napi_tx is enabled, virtnet_poll_cleantx() called
free_old_xmit_skbs() even for xdp send queue.
This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
device tx bytes counters because skb->len is meaningless value, and even
triggered oops due to general protection fault on freeing them.

Since xdp send queues do not aquire locks, old xdp_frames should be
freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
xdp send queues.

Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
handler is called even without calling start_xmit() because cb for tx is
by default enabled. Once the handler is called, it enabled the cb again,
and then the handler would be called again. We don't need this handler
for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().

Also, we need to disable tx NAPI when disabling XDP, so
virtnet_poll_tx() can safely access curr_queue_pairs and
xdp_queue_pairs, which are not atomically updated while disabling XDP.

Fixes: b92f1e6751a6 ("virtio-net: transmit napi")
Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a08da9e..7d35e6d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq)
 	u64_stats_update_end(&sq->stats.syncp);
 }
 
+static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
+{
+	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
+		return false;
+	else if (q < vi->curr_queue_pairs)
+		return true;
+	else
+		return false;
+}
+
 static void virtnet_poll_cleantx(struct receive_queue *rq)
 {
 	struct virtnet_info *vi = rq->vq->vdev->priv;
@@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
 	struct send_queue *sq = &vi->sq[index];
 	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index);
 
-	if (!sq->napi.weight)
+	if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index))
 		return;
 
 	if (__netif_tx_trylock(txq)) {
@@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
 {
 	struct send_queue *sq = container_of(napi, struct send_queue, napi);
 	struct virtnet_info *vi = sq->vq->vdev->priv;
-	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
+	unsigned int index = vq2txq(sq->vq);
+	struct netdev_queue *txq;
 
+	if (unlikely(is_xdp_raw_buffer_queue(vi, index))) {
+		/* We don't need to enable cb for XDP */
+		napi_complete_done(napi, 0);
+		return 0;
+	}
+
+	txq = netdev_get_tx_queue(vi->dev, index);
 	__netif_tx_lock(txq, raw_smp_processor_id());
 	free_old_xmit_skbs(sq);
 	__netif_tx_unlock(txq);
@@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	}
 
 	/* Make sure NAPI is not using any XDP TX queues for RX. */
-	if (netif_running(dev))
-		for (i = 0; i < vi->max_queue_pairs; i++)
+	if (netif_running(dev)) {
+		for (i = 0; i < vi->max_queue_pairs; i++) {
 			napi_disable(&vi->rq[i].napi);
+			virtnet_napi_tx_disable(&vi->sq[i].napi);
+		}
+	}
 
 	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
 	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
@@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 		if (old_prog)
 			bpf_prog_put(old_prog);
-		if (netif_running(dev))
+		if (netif_running(dev)) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
+					       &vi->sq[i].napi);
+		}
 	}
 
 	return 0;
 
 err:
 	if (netif_running(dev)) {
-		for (i = 0; i < vi->max_queue_pairs; i++)
+		for (i = 0; i < vi->max_queue_pairs; i++) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
+					       &vi->sq[i].napi);
+		}
 	}
 	if (prog)
 		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
@@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi)
 			put_page(vi->rq[i].alloc_frag.page);
 }
 
-static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
-{
-	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
-		return false;
-	else if (q < vi->curr_queue_pairs)
-		return true;
-	else
-		return false;
-}
-
 static void free_unused_bufs(struct virtnet_info *vi)
 {
 	void *buf;
-- 
1.8.3.1

Re: [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When napi_tx is enabled, virtnet_poll_cleantx() called
> free_old_xmit_skbs() even for xdp send queue.
> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
> device tx bytes counters because skb->len is meaningless value, and even
> triggered oops due to general protection fault on freeing them.
>
> Since xdp send queues do not aquire locks, old xdp_frames should be
> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
> xdp send queues.
>
> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
> handler is called even without calling start_xmit() because cb for tx is
> by default enabled. Once the handler is called, it enabled the cb again,
> and then the handler would be called again. We don't need this handler
> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
>
> Also, we need to disable tx NAPI when disabling XDP, so
> virtnet_poll_tx() can safely access curr_queue_pairs and
> xdp_queue_pairs, which are not atomically updated while disabling XDP.


I suggest to split this into another patch or squash this part to patch 1.

Other looks good.

Thanks


>
> Fixes: b92f1e6751a6 ("virtio-net: transmit napi")
> Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++----------------
>   1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a08da9e..7d35e6d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq)
>   	u64_stats_update_end(&sq->stats.syncp);
>   }
>   
> +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> +{
> +	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
> +		return false;
> +	else if (q < vi->curr_queue_pairs)
> +		return true;
> +	else
> +		return false;
> +}
> +
>   static void virtnet_poll_cleantx(struct receive_queue *rq)
>   {
>   	struct virtnet_info *vi = rq->vq->vdev->priv;
> @@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
>   	struct send_queue *sq = &vi->sq[index];
>   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index);
>   
> -	if (!sq->napi.weight)
> +	if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index))
>   		return;
>   
>   	if (__netif_tx_trylock(txq)) {
> @@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
>   {
>   	struct send_queue *sq = container_of(napi, struct send_queue, napi);
>   	struct virtnet_info *vi = sq->vq->vdev->priv;
> -	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
> +	unsigned int index = vq2txq(sq->vq);
> +	struct netdev_queue *txq;
>   
> +	if (unlikely(is_xdp_raw_buffer_queue(vi, index))) {
> +		/* We don't need to enable cb for XDP */
> +		napi_complete_done(napi, 0);
> +		return 0;
> +	}
> +
> +	txq = netdev_get_tx_queue(vi->dev, index);
>   	__netif_tx_lock(txq, raw_smp_processor_id());
>   	free_old_xmit_skbs(sq);
>   	__netif_tx_unlock(txq);
> @@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   	}
>   
>   	/* Make sure NAPI is not using any XDP TX queues for RX. */
> -	if (netif_running(dev))
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> +	if (netif_running(dev)) {
> +		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			napi_disable(&vi->rq[i].napi);
> +			virtnet_napi_tx_disable(&vi->sq[i].napi);
> +		}
> +	}
>   
>   	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>   	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> @@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		}
>   		if (old_prog)
>   			bpf_prog_put(old_prog);
> -		if (netif_running(dev))
> +		if (netif_running(dev)) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
> +					       &vi->sq[i].napi);
> +		}
>   	}
>   
>   	return 0;
>   
>   err:
>   	if (netif_running(dev)) {
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> +		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
> +					       &vi->sq[i].napi);
> +		}
>   	}
>   	if (prog)
>   		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
> @@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi)
>   			put_page(vi->rq[i].alloc_frag.page);
>   }
>   
> -static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> -{
> -	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
> -		return false;
> -	else if (q < vi->curr_queue_pairs)
> -		return true;
> -	else
> -		return false;
> -}
> -
>   static void free_unused_bufs(struct virtnet_info *vi)
>   {
>   	void *buf;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When napi_tx is enabled, virtnet_poll_cleantx() called
> free_old_xmit_skbs() even for xdp send queue.
> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
> device tx bytes counters because skb->len is meaningless value, and even
> triggered oops due to general protection fault on freeing them.
>
> Since xdp send queues do not aquire locks, old xdp_frames should be
> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
> xdp send queues.
>
> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
> handler is called even without calling start_xmit() because cb for tx is
> by default enabled. Once the handler is called, it enabled the cb again,
> and then the handler would be called again. We don't need this handler
> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
>
> Also, we need to disable tx NAPI when disabling XDP, so
> virtnet_poll_tx() can safely access curr_queue_pairs and
> xdp_queue_pairs, which are not atomically updated while disabling XDP.


I suggest to split this into another patch or squash this part to patch 1.

Other looks good.

Thanks


>
> Fixes: b92f1e6751a6 ("virtio-net: transmit napi")
> Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++----------------
>   1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a08da9e..7d35e6d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq)
>   	u64_stats_update_end(&sq->stats.syncp);
>   }
>   
> +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> +{
> +	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
> +		return false;
> +	else if (q < vi->curr_queue_pairs)
> +		return true;
> +	else
> +		return false;
> +}
> +
>   static void virtnet_poll_cleantx(struct receive_queue *rq)
>   {
>   	struct virtnet_info *vi = rq->vq->vdev->priv;
> @@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
>   	struct send_queue *sq = &vi->sq[index];
>   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index);
>   
> -	if (!sq->napi.weight)
> +	if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index))
>   		return;
>   
>   	if (__netif_tx_trylock(txq)) {
> @@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
>   {
>   	struct send_queue *sq = container_of(napi, struct send_queue, napi);
>   	struct virtnet_info *vi = sq->vq->vdev->priv;
> -	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
> +	unsigned int index = vq2txq(sq->vq);
> +	struct netdev_queue *txq;
>   
> +	if (unlikely(is_xdp_raw_buffer_queue(vi, index))) {
> +		/* We don't need to enable cb for XDP */
> +		napi_complete_done(napi, 0);
> +		return 0;
> +	}
> +
> +	txq = netdev_get_tx_queue(vi->dev, index);
>   	__netif_tx_lock(txq, raw_smp_processor_id());
>   	free_old_xmit_skbs(sq);
>   	__netif_tx_unlock(txq);
> @@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   	}
>   
>   	/* Make sure NAPI is not using any XDP TX queues for RX. */
> -	if (netif_running(dev))
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> +	if (netif_running(dev)) {
> +		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			napi_disable(&vi->rq[i].napi);
> +			virtnet_napi_tx_disable(&vi->sq[i].napi);
> +		}
> +	}
>   
>   	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>   	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> @@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		}
>   		if (old_prog)
>   			bpf_prog_put(old_prog);
> -		if (netif_running(dev))
> +		if (netif_running(dev)) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
> +					       &vi->sq[i].napi);
> +		}
>   	}
>   
>   	return 0;
>   
>   err:
>   	if (netif_running(dev)) {
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> +		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> +			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
> +					       &vi->sq[i].napi);
> +		}
>   	}
>   	if (prog)
>   		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
> @@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi)
>   			put_page(vi->rq[i].alloc_frag.page);
>   }
>   
> -static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> -{
> -	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
> -		return false;
> -	else if (q < vi->curr_queue_pairs)
> -		return true;
> -	else
> -		return false;
> -}
> -
>   static void free_unused_bufs(struct virtnet_info *vi)
>   {
>   	void *buf;

Re: [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Toshiaki Makita]

On 2019/01/17 21:39, Jason Wang wrote:
> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>> When napi_tx is enabled, virtnet_poll_cleantx() called
>> free_old_xmit_skbs() even for xdp send queue.
>> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
>> device tx bytes counters because skb->len is meaningless value, and even
>> triggered oops due to general protection fault on freeing them.
>>
>> Since xdp send queues do not aquire locks, old xdp_frames should be
>> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
>> xdp send queues.
>>
>> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
>> handler is called even without calling start_xmit() because cb for tx is
>> by default enabled. Once the handler is called, it enabled the cb again,
>> and then the handler would be called again. We don't need this handler
>> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
>>
>> Also, we need to disable tx NAPI when disabling XDP, so
>> virtnet_poll_tx() can safely access curr_queue_pairs and
>> xdp_queue_pairs, which are not atomically updated while disabling XDP.
> 
> 
> I suggest to split this into another patch or squash this part to patch 1.

This part is for invocation of is_xdp_raw_buffer_queue() from
virtnet_poll_tx(), which is added in this patch, so I'm thinking it's
more natural to keep this hunk in this patch.

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Toshiaki Makita]

On 2019/01/17 21:39, Jason Wang wrote:
> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>> When napi_tx is enabled, virtnet_poll_cleantx() called
>> free_old_xmit_skbs() even for xdp send queue.
>> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
>> device tx bytes counters because skb->len is meaningless value, and even
>> triggered oops due to general protection fault on freeing them.
>>
>> Since xdp send queues do not aquire locks, old xdp_frames should be
>> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
>> xdp send queues.
>>
>> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
>> handler is called even without calling start_xmit() because cb for tx is
>> by default enabled. Once the handler is called, it enabled the cb again,
>> and then the handler would be called again. We don't need this handler
>> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
>>
>> Also, we need to disable tx NAPI when disabling XDP, so
>> virtnet_poll_tx() can safely access curr_queue_pairs and
>> xdp_queue_pairs, which are not atomically updated while disabling XDP.
> 
> 
> I suggest to split this into another patch or squash this part to patch 1.

This part is for invocation of is_xdp_raw_buffer_queue() from
virtnet_poll_tx(), which is added in this patch, so I'm thinking it's
more natural to keep this hunk in this patch.

-- 
Toshiaki Makita

Re: [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Jason Wang]

On 2019/1/18 上午9:44, Toshiaki Makita wrote:
> On 2019/01/17 21:39, Jason Wang wrote:
>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>> When napi_tx is enabled, virtnet_poll_cleantx() called
>>> free_old_xmit_skbs() even for xdp send queue.
>>> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
>>> device tx bytes counters because skb->len is meaningless value, and even
>>> triggered oops due to general protection fault on freeing them.
>>>
>>> Since xdp send queues do not aquire locks, old xdp_frames should be
>>> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
>>> xdp send queues.
>>>
>>> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
>>> handler is called even without calling start_xmit() because cb for tx is
>>> by default enabled. Once the handler is called, it enabled the cb again,
>>> and then the handler would be called again. We don't need this handler
>>> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
>>>
>>> Also, we need to disable tx NAPI when disabling XDP, so
>>> virtnet_poll_tx() can safely access curr_queue_pairs and
>>> xdp_queue_pairs, which are not atomically updated while disabling XDP.
>>
>> I suggest to split this into another patch or squash this part to patch 1.
> This part is for invocation of is_xdp_raw_buffer_queue() from
> virtnet_poll_tx(), which is added in this patch, so I'm thinking it's
> more natural to keep this hunk in this patch.


I see.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Jason Wang]

On 2019/1/18 上午9:44, Toshiaki Makita wrote:
> On 2019/01/17 21:39, Jason Wang wrote:
>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>> When napi_tx is enabled, virtnet_poll_cleantx() called
>>> free_old_xmit_skbs() even for xdp send queue.
>>> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
>>> device tx bytes counters because skb->len is meaningless value, and even
>>> triggered oops due to general protection fault on freeing them.
>>>
>>> Since xdp send queues do not aquire locks, old xdp_frames should be
>>> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
>>> xdp send queues.
>>>
>>> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
>>> handler is called even without calling start_xmit() because cb for tx is
>>> by default enabled. Once the handler is called, it enabled the cb again,
>>> and then the handler would be called again. We don't need this handler
>>> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
>>>
>>> Also, we need to disable tx NAPI when disabling XDP, so
>>> virtnet_poll_tx() can safely access curr_queue_pairs and
>>> xdp_queue_pairs, which are not atomically updated while disabling XDP.
>>
>> I suggest to split this into another patch or squash this part to patch 1.
> This part is for invocation of is_xdp_raw_buffer_queue() from
> virtnet_poll_tx(), which is added in this patch, so I'm thinking it's
> more natural to keep this hunk in this patch.


I see.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

[PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames

[Toshiaki Makita]

When napi_tx is enabled, virtnet_poll_cleantx() called
free_old_xmit_skbs() even for xdp send queue.
This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
device tx bytes counters because skb->len is meaningless value, and even
triggered oops due to general protection fault on freeing them.

Since xdp send queues do not aquire locks, old xdp_frames should be
freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
xdp send queues.

Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
handler is called even without calling start_xmit() because cb for tx is
by default enabled. Once the handler is called, it enabled the cb again,
and then the handler would be called again. We don't need this handler
for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().

Also, we need to disable tx NAPI when disabling XDP, so
virtnet_poll_tx() can safely access curr_queue_pairs and
xdp_queue_pairs, which are not atomically updated while disabling XDP.

Fixes: b92f1e6751a6 ("virtio-net: transmit napi")
Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a08da9e..7d35e6d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq)
 	u64_stats_update_end(&sq->stats.syncp);
 }
 
+static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
+{
+	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
+		return false;
+	else if (q < vi->curr_queue_pairs)
+		return true;
+	else
+		return false;
+}
+
 static void virtnet_poll_cleantx(struct receive_queue *rq)
 {
 	struct virtnet_info *vi = rq->vq->vdev->priv;
@@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
 	struct send_queue *sq = &vi->sq[index];
 	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index);
 
-	if (!sq->napi.weight)
+	if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index))
 		return;
 
 	if (__netif_tx_trylock(txq)) {
@@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
 {
 	struct send_queue *sq = container_of(napi, struct send_queue, napi);
 	struct virtnet_info *vi = sq->vq->vdev->priv;
-	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
+	unsigned int index = vq2txq(sq->vq);
+	struct netdev_queue *txq;
 
+	if (unlikely(is_xdp_raw_buffer_queue(vi, index))) {
+		/* We don't need to enable cb for XDP */
+		napi_complete_done(napi, 0);
+		return 0;
+	}
+
+	txq = netdev_get_tx_queue(vi->dev, index);
 	__netif_tx_lock(txq, raw_smp_processor_id());
 	free_old_xmit_skbs(sq);
 	__netif_tx_unlock(txq);
@@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	}
 
 	/* Make sure NAPI is not using any XDP TX queues for RX. */
-	if (netif_running(dev))
-		for (i = 0; i < vi->max_queue_pairs; i++)
+	if (netif_running(dev)) {
+		for (i = 0; i < vi->max_queue_pairs; i++) {
 			napi_disable(&vi->rq[i].napi);
+			virtnet_napi_tx_disable(&vi->sq[i].napi);
+		}
+	}
 
 	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
 	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
@@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 		if (old_prog)
 			bpf_prog_put(old_prog);
-		if (netif_running(dev))
+		if (netif_running(dev)) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
+					       &vi->sq[i].napi);
+		}
 	}
 
 	return 0;
 
 err:
 	if (netif_running(dev)) {
-		for (i = 0; i < vi->max_queue_pairs; i++)
+		for (i = 0; i < vi->max_queue_pairs; i++) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
+					       &vi->sq[i].napi);
+		}
 	}
 	if (prog)
 		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
@@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi)
 			put_page(vi->rq[i].alloc_frag.page);
 }
 
-static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
-{
-	if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
-		return false;
-	else if (q < vi->curr_queue_pairs)
-		return true;
-	else
-		return false;
-}
-
 static void free_unused_bufs(struct virtnet_info *vi)
 {
 	void *buf;
-- 
1.8.3.1

[PATCH net 3/7] virtio_net: Fix not restoring real_num_rx_queues

[Toshiaki Makita]

When _virtnet_set_queues() failed we did not restore real_num_rx_queues.
Fix this by placing the change of real_num_rx_queues after
_virtnet_set_queues().
This order is also in line with virtnet_set_channels().

Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7d35e6d..670cc15 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2427,10 +2427,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 	}
 
-	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
 	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
 	if (err)
 		goto err;
+	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
 	vi->xdp_queue_pairs = xdp_qp;
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-- 
1.8.3.1

Re: [PATCH net 3/7] virtio_net: Fix not restoring real_num_rx_queues

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When _virtnet_set_queues() failed we did not restore real_num_rx_queues.
> Fix this by placing the change of real_num_rx_queues after
> _virtnet_set_queues().
> This order is also in line with virtnet_set_channels().
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7d35e6d..670cc15 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2427,10 +2427,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		}
>   	}
>   
> -	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>   	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>   	if (err)
>   		goto err;
> +	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>   	vi->xdp_queue_pairs = xdp_qp;
>   
>   	for (i = 0; i < vi->max_queue_pairs; i++) {


Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH net 3/7] virtio_net: Fix not restoring real_num_rx_queues

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When _virtnet_set_queues() failed we did not restore real_num_rx_queues.
> Fix this by placing the change of real_num_rx_queues after
> _virtnet_set_queues().
> This order is also in line with virtnet_set_channels().
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7d35e6d..670cc15 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2427,10 +2427,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		}
>   	}
>   
> -	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>   	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>   	if (err)
>   		goto err;
> +	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>   	vi->xdp_queue_pairs = xdp_qp;
>   
>   	for (i = 0; i < vi->max_queue_pairs; i++) {


Acked-by: Jason Wang <jasowang@redhat.com>


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net 3/7] virtio_net: Fix not restoring real_num_rx_queues

[Toshiaki Makita]

When _virtnet_set_queues() failed we did not restore real_num_rx_queues.
Fix this by placing the change of real_num_rx_queues after
_virtnet_set_queues().
This order is also in line with virtnet_set_channels().

Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7d35e6d..670cc15 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2427,10 +2427,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 	}
 
-	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
 	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
 	if (err)
 		goto err;
+	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
 	vi->xdp_queue_pairs = xdp_qp;
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-- 
1.8.3.1

[PATCH net 4/7] virtio_net: Fix out of bounds access of sq

[Toshiaki Makita]

When XDP is disabled, curr_queue_pairs + smp_processor_id() can be
larger than max_queue_pairs.
There is no guarantee that we have enough XDP send queues dedicated for
each cpu when XDP is disabled, so do not count drops on sq in that case.

Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 670cc15..204eedf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -491,20 +491,17 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	int ret, err;
 	int i;
 
-	sq = virtnet_xdp_sq(vi);
-
-	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
-		ret = -EINVAL;
-		drops = n;
-		goto out;
-	}
-
 	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
 	 * indicate XDP resources have been successfully allocated.
 	 */
 	xdp_prog = rcu_dereference(rq->xdp_prog);
-	if (!xdp_prog) {
-		ret = -ENXIO;
+	if (!xdp_prog)
+		return -ENXIO;
+
+	sq = virtnet_xdp_sq(vi);
+
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
+		ret = -EINVAL;
 		drops = n;
 		goto out;
 	}
-- 
1.8.3.1

[PATCH net 4/7] virtio_net: Fix out of bounds access of sq

[Toshiaki Makita]

When XDP is disabled, curr_queue_pairs + smp_processor_id() can be
larger than max_queue_pairs.
There is no guarantee that we have enough XDP send queues dedicated for
each cpu when XDP is disabled, so do not count drops on sq in that case.

Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 670cc15..204eedf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -491,20 +491,17 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	int ret, err;
 	int i;
 
-	sq = virtnet_xdp_sq(vi);
-
-	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
-		ret = -EINVAL;
-		drops = n;
-		goto out;
-	}
-
 	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
 	 * indicate XDP resources have been successfully allocated.
 	 */
 	xdp_prog = rcu_dereference(rq->xdp_prog);
-	if (!xdp_prog) {
-		ret = -ENXIO;
+	if (!xdp_prog)
+		return -ENXIO;
+
+	sq = virtnet_xdp_sq(vi);
+
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
+		ret = -EINVAL;
 		drops = n;
 		goto out;
 	}
-- 
1.8.3.1

Re: [PATCH net 4/7] virtio_net: Fix out of bounds access of sq

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When XDP is disabled, curr_queue_pairs + smp_processor_id() can be
> larger than max_queue_pairs.
> There is no guarantee that we have enough XDP send queues dedicated for
> each cpu when XDP is disabled, so do not count drops on sq in that case.
>
> Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 670cc15..204eedf 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -491,20 +491,17 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>   	int ret, err;
>   	int i;
>   
> -	sq = virtnet_xdp_sq(vi);
> -
> -	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
> -		ret = -EINVAL;
> -		drops = n;
> -		goto out;
> -	}
> -
>   	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
>   	 * indicate XDP resources have been successfully allocated.
>   	 */
>   	xdp_prog = rcu_dereference(rq->xdp_prog);
> -	if (!xdp_prog) {
> -		ret = -ENXIO;
> +	if (!xdp_prog)
> +		return -ENXIO;
> +
> +	sq = virtnet_xdp_sq(vi);
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
> +		ret = -EINVAL;
>   		drops = n;
>   		goto out;
>   	}


Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH net 4/7] virtio_net: Fix out of bounds access of sq

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When XDP is disabled, curr_queue_pairs + smp_processor_id() can be
> larger than max_queue_pairs.
> There is no guarantee that we have enough XDP send queues dedicated for
> each cpu when XDP is disabled, so do not count drops on sq in that case.
>
> Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 670cc15..204eedf 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -491,20 +491,17 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>   	int ret, err;
>   	int i;
>   
> -	sq = virtnet_xdp_sq(vi);
> -
> -	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
> -		ret = -EINVAL;
> -		drops = n;
> -		goto out;
> -	}
> -
>   	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
>   	 * indicate XDP resources have been successfully allocated.
>   	 */
>   	xdp_prog = rcu_dereference(rq->xdp_prog);
> -	if (!xdp_prog) {
> -		ret = -ENXIO;
> +	if (!xdp_prog)
> +		return -ENXIO;
> +
> +	sq = virtnet_xdp_sq(vi);
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
> +		ret = -EINVAL;
>   		drops = n;
>   		goto out;
>   	}


Acked-by: Jason Wang <jasowang@redhat.com>


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Toshiaki Makita]

Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
ready for XDP") tried to avoid access to unexpected sq while XDP is
disabled, but was not complete.

There was a small window which causes out of bounds sq access in
virtnet_xdp_xmit() while disabling XDP.

An example case of
 - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
 - online_cpu_num = xdp_queue_paris = 4
when XDP is enabled:

CPU 0                         CPU 1
(Disabling XDP)               (Processing redirected XDP frames)

                              virtnet_xdp_xmit()
virtnet_xdp_set()
 _virtnet_set_queues()
  set curr_queue_pairs (2)
                               check if rq->xdp_prog is not NULL
                               virtnet_xdp_sq(vi)
                                qp = curr_queue_pairs -
                                     xdp_queue_pairs +
                                     smp_processor_id()
                                   = 2 - 4 + 1 = -1
                                sq = &vi->sq[qp] // out of bounds access
  set xdp_queue_pairs (0)
  rq->xdp_prog = NULL

Basically we should not change curr_queue_pairs and xdp_queue_pairs
while someone can read the values. Thus, when disabling XDP, assign NULL
to rq->xdp_prog first, and wait for RCU grace period, then change
xxx_queue_pairs.
Note that we need to keep the current order when enabling XDP though.

Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 204eedf..ae93f0e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 	}
 
-	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
-	if (err)
-		goto err;
-	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
-	vi->xdp_queue_pairs = xdp_qp;
+	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
+	if (!old_prog && prog) {
+		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+		if (err)
+			goto err_new_prog;
+		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
+		vi->xdp_queue_pairs = xdp_qp;
+	}
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-		old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
 		rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
 		if (i == 0) {
 			if (!old_prog)
@@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			if (!prog)
 				virtnet_restore_guest_offloads(vi);
 		}
+	}
+
+	if (old_prog && !prog) {
+		synchronize_net();
+		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+		if (err)
+			goto err_old_prog;
+		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
+		vi->xdp_queue_pairs = xdp_qp;
+	}
+
+	for (i = 0; i < vi->max_queue_pairs; i++) {
 		if (old_prog)
 			bpf_prog_put(old_prog);
 		if (netif_running(dev)) {
@@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 
 	return 0;
 
-err:
+err_old_prog:
+	virtnet_clear_guest_offloads(vi);
+	for (i = 0; i < vi->max_queue_pairs; i++)
+		rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
+err_new_prog:
 	if (netif_running(dev)) {
 		for (i = 0; i < vi->max_queue_pairs; i++) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
-- 
1.8.3.1

[PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Toshiaki Makita]

Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
ready for XDP") tried to avoid access to unexpected sq while XDP is
disabled, but was not complete.

There was a small window which causes out of bounds sq access in
virtnet_xdp_xmit() while disabling XDP.

An example case of
 - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
 - online_cpu_num = xdp_queue_paris = 4
when XDP is enabled:

CPU 0                         CPU 1
(Disabling XDP)               (Processing redirected XDP frames)

                              virtnet_xdp_xmit()
virtnet_xdp_set()
 _virtnet_set_queues()
  set curr_queue_pairs (2)
                               check if rq->xdp_prog is not NULL
                               virtnet_xdp_sq(vi)
                                qp = curr_queue_pairs -
                                     xdp_queue_pairs +
                                     smp_processor_id()
                                   = 2 - 4 + 1 = -1
                                sq = &vi->sq[qp] // out of bounds access
  set xdp_queue_pairs (0)
  rq->xdp_prog = NULL

Basically we should not change curr_queue_pairs and xdp_queue_pairs
while someone can read the values. Thus, when disabling XDP, assign NULL
to rq->xdp_prog first, and wait for RCU grace period, then change
xxx_queue_pairs.
Note that we need to keep the current order when enabling XDP though.

Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 204eedf..ae93f0e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		}
 	}
 
-	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
-	if (err)
-		goto err;
-	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
-	vi->xdp_queue_pairs = xdp_qp;
+	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
+	if (!old_prog && prog) {
+		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+		if (err)
+			goto err_new_prog;
+		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
+		vi->xdp_queue_pairs = xdp_qp;
+	}
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-		old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
 		rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
 		if (i == 0) {
 			if (!old_prog)
@@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			if (!prog)
 				virtnet_restore_guest_offloads(vi);
 		}
+	}
+
+	if (old_prog && !prog) {
+		synchronize_net();
+		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+		if (err)
+			goto err_old_prog;
+		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
+		vi->xdp_queue_pairs = xdp_qp;
+	}
+
+	for (i = 0; i < vi->max_queue_pairs; i++) {
 		if (old_prog)
 			bpf_prog_put(old_prog);
 		if (netif_running(dev)) {
@@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 
 	return 0;
 
-err:
+err_old_prog:
+	virtnet_clear_guest_offloads(vi);
+	for (i = 0; i < vi->max_queue_pairs; i++)
+		rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
+err_new_prog:
 	if (netif_running(dev)) {
 		for (i = 0; i < vi->max_queue_pairs; i++) {
 			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
-- 
1.8.3.1

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
> ready for XDP") tried to avoid access to unexpected sq while XDP is
> disabled, but was not complete.
>
> There was a small window which causes out of bounds sq access in
> virtnet_xdp_xmit() while disabling XDP.
>
> An example case of
>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>   - online_cpu_num = xdp_queue_paris = 4
> when XDP is enabled:
>
> CPU 0                         CPU 1
> (Disabling XDP)               (Processing redirected XDP frames)
>
>                                virtnet_xdp_xmit()
> virtnet_xdp_set()
>   _virtnet_set_queues()
>    set curr_queue_pairs (2)
>                                 check if rq->xdp_prog is not NULL
>                                 virtnet_xdp_sq(vi)
>                                  qp = curr_queue_pairs -
>                                       xdp_queue_pairs +
>                                       smp_processor_id()
>                                     = 2 - 4 + 1 = -1
>                                  sq = &vi->sq[qp] // out of bounds access
>    set xdp_queue_pairs (0)
>    rq->xdp_prog = NULL
>
> Basically we should not change curr_queue_pairs and xdp_queue_pairs
> while someone can read the values. Thus, when disabling XDP, assign NULL
> to rq->xdp_prog first, and wait for RCU grace period, then change
> xxx_queue_pairs.
> Note that we need to keep the current order when enabling XDP though.
>
> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>


I wonder whether or not we could simply do:


if (prog) {

     rcu_assign_pointer()

     synchronize_net()

}

set queues

if (!prog) {

     rcu_assign_pointer()

}

Thanks


> ---
>   drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 204eedf..ae93f0e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		}
>   	}
>   
> -	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> -	if (err)
> -		goto err;
> -	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> -	vi->xdp_queue_pairs = xdp_qp;
> +	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
> +	if (!old_prog && prog) {
> +		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> +		if (err)
> +			goto err_new_prog;
> +		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> +		vi->xdp_queue_pairs = xdp_qp;
> +	}
>   
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
> -		old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>   		rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>   		if (i == 0) {
>   			if (!old_prog)
> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   			if (!prog)
>   				virtnet_restore_guest_offloads(vi);
>   		}
> +	}
> +
> +	if (old_prog && !prog) {
> +		synchronize_net();
> +		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> +		if (err)
> +			goto err_old_prog;
> +		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> +		vi->xdp_queue_pairs = xdp_qp;
> +	}
> +
> +	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		if (old_prog)
>   			bpf_prog_put(old_prog);
>   		if (netif_running(dev)) {
> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   
>   	return 0;
>   
> -err:
> +err_old_prog:
> +	virtnet_clear_guest_offloads(vi);
> +	for (i = 0; i < vi->max_queue_pairs; i++)
> +		rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
> +err_new_prog:
>   	if (netif_running(dev)) {
>   		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
> ready for XDP") tried to avoid access to unexpected sq while XDP is
> disabled, but was not complete.
>
> There was a small window which causes out of bounds sq access in
> virtnet_xdp_xmit() while disabling XDP.
>
> An example case of
>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>   - online_cpu_num = xdp_queue_paris = 4
> when XDP is enabled:
>
> CPU 0                         CPU 1
> (Disabling XDP)               (Processing redirected XDP frames)
>
>                                virtnet_xdp_xmit()
> virtnet_xdp_set()
>   _virtnet_set_queues()
>    set curr_queue_pairs (2)
>                                 check if rq->xdp_prog is not NULL
>                                 virtnet_xdp_sq(vi)
>                                  qp = curr_queue_pairs -
>                                       xdp_queue_pairs +
>                                       smp_processor_id()
>                                     = 2 - 4 + 1 = -1
>                                  sq = &vi->sq[qp] // out of bounds access
>    set xdp_queue_pairs (0)
>    rq->xdp_prog = NULL
>
> Basically we should not change curr_queue_pairs and xdp_queue_pairs
> while someone can read the values. Thus, when disabling XDP, assign NULL
> to rq->xdp_prog first, and wait for RCU grace period, then change
> xxx_queue_pairs.
> Note that we need to keep the current order when enabling XDP though.
>
> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>


I wonder whether or not we could simply do:


if (prog) {

     rcu_assign_pointer()

     synchronize_net()

}

set queues

if (!prog) {

     rcu_assign_pointer()

}

Thanks


> ---
>   drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 204eedf..ae93f0e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		}
>   	}
>   
> -	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> -	if (err)
> -		goto err;
> -	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> -	vi->xdp_queue_pairs = xdp_qp;
> +	old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
> +	if (!old_prog && prog) {
> +		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> +		if (err)
> +			goto err_new_prog;
> +		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> +		vi->xdp_queue_pairs = xdp_qp;
> +	}
>   
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
> -		old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>   		rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>   		if (i == 0) {
>   			if (!old_prog)
> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   			if (!prog)
>   				virtnet_restore_guest_offloads(vi);
>   		}
> +	}
> +
> +	if (old_prog && !prog) {
> +		synchronize_net();
> +		err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> +		if (err)
> +			goto err_old_prog;
> +		netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> +		vi->xdp_queue_pairs = xdp_qp;
> +	}
> +
> +	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		if (old_prog)
>   			bpf_prog_put(old_prog);
>   		if (netif_running(dev)) {
> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   
>   	return 0;
>   
> -err:
> +err_old_prog:
> +	virtnet_clear_guest_offloads(vi);
> +	for (i = 0; i < vi->max_queue_pairs; i++)
> +		rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
> +err_new_prog:
>   	if (netif_running(dev)) {
>   		for (i = 0; i < vi->max_queue_pairs; i++) {
>   			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Jason Wang]

On 2019/1/17 下午8:53, Jason Wang wrote:
>
> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>> disabled, but was not complete.
>>
>> There was a small window which causes out of bounds sq access in
>> virtnet_xdp_xmit() while disabling XDP.
>>
>> An example case of
>>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>   - online_cpu_num = xdp_queue_paris = 4
>> when XDP is enabled:
>>
>> CPU 0                         CPU 1
>> (Disabling XDP)               (Processing redirected XDP frames)
>>
>>                                virtnet_xdp_xmit()
>> virtnet_xdp_set()
>>   _virtnet_set_queues()
>>    set curr_queue_pairs (2)
>>                                 check if rq->xdp_prog is not NULL
>>                                 virtnet_xdp_sq(vi)
>>                                  qp = curr_queue_pairs -
>>                                       xdp_queue_pairs +
>>                                       smp_processor_id()
>>                                     = 2 - 4 + 1 = -1
>>                                  sq = &vi->sq[qp] // out of bounds 
>> access
>>    set xdp_queue_pairs (0)
>>    rq->xdp_prog = NULL
>>
>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>> while someone can read the values. Thus, when disabling XDP, assign NULL
>> to rq->xdp_prog first, and wait for RCU grace period, then change
>> xxx_queue_pairs.
>> Note that we need to keep the current order when enabling XDP though.
>>
>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>
>
> I wonder whether or not we could simply do:
>
>
> if (prog) {


Should be !prog


>
>     rcu_assign_pointer()
>
>     synchronize_net()
>
> }
>
> set queues
>
> if (!prog) {


Should be prog.

Thanks


>
>     rcu_assign_pointer()
>
> }
>
> Thanks
>
>
>> ---
>>   drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 204eedf..ae93f0e 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device 
>> *dev, struct bpf_prog *prog,
>>           }
>>       }
>>   -    err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> -    if (err)
>> -        goto err;
>> -    netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> -    vi->xdp_queue_pairs = xdp_qp;
>> +    old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
>> +    if (!old_prog && prog) {
>> +        err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> +        if (err)
>> +            goto err_new_prog;
>> +        netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> +        vi->xdp_queue_pairs = xdp_qp;
>> +    }
>>         for (i = 0; i < vi->max_queue_pairs; i++) {
>> -        old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>>           rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>>           if (i == 0) {
>>               if (!old_prog)
>> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device 
>> *dev, struct bpf_prog *prog,
>>               if (!prog)
>>                   virtnet_restore_guest_offloads(vi);
>>           }
>> +    }
>> +
>> +    if (old_prog && !prog) {
>> +        synchronize_net();
>> +        err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> +        if (err)
>> +            goto err_old_prog;
>> +        netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> +        vi->xdp_queue_pairs = xdp_qp;
>> +    }
>> +
>> +    for (i = 0; i < vi->max_queue_pairs; i++) {
>>           if (old_prog)
>>               bpf_prog_put(old_prog);
>>           if (netif_running(dev)) {
>> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device 
>> *dev, struct bpf_prog *prog,
>>         return 0;
>>   -err:
>> +err_old_prog:
>> +    virtnet_clear_guest_offloads(vi);
>> +    for (i = 0; i < vi->max_queue_pairs; i++)
>> +        rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
>> +err_new_prog:
>>       if (netif_running(dev)) {
>>           for (i = 0; i < vi->max_queue_pairs; i++) {
>>               virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Toshiaki Makita]

On 2019/01/17 22:05, Jason Wang wrote:
> On 2019/1/17 下午8:53, Jason Wang wrote:
>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
>>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>>> disabled, but was not complete.
>>>
>>> There was a small window which causes out of bounds sq access in
>>> virtnet_xdp_xmit() while disabling XDP.
>>>
>>> An example case of
>>>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>>   - online_cpu_num = xdp_queue_paris = 4
>>> when XDP is enabled:
>>>
>>> CPU 0                         CPU 1
>>> (Disabling XDP)               (Processing redirected XDP frames)
>>>
>>>                                virtnet_xdp_xmit()
>>> virtnet_xdp_set()
>>>   _virtnet_set_queues()
>>>    set curr_queue_pairs (2)
>>>                                 check if rq->xdp_prog is not NULL
>>>                                 virtnet_xdp_sq(vi)
>>>                                  qp = curr_queue_pairs -
>>>                                       xdp_queue_pairs +
>>>                                       smp_processor_id()
>>>                                     = 2 - 4 + 1 = -1
>>>                                  sq = &vi->sq[qp] // out of bounds
>>> access
>>>    set xdp_queue_pairs (0)
>>>    rq->xdp_prog = NULL
>>>
>>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>>> while someone can read the values. Thus, when disabling XDP, assign NULL
>>> to rq->xdp_prog first, and wait for RCU grace period, then change
>>> xxx_queue_pairs.
>>> Note that we need to keep the current order when enabling XDP though.
>>>
>>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>
>>
>> I wonder whether or not we could simply do:
>>
>>
>> if (prog) {
> 
> 
> Should be !prog
> 
> 
>>
>>     rcu_assign_pointer()
>>
>>     synchronize_net()
>>
>> }
>>
>> set queues
>>
>> if (!prog) {
> 
> 
> Should be prog.

Either would work.

With your suggestion the code will look like:

---
if (!prog) {
	for (...) {
		rcu_assign_pointer();
		...
	}
	synchronize_net();
}

virtnet_set_queues();
netif_set_real_num_rx_queues();
vi->xdp_queue_pairs = xdp_qp;

if (prog) {
	for (...) {
		rcu_assign_pointer();
		...
	}
}
---

But strictly speaking, virtnet_set_queues() should not be necessary if
(prog != NULL && old_prog != NULL).
If you prefer this, I can modify it accordingly.

-- 
Toshiaki Makita

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Jason Wang]

On 2019/1/18 上午9:56, Toshiaki Makita wrote:
> On 2019/01/17 22:05, Jason Wang wrote:
>> On 2019/1/17 下午8:53, Jason Wang wrote:
>>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
>>>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>>>> disabled, but was not complete.
>>>>
>>>> There was a small window which causes out of bounds sq access in
>>>> virtnet_xdp_xmit() while disabling XDP.
>>>>
>>>> An example case of
>>>>    - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>>>    - online_cpu_num = xdp_queue_paris = 4
>>>> when XDP is enabled:
>>>>
>>>> CPU 0                         CPU 1
>>>> (Disabling XDP)               (Processing redirected XDP frames)
>>>>
>>>>                                 virtnet_xdp_xmit()
>>>> virtnet_xdp_set()
>>>>    _virtnet_set_queues()
>>>>     set curr_queue_pairs (2)
>>>>                                  check if rq->xdp_prog is not NULL
>>>>                                  virtnet_xdp_sq(vi)
>>>>                                   qp = curr_queue_pairs -
>>>>                                        xdp_queue_pairs +
>>>>                                        smp_processor_id()
>>>>                                      = 2 - 4 + 1 = -1
>>>>                                   sq = &vi->sq[qp] // out of bounds
>>>> access
>>>>     set xdp_queue_pairs (0)
>>>>     rq->xdp_prog = NULL
>>>>
>>>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>>>> while someone can read the values. Thus, when disabling XDP, assign NULL
>>>> to rq->xdp_prog first, and wait for RCU grace period, then change
>>>> xxx_queue_pairs.
>>>> Note that we need to keep the current order when enabling XDP though.
>>>>
>>>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>
>>> I wonder whether or not we could simply do:
>>>
>>>
>>> if (prog) {
>>
>> Should be !prog
>>
>>
>>>      rcu_assign_pointer()
>>>
>>>      synchronize_net()
>>>
>>> }
>>>
>>> set queues
>>>
>>> if (!prog) {
>>
>> Should be prog.
> Either would work.
>
> With your suggestion the code will look like:
>
> ---
> if (!prog) {
> 	for (...) {
> 		rcu_assign_pointer();
> 		...
> 	}
> 	synchronize_net();
> }
>
> virtnet_set_queues();
> netif_set_real_num_rx_queues();
> vi->xdp_queue_pairs = xdp_qp;
>
> if (prog) {
> 	for (...) {
> 		rcu_assign_pointer();
> 		...
> 	}
> }


Yes, I think this makes code more easier to be understand.


> ---
>
> But strictly speaking, virtnet_set_queues() should not be necessary if
> (prog != NULL && old_prog != NULL).


Yes, but it was another possible 'issue'.


> If you prefer this, I can modify it accordingly.


I prefer to do this change.

Thanks

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Toshiaki Makita]

On 2019/01/18 12:52, Jason Wang wrote:
> On 2019/1/18 上午9:56, Toshiaki Makita wrote:
>> On 2019/01/17 22:05, Jason Wang wrote:
>>> On 2019/1/17 下午8:53, Jason Wang wrote:
>>>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>>>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards
>>>>> dev not
>>>>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>>>>> disabled, but was not complete.
>>>>>
>>>>> There was a small window which causes out of bounds sq access in
>>>>> virtnet_xdp_xmit() while disabling XDP.
>>>>>
>>>>> An example case of
>>>>>    - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>>>>    - online_cpu_num = xdp_queue_paris = 4
>>>>> when XDP is enabled:
>>>>>
>>>>> CPU 0                         CPU 1
>>>>> (Disabling XDP)               (Processing redirected XDP frames)
>>>>>
>>>>>                                 virtnet_xdp_xmit()
>>>>> virtnet_xdp_set()
>>>>>    _virtnet_set_queues()
>>>>>     set curr_queue_pairs (2)
>>>>>                                  check if rq->xdp_prog is not NULL
>>>>>                                  virtnet_xdp_sq(vi)
>>>>>                                   qp = curr_queue_pairs -
>>>>>                                        xdp_queue_pairs +
>>>>>                                        smp_processor_id()
>>>>>                                      = 2 - 4 + 1 = -1
>>>>>                                   sq = &vi->sq[qp] // out of bounds
>>>>> access
>>>>>     set xdp_queue_pairs (0)
>>>>>     rq->xdp_prog = NULL
>>>>>
>>>>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>>>>> while someone can read the values. Thus, when disabling XDP, assign
>>>>> NULL
>>>>> to rq->xdp_prog first, and wait for RCU grace period, then change
>>>>> xxx_queue_pairs.
>>>>> Note that we need to keep the current order when enabling XDP though.
>>>>>
>>>>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>>>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>>
>>>> I wonder whether or not we could simply do:
>>>>
>>>>
>>>> if (prog) {
>>>
>>> Should be !prog
>>>
>>>
>>>>      rcu_assign_pointer()
>>>>
>>>>      synchronize_net()
>>>>
>>>> }
>>>>
>>>> set queues
>>>>
>>>> if (!prog) {
>>>
>>> Should be prog.
>> Either would work.
>>
>> With your suggestion the code will look like:
>>
>> ---
>> if (!prog) {
>>     for (...) {
>>         rcu_assign_pointer();
>>         ...
>>     }
>>     synchronize_net();
>> }
>>
>> virtnet_set_queues();
>> netif_set_real_num_rx_queues();
>> vi->xdp_queue_pairs = xdp_qp;
>>
>> if (prog) {
>>     for (...) {
>>         rcu_assign_pointer();
>>         ...
>>     }
>> }
> 
> 
> Yes, I think this makes code more easier to be understand.
> 
> 
>> ---
>>
>> But strictly speaking, virtnet_set_queues() should not be necessary if
>> (prog != NULL && old_prog != NULL).
> 
> 
> Yes, but it was another possible 'issue'.
> 
> 
>> If you prefer this, I can modify it accordingly.
> 
> 
> I prefer to do this change.

OK, will do in v2.

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Toshiaki Makita]

On 2019/01/18 12:52, Jason Wang wrote:
> On 2019/1/18 上午9:56, Toshiaki Makita wrote:
>> On 2019/01/17 22:05, Jason Wang wrote:
>>> On 2019/1/17 下午8:53, Jason Wang wrote:
>>>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>>>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards
>>>>> dev not
>>>>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>>>>> disabled, but was not complete.
>>>>>
>>>>> There was a small window which causes out of bounds sq access in
>>>>> virtnet_xdp_xmit() while disabling XDP.
>>>>>
>>>>> An example case of
>>>>>    - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>>>>    - online_cpu_num = xdp_queue_paris = 4
>>>>> when XDP is enabled:
>>>>>
>>>>> CPU 0                         CPU 1
>>>>> (Disabling XDP)               (Processing redirected XDP frames)
>>>>>
>>>>>                                 virtnet_xdp_xmit()
>>>>> virtnet_xdp_set()
>>>>>    _virtnet_set_queues()
>>>>>     set curr_queue_pairs (2)
>>>>>                                  check if rq->xdp_prog is not NULL
>>>>>                                  virtnet_xdp_sq(vi)
>>>>>                                   qp = curr_queue_pairs -
>>>>>                                        xdp_queue_pairs +
>>>>>                                        smp_processor_id()
>>>>>                                      = 2 - 4 + 1 = -1
>>>>>                                   sq = &vi->sq[qp] // out of bounds
>>>>> access
>>>>>     set xdp_queue_pairs (0)
>>>>>     rq->xdp_prog = NULL
>>>>>
>>>>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>>>>> while someone can read the values. Thus, when disabling XDP, assign
>>>>> NULL
>>>>> to rq->xdp_prog first, and wait for RCU grace period, then change
>>>>> xxx_queue_pairs.
>>>>> Note that we need to keep the current order when enabling XDP though.
>>>>>
>>>>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>>>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>>
>>>> I wonder whether or not we could simply do:
>>>>
>>>>
>>>> if (prog) {
>>>
>>> Should be !prog
>>>
>>>
>>>>      rcu_assign_pointer()
>>>>
>>>>      synchronize_net()
>>>>
>>>> }
>>>>
>>>> set queues
>>>>
>>>> if (!prog) {
>>>
>>> Should be prog.
>> Either would work.
>>
>> With your suggestion the code will look like:
>>
>> ---
>> if (!prog) {
>>     for (...) {
>>         rcu_assign_pointer();
>>         ...
>>     }
>>     synchronize_net();
>> }
>>
>> virtnet_set_queues();
>> netif_set_real_num_rx_queues();
>> vi->xdp_queue_pairs = xdp_qp;
>>
>> if (prog) {
>>     for (...) {
>>         rcu_assign_pointer();
>>         ...
>>     }
>> }
> 
> 
> Yes, I think this makes code more easier to be understand.
> 
> 
>> ---
>>
>> But strictly speaking, virtnet_set_queues() should not be necessary if
>> (prog != NULL && old_prog != NULL).
> 
> 
> Yes, but it was another possible 'issue'.
> 
> 
>> If you prefer this, I can modify it accordingly.
> 
> 
> I prefer to do this change.

OK, will do in v2.

-- 
Toshiaki Makita

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Jason Wang]

On 2019/1/18 上午9:56, Toshiaki Makita wrote:
> On 2019/01/17 22:05, Jason Wang wrote:
>> On 2019/1/17 下午8:53, Jason Wang wrote:
>>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
>>>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>>>> disabled, but was not complete.
>>>>
>>>> There was a small window which causes out of bounds sq access in
>>>> virtnet_xdp_xmit() while disabling XDP.
>>>>
>>>> An example case of
>>>>    - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>>>    - online_cpu_num = xdp_queue_paris = 4
>>>> when XDP is enabled:
>>>>
>>>> CPU 0                         CPU 1
>>>> (Disabling XDP)               (Processing redirected XDP frames)
>>>>
>>>>                                 virtnet_xdp_xmit()
>>>> virtnet_xdp_set()
>>>>    _virtnet_set_queues()
>>>>     set curr_queue_pairs (2)
>>>>                                  check if rq->xdp_prog is not NULL
>>>>                                  virtnet_xdp_sq(vi)
>>>>                                   qp = curr_queue_pairs -
>>>>                                        xdp_queue_pairs +
>>>>                                        smp_processor_id()
>>>>                                      = 2 - 4 + 1 = -1
>>>>                                   sq = &vi->sq[qp] // out of bounds
>>>> access
>>>>     set xdp_queue_pairs (0)
>>>>     rq->xdp_prog = NULL
>>>>
>>>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>>>> while someone can read the values. Thus, when disabling XDP, assign NULL
>>>> to rq->xdp_prog first, and wait for RCU grace period, then change
>>>> xxx_queue_pairs.
>>>> Note that we need to keep the current order when enabling XDP though.
>>>>
>>>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>
>>> I wonder whether or not we could simply do:
>>>
>>>
>>> if (prog) {
>>
>> Should be !prog
>>
>>
>>>      rcu_assign_pointer()
>>>
>>>      synchronize_net()
>>>
>>> }
>>>
>>> set queues
>>>
>>> if (!prog) {
>>
>> Should be prog.
> Either would work.
>
> With your suggestion the code will look like:
>
> ---
> if (!prog) {
> 	for (...) {
> 		rcu_assign_pointer();
> 		...
> 	}
> 	synchronize_net();
> }
>
> virtnet_set_queues();
> netif_set_real_num_rx_queues();
> vi->xdp_queue_pairs = xdp_qp;
>
> if (prog) {
> 	for (...) {
> 		rcu_assign_pointer();
> 		...
> 	}
> }


Yes, I think this makes code more easier to be understand.


> ---
>
> But strictly speaking, virtnet_set_queues() should not be necessary if
> (prog != NULL && old_prog != NULL).


Yes, but it was another possible 'issue'.


> If you prefer this, I can modify it accordingly.


I prefer to do this change.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Toshiaki Makita]

On 2019/01/17 22:05, Jason Wang wrote:
> On 2019/1/17 下午8:53, Jason Wang wrote:
>> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
>>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>>> disabled, but was not complete.
>>>
>>> There was a small window which causes out of bounds sq access in
>>> virtnet_xdp_xmit() while disabling XDP.
>>>
>>> An example case of
>>>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>>   - online_cpu_num = xdp_queue_paris = 4
>>> when XDP is enabled:
>>>
>>> CPU 0                         CPU 1
>>> (Disabling XDP)               (Processing redirected XDP frames)
>>>
>>>                                virtnet_xdp_xmit()
>>> virtnet_xdp_set()
>>>   _virtnet_set_queues()
>>>    set curr_queue_pairs (2)
>>>                                 check if rq->xdp_prog is not NULL
>>>                                 virtnet_xdp_sq(vi)
>>>                                  qp = curr_queue_pairs -
>>>                                       xdp_queue_pairs +
>>>                                       smp_processor_id()
>>>                                     = 2 - 4 + 1 = -1
>>>                                  sq = &vi->sq[qp] // out of bounds
>>> access
>>>    set xdp_queue_pairs (0)
>>>    rq->xdp_prog = NULL
>>>
>>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>>> while someone can read the values. Thus, when disabling XDP, assign NULL
>>> to rq->xdp_prog first, and wait for RCU grace period, then change
>>> xxx_queue_pairs.
>>> Note that we need to keep the current order when enabling XDP though.
>>>
>>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>
>>
>> I wonder whether or not we could simply do:
>>
>>
>> if (prog) {
> 
> 
> Should be !prog
> 
> 
>>
>>     rcu_assign_pointer()
>>
>>     synchronize_net()
>>
>> }
>>
>> set queues
>>
>> if (!prog) {
> 
> 
> Should be prog.

Either would work.

With your suggestion the code will look like:

---
if (!prog) {
	for (...) {
		rcu_assign_pointer();
		...
	}
	synchronize_net();
}

virtnet_set_queues();
netif_set_real_num_rx_queues();
vi->xdp_queue_pairs = xdp_qp;

if (prog) {
	for (...) {
		rcu_assign_pointer();
		...
	}
}
---

But strictly speaking, virtnet_set_queues() should not be necessary if
(prog != NULL && old_prog != NULL).
If you prefer this, I can modify it accordingly.

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled

[Jason Wang]

On 2019/1/17 下午8:53, Jason Wang wrote:
>
> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>> disabled, but was not complete.
>>
>> There was a small window which causes out of bounds sq access in
>> virtnet_xdp_xmit() while disabling XDP.
>>
>> An example case of
>>   - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>>   - online_cpu_num = xdp_queue_paris = 4
>> when XDP is enabled:
>>
>> CPU 0                         CPU 1
>> (Disabling XDP)               (Processing redirected XDP frames)
>>
>>                                virtnet_xdp_xmit()
>> virtnet_xdp_set()
>>   _virtnet_set_queues()
>>    set curr_queue_pairs (2)
>>                                 check if rq->xdp_prog is not NULL
>>                                 virtnet_xdp_sq(vi)
>>                                  qp = curr_queue_pairs -
>>                                       xdp_queue_pairs +
>>                                       smp_processor_id()
>>                                     = 2 - 4 + 1 = -1
>>                                  sq = &vi->sq[qp] // out of bounds 
>> access
>>    set xdp_queue_pairs (0)
>>    rq->xdp_prog = NULL
>>
>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>> while someone can read the values. Thus, when disabling XDP, assign NULL
>> to rq->xdp_prog first, and wait for RCU grace period, then change
>> xxx_queue_pairs.
>> Note that we need to keep the current order when enabling XDP though.
>>
>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>
>
> I wonder whether or not we could simply do:
>
>
> if (prog) {


Should be !prog


>
>     rcu_assign_pointer()
>
>     synchronize_net()
>
> }
>
> set queues
>
> if (!prog) {


Should be prog.

Thanks


>
>     rcu_assign_pointer()
>
> }
>
> Thanks
>
>
>> ---
>>   drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 204eedf..ae93f0e 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device 
>> *dev, struct bpf_prog *prog,
>>           }
>>       }
>>   -    err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> -    if (err)
>> -        goto err;
>> -    netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> -    vi->xdp_queue_pairs = xdp_qp;
>> +    old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
>> +    if (!old_prog && prog) {
>> +        err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> +        if (err)
>> +            goto err_new_prog;
>> +        netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> +        vi->xdp_queue_pairs = xdp_qp;
>> +    }
>>         for (i = 0; i < vi->max_queue_pairs; i++) {
>> -        old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>>           rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>>           if (i == 0) {
>>               if (!old_prog)
>> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device 
>> *dev, struct bpf_prog *prog,
>>               if (!prog)
>>                   virtnet_restore_guest_offloads(vi);
>>           }
>> +    }
>> +
>> +    if (old_prog && !prog) {
>> +        synchronize_net();
>> +        err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> +        if (err)
>> +            goto err_old_prog;
>> +        netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> +        vi->xdp_queue_pairs = xdp_qp;
>> +    }
>> +
>> +    for (i = 0; i < vi->max_queue_pairs; i++) {
>>           if (old_prog)
>>               bpf_prog_put(old_prog);
>>           if (netif_running(dev)) {
>> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device 
>> *dev, struct bpf_prog *prog,
>>         return 0;
>>   -err:
>> +err_old_prog:
>> +    virtnet_clear_guest_offloads(vi);
>> +    for (i = 0; i < vi->max_queue_pairs; i++)
>> +        rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
>> +err_new_prog:
>>       if (netif_running(dev)) {
>>           for (i = 0; i < vi->max_queue_pairs; i++) {
>>               virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs

[Toshiaki Makita]

put_page() can work as a fallback for freeing xdp_frames, but the
appropriate way is to use xdp_return_frame().

Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ae93f0e..996de69 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
 			if (!is_xdp_raw_buffer_queue(vi, i))
 				dev_kfree_skb(buf);
 			else
-				put_page(virt_to_head_page(buf));
+				xdp_return_frame(buf);
 		}
 	}
 
-- 
1.8.3.1

Re: [PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> put_page() can work as a fallback for freeing xdp_frames, but the
> appropriate way is to use xdp_return_frame().
>
> Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ae93f0e..996de69 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
>   			if (!is_xdp_raw_buffer_queue(vi, i))
>   				dev_kfree_skb(buf);
>   			else
> -				put_page(virt_to_head_page(buf));
> +				xdp_return_frame(buf);
>   		}
>   	}
>   


Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs

[Jesper Dangaard Brouer]

On Thu, 17 Jan 2019 20:56:39 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> > put_page() can work as a fallback for freeing xdp_frames, but the
> > appropriate way is to use xdp_return_frame().
> >
> > Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> > ---
> >   drivers/net/virtio_net.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index ae93f0e..996de69 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
> >   			if (!is_xdp_raw_buffer_queue(vi, i))
> >   				dev_kfree_skb(buf);
> >   			else
> > -				put_page(virt_to_head_page(buf));
> > +				xdp_return_frame(buf);

I assume the is_xdp_raw_buffer_queue() test assures that this void *buf
is a struct xdp_frame.

> >   		}
> >   	}
> >     
> 
> 
> Acked-by: Jason Wang <jasowang@redhat.com>

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs

[Jesper Dangaard Brouer]

On Thu, 17 Jan 2019 20:56:39 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> > put_page() can work as a fallback for freeing xdp_frames, but the
> > appropriate way is to use xdp_return_frame().
> >
> > Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> > ---
> >   drivers/net/virtio_net.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index ae93f0e..996de69 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
> >   			if (!is_xdp_raw_buffer_queue(vi, i))
> >   				dev_kfree_skb(buf);
> >   			else
> > -				put_page(virt_to_head_page(buf));
> > +				xdp_return_frame(buf);

I assume the is_xdp_raw_buffer_queue() test assures that this void *buf
is a struct xdp_frame.

> >   		}
> >   	}
> >     
> 
> 
> Acked-by: Jason Wang <jasowang@redhat.com>

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs

[Jason Wang]

On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> put_page() can work as a fallback for freeing xdp_frames, but the
> appropriate way is to use xdp_return_frame().
>
> Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ae93f0e..996de69 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
>   			if (!is_xdp_raw_buffer_queue(vi, i))
>   				dev_kfree_skb(buf);
>   			else
> -				put_page(virt_to_head_page(buf));
> +				xdp_return_frame(buf);
>   		}
>   	}
>   


Acked-by: Jason Wang <jasowang@redhat.com>


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs

[Toshiaki Makita]

put_page() can work as a fallback for freeing xdp_frames, but the
appropriate way is to use xdp_return_frame().

Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ae93f0e..996de69 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
 			if (!is_xdp_raw_buffer_queue(vi, i))
 				dev_kfree_skb(buf);
 			else
-				put_page(virt_to_head_page(buf));
+				xdp_return_frame(buf);
 		}
 	}
 
-- 
1.8.3.1

[PATCH net 7/7] virtio_net: Differentiate sk_buff and xdp_frame on freeing

[Toshiaki Makita]

We do not reset or free up unused buffers when enabling/disabling XDP,
so it can happen that xdp_frames are freed after disabling XDP or
sk_buffs are freed after enabling XDP on xdp tx queues.
Thus we need to handle both forms (xdp_frames and sk_buffs) regardless
of XDP setting.
One way to trigger this problem is to disable XDP when napi_tx is
enabled. In that case, virtnet_xdp_set() calls virtnet_napi_enable()
which kicks NAPI. The NAPI handler will call virtnet_poll_cleantx()
which invokes free_old_xmit_skbs() for queues which have been used by
XDP.

Note that even with this change we need to keep skipping
free_old_xmit_skbs() from NAPI handlers when XDP is enabled, because XDP
tx queues do not aquire queue locks.

Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 54 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 996de69..6598c25 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -57,6 +57,8 @@
 #define VIRTIO_XDP_TX		BIT(0)
 #define VIRTIO_XDP_REDIR	BIT(1)
 
+#define VIRTIO_XDP_FLAG	BIT(0)
+
 /* RX packet size EWMA. The average packet size is used to determine the packet
  * buffer size when refilling RX rings. As the entire RX ring may be refilled
  * at once, the weight is chosen so that the EWMA will be insensitive to short-
@@ -252,6 +254,21 @@ struct padded_vnet_hdr {
 	char padding[4];
 };
 
+static bool is_xdp_frame(void *ptr)
+{
+	return (unsigned long)ptr & VIRTIO_XDP_FLAG;
+}
+
+static void *xdp_to_ptr(struct xdp_frame *ptr)
+{
+	return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
+}
+
+static struct xdp_frame *ptr_to_xdp(void *ptr)
+{
+	return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
+}
+
 /* Converting between virtqueue no. and kernel tx/rx queue no.
  * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
  */
@@ -462,7 +479,8 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
 
 	sg_init_one(sq->sg, xdpf->data, xdpf->len);
 
-	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
+	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
+				   GFP_ATOMIC);
 	if (unlikely(err))
 		return -ENOSPC; /* Caller handle free/refcnt */
 
@@ -482,13 +500,13 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct receive_queue *rq = vi->rq;
-	struct xdp_frame *xdpf_sent;
 	struct bpf_prog *xdp_prog;
 	struct send_queue *sq;
 	unsigned int len;
 	int drops = 0;
 	int kicks = 0;
 	int ret, err;
+	void *ptr;
 	int i;
 
 	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
@@ -507,8 +525,12 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	}
 
 	/* Free up any pending old buffers before queueing new ones. */
-	while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
-		xdp_return_frame(xdpf_sent);
+	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+		if (likely(is_xdp_frame(ptr)))
+			xdp_return_frame(ptr_to_xdp(ptr));
+		else
+			dev_consume_skb_any(ptr);
+	}
 
 	for (i = 0; i < n; i++) {
 		struct xdp_frame *xdpf = frames[i];
@@ -1329,18 +1351,26 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 
 static void free_old_xmit_skbs(struct send_queue *sq)
 {
-	struct sk_buff *skb;
 	unsigned int len;
 	unsigned int packets = 0;
 	unsigned int bytes = 0;
+	void *ptr;
 
-	while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
-		pr_debug("Sent skb %p\n", skb);
+	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+		if (likely(!is_xdp_frame(ptr))) {
+			struct sk_buff *skb = ptr;
 
-		bytes += skb->len;
-		packets++;
+			pr_debug("Sent skb %p\n", skb);
 
-		dev_consume_skb_any(skb);
+			bytes += skb->len;
+			dev_consume_skb_any(skb);
+		} else {
+			struct xdp_frame *frame = ptr_to_xdp(ptr);
+
+			bytes += frame->len;
+			xdp_return_frame(frame);
+		}
+		packets++;
 	}
 
 	/* Avoid overhead when no packets have been processed
@@ -2665,10 +2695,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		struct virtqueue *vq = vi->sq[i].vq;
 		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
-			if (!is_xdp_raw_buffer_queue(vi, i))
+			if (!is_xdp_frame(buf))
 				dev_kfree_skb(buf);
 			else
-				xdp_return_frame(buf);
+				xdp_return_frame(ptr_to_xdp(buf));
 		}
 	}
 
-- 
1.8.3.1

Re: [PATCH net 7/7] virtio_net: Differentiate sk_buff and xdp_frame on freeing

[Jason Wang]

On 2019/1/17 7:20, Toshiaki Makita wrote:
> We do not reset or free up unused buffers when enabling/disabling XDP,
> so it can happen that xdp_frames are freed after disabling XDP or
> sk_buffs are freed after enabling XDP on xdp tx queues.
> Thus we need to handle both forms (xdp_frames and sk_buffs) regardless
> of XDP setting.
> One way to trigger this problem is to disable XDP when napi_tx is
> enabled. In that case, virtnet_xdp_set() calls virtnet_napi_enable()
> which kicks NAPI. The NAPI handler will call virtnet_poll_cleantx()
> which invokes free_old_xmit_skbs() for queues which have been used by
> XDP.
>
> Note that even with this change we need to keep skipping
> free_old_xmit_skbs() from NAPI handlers when XDP is enabled, because XDP
> tx queues do not aquire queue locks.
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 54 +++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 996de69..6598c25 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -57,6 +57,8 @@
>   #define VIRTIO_XDP_TX		BIT(0)
>   #define VIRTIO_XDP_REDIR	BIT(1)
>   
> +#define VIRTIO_XDP_FLAG	BIT(0)
> +
>   /* RX packet size EWMA. The average packet size is used to determine the packet
>    * buffer size when refilling RX rings. As the entire RX ring may be refilled
>    * at once, the weight is chosen so that the EWMA will be insensitive to short-
> @@ -252,6 +254,21 @@ struct padded_vnet_hdr {
>   	char padding[4];
>   };
>   
> +static bool is_xdp_frame(void *ptr)
> +{
> +	return (unsigned long)ptr & VIRTIO_XDP_FLAG;
> +}
> +
> +static void *xdp_to_ptr(struct xdp_frame *ptr)
> +{
> +	return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
> +}
> +
> +static struct xdp_frame *ptr_to_xdp(void *ptr)
> +{
> +	return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
> +}
> +
>   /* Converting between virtqueue no. and kernel tx/rx queue no.
>    * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
>    */
> @@ -462,7 +479,8 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>   
>   	sg_init_one(sq->sg, xdpf->data, xdpf->len);
>   
> -	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
> +	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
> +				   GFP_ATOMIC);
>   	if (unlikely(err))
>   		return -ENOSPC; /* Caller handle free/refcnt */
>   
> @@ -482,13 +500,13 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	struct receive_queue *rq = vi->rq;
> -	struct xdp_frame *xdpf_sent;
>   	struct bpf_prog *xdp_prog;
>   	struct send_queue *sq;
>   	unsigned int len;
>   	int drops = 0;
>   	int kicks = 0;
>   	int ret, err;
> +	void *ptr;
>   	int i;
>   
>   	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
> @@ -507,8 +525,12 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>   	}
>   
>   	/* Free up any pending old buffers before queueing new ones. */
> -	while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
> -		xdp_return_frame(xdpf_sent);
> +	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> +		if (likely(is_xdp_frame(ptr)))
> +			xdp_return_frame(ptr_to_xdp(ptr));
> +		else
> +			dev_consume_skb_any(ptr);
> +	}
>   
>   	for (i = 0; i < n; i++) {
>   		struct xdp_frame *xdpf = frames[i];
> @@ -1329,18 +1351,26 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>   
>   static void free_old_xmit_skbs(struct send_queue *sq)
>   {
> -	struct sk_buff *skb;
>   	unsigned int len;
>   	unsigned int packets = 0;
>   	unsigned int bytes = 0;
> +	void *ptr;
>   
> -	while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> -		pr_debug("Sent skb %p\n", skb);
> +	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> +		if (likely(!is_xdp_frame(ptr))) {
> +			struct sk_buff *skb = ptr;
>   
> -		bytes += skb->len;
> -		packets++;
> +			pr_debug("Sent skb %p\n", skb);
>   
> -		dev_consume_skb_any(skb);
> +			bytes += skb->len;
> +			dev_consume_skb_any(skb);
> +		} else {
> +			struct xdp_frame *frame = ptr_to_xdp(ptr);
> +
> +			bytes += frame->len;
> +			xdp_return_frame(frame);
> +		}
> +		packets++;
>   	}
>   
>   	/* Avoid overhead when no packets have been processed
> @@ -2665,10 +2695,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		struct virtqueue *vq = vi->sq[i].vq;
>   		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
> -			if (!is_xdp_raw_buffer_queue(vi, i))
> +			if (!is_xdp_frame(buf))
>   				dev_kfree_skb(buf);
>   			else
> -				xdp_return_frame(buf);
> +				xdp_return_frame(ptr_to_xdp(buf));
>   		}
>   	}
>   


Acked-by: Jason Wang <jasowang@redhat.com>

Thanks!

Re: [PATCH net 7/7] virtio_net: Differentiate sk_buff and xdp_frame on freeing

[Jason Wang]

On 2019/1/17 7:20, Toshiaki Makita wrote:
> We do not reset or free up unused buffers when enabling/disabling XDP,
> so it can happen that xdp_frames are freed after disabling XDP or
> sk_buffs are freed after enabling XDP on xdp tx queues.
> Thus we need to handle both forms (xdp_frames and sk_buffs) regardless
> of XDP setting.
> One way to trigger this problem is to disable XDP when napi_tx is
> enabled. In that case, virtnet_xdp_set() calls virtnet_napi_enable()
> which kicks NAPI. The NAPI handler will call virtnet_poll_cleantx()
> which invokes free_old_xmit_skbs() for queues which have been used by
> XDP.
>
> Note that even with this change we need to keep skipping
> free_old_xmit_skbs() from NAPI handlers when XDP is enabled, because XDP
> tx queues do not aquire queue locks.
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>   drivers/net/virtio_net.c | 54 +++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 996de69..6598c25 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -57,6 +57,8 @@
>   #define VIRTIO_XDP_TX		BIT(0)
>   #define VIRTIO_XDP_REDIR	BIT(1)
>   
> +#define VIRTIO_XDP_FLAG	BIT(0)
> +
>   /* RX packet size EWMA. The average packet size is used to determine the packet
>    * buffer size when refilling RX rings. As the entire RX ring may be refilled
>    * at once, the weight is chosen so that the EWMA will be insensitive to short-
> @@ -252,6 +254,21 @@ struct padded_vnet_hdr {
>   	char padding[4];
>   };
>   
> +static bool is_xdp_frame(void *ptr)
> +{
> +	return (unsigned long)ptr & VIRTIO_XDP_FLAG;
> +}
> +
> +static void *xdp_to_ptr(struct xdp_frame *ptr)
> +{
> +	return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
> +}
> +
> +static struct xdp_frame *ptr_to_xdp(void *ptr)
> +{
> +	return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
> +}
> +
>   /* Converting between virtqueue no. and kernel tx/rx queue no.
>    * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
>    */
> @@ -462,7 +479,8 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>   
>   	sg_init_one(sq->sg, xdpf->data, xdpf->len);
>   
> -	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
> +	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
> +				   GFP_ATOMIC);
>   	if (unlikely(err))
>   		return -ENOSPC; /* Caller handle free/refcnt */
>   
> @@ -482,13 +500,13 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	struct receive_queue *rq = vi->rq;
> -	struct xdp_frame *xdpf_sent;
>   	struct bpf_prog *xdp_prog;
>   	struct send_queue *sq;
>   	unsigned int len;
>   	int drops = 0;
>   	int kicks = 0;
>   	int ret, err;
> +	void *ptr;
>   	int i;
>   
>   	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
> @@ -507,8 +525,12 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>   	}
>   
>   	/* Free up any pending old buffers before queueing new ones. */
> -	while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
> -		xdp_return_frame(xdpf_sent);
> +	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> +		if (likely(is_xdp_frame(ptr)))
> +			xdp_return_frame(ptr_to_xdp(ptr));
> +		else
> +			dev_consume_skb_any(ptr);
> +	}
>   
>   	for (i = 0; i < n; i++) {
>   		struct xdp_frame *xdpf = frames[i];
> @@ -1329,18 +1351,26 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>   
>   static void free_old_xmit_skbs(struct send_queue *sq)
>   {
> -	struct sk_buff *skb;
>   	unsigned int len;
>   	unsigned int packets = 0;
>   	unsigned int bytes = 0;
> +	void *ptr;
>   
> -	while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> -		pr_debug("Sent skb %p\n", skb);
> +	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> +		if (likely(!is_xdp_frame(ptr))) {
> +			struct sk_buff *skb = ptr;
>   
> -		bytes += skb->len;
> -		packets++;
> +			pr_debug("Sent skb %p\n", skb);
>   
> -		dev_consume_skb_any(skb);
> +			bytes += skb->len;
> +			dev_consume_skb_any(skb);
> +		} else {
> +			struct xdp_frame *frame = ptr_to_xdp(ptr);
> +
> +			bytes += frame->len;
> +			xdp_return_frame(frame);
> +		}
> +		packets++;
>   	}
>   
>   	/* Avoid overhead when no packets have been processed
> @@ -2665,10 +2695,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		struct virtqueue *vq = vi->sq[i].vq;
>   		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
> -			if (!is_xdp_raw_buffer_queue(vi, i))
> +			if (!is_xdp_frame(buf))
>   				dev_kfree_skb(buf);
>   			else
> -				xdp_return_frame(buf);
> +				xdp_return_frame(ptr_to_xdp(buf));
>   		}
>   	}
>   


Acked-by: Jason Wang <jasowang@redhat.com>

Thanks!

[PATCH net 7/7] virtio_net: Differentiate sk_buff and xdp_frame on freeing

[Toshiaki Makita]

We do not reset or free up unused buffers when enabling/disabling XDP,
so it can happen that xdp_frames are freed after disabling XDP or
sk_buffs are freed after enabling XDP on xdp tx queues.
Thus we need to handle both forms (xdp_frames and sk_buffs) regardless
of XDP setting.
One way to trigger this problem is to disable XDP when napi_tx is
enabled. In that case, virtnet_xdp_set() calls virtnet_napi_enable()
which kicks NAPI. The NAPI handler will call virtnet_poll_cleantx()
which invokes free_old_xmit_skbs() for queues which have been used by
XDP.

Note that even with this change we need to keep skipping
free_old_xmit_skbs() from NAPI handlers when XDP is enabled, because XDP
tx queues do not aquire queue locks.

Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 drivers/net/virtio_net.c | 54 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 996de69..6598c25 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -57,6 +57,8 @@
 #define VIRTIO_XDP_TX		BIT(0)
 #define VIRTIO_XDP_REDIR	BIT(1)
 
+#define VIRTIO_XDP_FLAG	BIT(0)
+
 /* RX packet size EWMA. The average packet size is used to determine the packet
  * buffer size when refilling RX rings. As the entire RX ring may be refilled
  * at once, the weight is chosen so that the EWMA will be insensitive to short-
@@ -252,6 +254,21 @@ struct padded_vnet_hdr {
 	char padding[4];
 };
 
+static bool is_xdp_frame(void *ptr)
+{
+	return (unsigned long)ptr & VIRTIO_XDP_FLAG;
+}
+
+static void *xdp_to_ptr(struct xdp_frame *ptr)
+{
+	return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
+}
+
+static struct xdp_frame *ptr_to_xdp(void *ptr)
+{
+	return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
+}
+
 /* Converting between virtqueue no. and kernel tx/rx queue no.
  * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
  */
@@ -462,7 +479,8 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
 
 	sg_init_one(sq->sg, xdpf->data, xdpf->len);
 
-	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
+	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
+				   GFP_ATOMIC);
 	if (unlikely(err))
 		return -ENOSPC; /* Caller handle free/refcnt */
 
@@ -482,13 +500,13 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct receive_queue *rq = vi->rq;
-	struct xdp_frame *xdpf_sent;
 	struct bpf_prog *xdp_prog;
 	struct send_queue *sq;
 	unsigned int len;
 	int drops = 0;
 	int kicks = 0;
 	int ret, err;
+	void *ptr;
 	int i;
 
 	/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
@@ -507,8 +525,12 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	}
 
 	/* Free up any pending old buffers before queueing new ones. */
-	while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
-		xdp_return_frame(xdpf_sent);
+	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+		if (likely(is_xdp_frame(ptr)))
+			xdp_return_frame(ptr_to_xdp(ptr));
+		else
+			dev_consume_skb_any(ptr);
+	}
 
 	for (i = 0; i < n; i++) {
 		struct xdp_frame *xdpf = frames[i];
@@ -1329,18 +1351,26 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 
 static void free_old_xmit_skbs(struct send_queue *sq)
 {
-	struct sk_buff *skb;
 	unsigned int len;
 	unsigned int packets = 0;
 	unsigned int bytes = 0;
+	void *ptr;
 
-	while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
-		pr_debug("Sent skb %p\n", skb);
+	while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+		if (likely(!is_xdp_frame(ptr))) {
+			struct sk_buff *skb = ptr;
 
-		bytes += skb->len;
-		packets++;
+			pr_debug("Sent skb %p\n", skb);
 
-		dev_consume_skb_any(skb);
+			bytes += skb->len;
+			dev_consume_skb_any(skb);
+		} else {
+			struct xdp_frame *frame = ptr_to_xdp(ptr);
+
+			bytes += frame->len;
+			xdp_return_frame(frame);
+		}
+		packets++;
 	}
 
 	/* Avoid overhead when no packets have been processed
@@ -2665,10 +2695,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		struct virtqueue *vq = vi->sq[i].vq;
 		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
-			if (!is_xdp_raw_buffer_queue(vi, i))
+			if (!is_xdp_frame(buf))
 				dev_kfree_skb(buf);
 			else
-				xdp_return_frame(buf);
+				xdp_return_frame(ptr_to_xdp(buf));
 		}
 	}
 
-- 
1.8.3.1

Re: [PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx

[Michael S. Tsirkin]

On Thu, Jan 17, 2019 at 08:20:38PM +0900, Toshiaki Makita wrote:
> While I'm looking into how to account standard tx counters on XDP tx
> processing, I found several bugs around XDP tx and napi_tx.
> 
> Patch1: Fix oops on error path. Patch2 depends on this.
> Patch2: Fix memory corruption on freeing xdp_frames with napi_tx enabled.
> Patch3: Minor fix patch5 depends on.
> Patch4: Fix memory corruption on processing xdp_frames when XDP is disabled.
>   Also patch5 depends on this.
> Patch5: Fix memory corruption on processing xdp_frames while XDP is being
>   disabled.
> Patch6: Minor fix patch7 depends on.
> Patch7: Fix memory corruption on freeing sk_buff or xdp_frames when a normal
>   queue is reused for XDP and vise versa.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Series:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

I guess we need this stuff on stable?
I'm especially happy with Patch7 as it makes my BQL
work a bit easier.

> Toshiaki Makita (7):
>   virtio_net: Don't enable NAPI when interface is down
>   virtio_net: Don't call free_old_xmit_skbs for xdp_frames
>   virtio_net: Fix not restoring real_num_rx_queues
>   virtio_net: Fix out of bounds access of sq
>   virtio_net: Don't process redirected XDP frames when XDP is disabled
>   virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
>   virtio_net: Differentiate sk_buff and xdp_frame on freeing
> 
>  drivers/net/virtio_net.c | 154 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 109 insertions(+), 45 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx

[Michael S. Tsirkin]

On Thu, Jan 17, 2019 at 08:20:38PM +0900, Toshiaki Makita wrote:
> While I'm looking into how to account standard tx counters on XDP tx
> processing, I found several bugs around XDP tx and napi_tx.
> 
> Patch1: Fix oops on error path. Patch2 depends on this.
> Patch2: Fix memory corruption on freeing xdp_frames with napi_tx enabled.
> Patch3: Minor fix patch5 depends on.
> Patch4: Fix memory corruption on processing xdp_frames when XDP is disabled.
>   Also patch5 depends on this.
> Patch5: Fix memory corruption on processing xdp_frames while XDP is being
>   disabled.
> Patch6: Minor fix patch7 depends on.
> Patch7: Fix memory corruption on freeing sk_buff or xdp_frames when a normal
>   queue is reused for XDP and vise versa.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Series:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

I guess we need this stuff on stable?
I'm especially happy with Patch7 as it makes my BQL
work a bit easier.

> Toshiaki Makita (7):
>   virtio_net: Don't enable NAPI when interface is down
>   virtio_net: Don't call free_old_xmit_skbs for xdp_frames
>   virtio_net: Fix not restoring real_num_rx_queues
>   virtio_net: Fix out of bounds access of sq
>   virtio_net: Don't process redirected XDP frames when XDP is disabled
>   virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
>   virtio_net: Differentiate sk_buff and xdp_frame on freeing
> 
>  drivers/net/virtio_net.c | 154 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 109 insertions(+), 45 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx

[Toshiaki Makita]

On 2019/01/17 23:55, Michael S. Tsirkin wrote:
> On Thu, Jan 17, 2019 at 08:20:38PM +0900, Toshiaki Makita wrote:
>> While I'm looking into how to account standard tx counters on XDP tx
>> processing, I found several bugs around XDP tx and napi_tx.
>>
>> Patch1: Fix oops on error path. Patch2 depends on this.
>> Patch2: Fix memory corruption on freeing xdp_frames with napi_tx enabled.
>> Patch3: Minor fix patch5 depends on.
>> Patch4: Fix memory corruption on processing xdp_frames when XDP is disabled.
>>   Also patch5 depends on this.
>> Patch5: Fix memory corruption on processing xdp_frames while XDP is being
>>   disabled.
>> Patch6: Minor fix patch7 depends on.
>> Patch7: Fix memory corruption on freeing sk_buff or xdp_frames when a normal
>>   queue is reused for XDP and vise versa.
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> 
> Series:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks for the review.

> 
> I guess we need this stuff on stable?

I think so.

> I'm especially happy with Patch7 as it makes my BQL
> work a bit easier.
> 
>> Toshiaki Makita (7):
>>   virtio_net: Don't enable NAPI when interface is down
>>   virtio_net: Don't call free_old_xmit_skbs for xdp_frames
>>   virtio_net: Fix not restoring real_num_rx_queues
>>   virtio_net: Fix out of bounds access of sq
>>   virtio_net: Don't process redirected XDP frames when XDP is disabled
>>   virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
>>   virtio_net: Differentiate sk_buff and xdp_frame on freeing
>>
>>  drivers/net/virtio_net.c | 154 +++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 109 insertions(+), 45 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
> 
> 

-- 
Toshiaki Makita

Re: [PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx

[Toshiaki Makita]

On 2019/01/17 23:55, Michael S. Tsirkin wrote:
> On Thu, Jan 17, 2019 at 08:20:38PM +0900, Toshiaki Makita wrote:
>> While I'm looking into how to account standard tx counters on XDP tx
>> processing, I found several bugs around XDP tx and napi_tx.
>>
>> Patch1: Fix oops on error path. Patch2 depends on this.
>> Patch2: Fix memory corruption on freeing xdp_frames with napi_tx enabled.
>> Patch3: Minor fix patch5 depends on.
>> Patch4: Fix memory corruption on processing xdp_frames when XDP is disabled.
>>   Also patch5 depends on this.
>> Patch5: Fix memory corruption on processing xdp_frames while XDP is being
>>   disabled.
>> Patch6: Minor fix patch7 depends on.
>> Patch7: Fix memory corruption on freeing sk_buff or xdp_frames when a normal
>>   queue is reused for XDP and vise versa.
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> 
> Series:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks for the review.

> 
> I guess we need this stuff on stable?

I think so.

> I'm especially happy with Patch7 as it makes my BQL
> work a bit easier.
> 
>> Toshiaki Makita (7):
>>   virtio_net: Don't enable NAPI when interface is down
>>   virtio_net: Don't call free_old_xmit_skbs for xdp_frames
>>   virtio_net: Fix not restoring real_num_rx_queues
>>   virtio_net: Fix out of bounds access of sq
>>   virtio_net: Don't process redirected XDP frames when XDP is disabled
>>   virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
>>   virtio_net: Differentiate sk_buff and xdp_frame on freeing
>>
>>  drivers/net/virtio_net.c | 154 +++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 109 insertions(+), 45 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
> 
> 

-- 
Toshiaki Makita