BPF Archive on lore.kernel.org
 help / Atom feed
* [PATCH net 0/3] XDP generic related fixes
@ 2019-05-16 21:54 Stephen Hemminger
  2019-05-16 21:54 ` [PATCH net 1/3] netvsc: unshare skb in VF rx handler Stephen Hemminger
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Stephen Hemminger @ 2019-05-16 21:54 UTC (permalink / raw)
  To: netdev, davem; +Cc: xdp-newbies, bpf, Stephen Hemminger

This set of patches came about while investigating XDP
generic on Azure. The split brain nature of the accelerated
networking exposed issues with the stack device model.

The real fix is in the second patch which is a redo
of earlier patch from Jason Wang.

Stephen Hemminger (3):
  netvsc: unshare skb in VF rx handler
  net: core: generic XDP support for stacked device
  netdevice: clarify meaning of rx_handler_result

 drivers/net/hyperv/netvsc_drv.c |  6 ++++++
 include/linux/netdevice.h       | 16 ++++++++--------
 net/core/dev.c                  | 10 ++++++++++
 3 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net 1/3] netvsc: unshare skb in VF rx handler
  2019-05-16 21:54 [PATCH net 0/3] XDP generic related fixes Stephen Hemminger
@ 2019-05-16 21:54 ` Stephen Hemminger
  2019-05-16 21:54 ` [PATCH net 2/3] net: core: generic XDP support for stacked device Stephen Hemminger
  2019-05-16 21:54 ` [PATCH net 3/3] netdevice: clarify meaning of rx_handler_result Stephen Hemminger
  2 siblings, 0 replies; 7+ messages in thread
From: Stephen Hemminger @ 2019-05-16 21:54 UTC (permalink / raw)
  To: netdev, davem; +Cc: xdp-newbies, bpf, Stephen Hemminger

The netvsc VF skb handler should make sure that skb is not
shared. Similar logic already exists in bonding and team device
drivers.

This is not an issue in practice because the VF devicex
does not send up shared skb's. But the netvsc driver
should do the right thing if it did.

Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 06393b215102..9873b8679f81 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2000,6 +2000,12 @@ static rx_handler_result_t netvsc_vf_handle_frame(struct sk_buff **pskb)
 	struct netvsc_vf_pcpu_stats *pcpu_stats
 		 = this_cpu_ptr(ndev_ctx->vf_stats);
 
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (unlikely(!skb))
+		return RX_HANDLER_CONSUMED;
+
+	*pskb = skb;
+
 	skb->dev = ndev;
 
 	u64_stats_update_begin(&pcpu_stats->syncp);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net 2/3] net: core: generic XDP support for stacked device
  2019-05-16 21:54 [PATCH net 0/3] XDP generic related fixes Stephen Hemminger
  2019-05-16 21:54 ` [PATCH net 1/3] netvsc: unshare skb in VF rx handler Stephen Hemminger
@ 2019-05-16 21:54 ` Stephen Hemminger
  2019-05-17  1:09   ` Jason Wang
  2019-05-16 21:54 ` [PATCH net 3/3] netdevice: clarify meaning of rx_handler_result Stephen Hemminger
  2 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2019-05-16 21:54 UTC (permalink / raw)
  To: netdev, davem; +Cc: xdp-newbies, bpf, Stephen Hemminger, Jason Wang

When a device is stacked like (team, bonding, failsafe or netvsc) the
XDP generic program for the parent device is not called.  In these
cases, the rx handler changes skb->dev to its own in the receive
handler, and returns RX_HANDLER_ANOTHER.  Fix this by calling
do_xdp_generic if necessary before starting another round.

Review of all the places RX_HANDLER_ANOTHER is returned
show that the current devices do correctly change skb->dev.

There was an older patch that got abandoned that did the
same thing, this is just a rewrite.

Suggested-by: Jason Wang <jasowang@redhat.com>
Fixes: d445516966dc ("net: xdp: support xdp generic on virtual devices")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 net/core/dev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 108ac8137b9b..9165fd3c9e90 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4921,6 +4921,16 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
 			ret = NET_RX_SUCCESS;
 			goto out;
 		case RX_HANDLER_ANOTHER:
+			if (static_branch_unlikely(&generic_xdp_needed_key)) {
+				struct bpf_prog *xdp_prog;
+
+				xdp_prog = rcu_dereference(skb->dev->xdp_prog);
+				ret = do_xdp_generic(xdp_prog, skb);
+				if (ret != XDP_PASS) {
+					ret = NET_RX_SUCCESS;
+					goto out;
+				}
+			}
 			goto another_round;
 		case RX_HANDLER_EXACT:
 			deliver_exact = true;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net 3/3] netdevice: clarify meaning of rx_handler_result
  2019-05-16 21:54 [PATCH net 0/3] XDP generic related fixes Stephen Hemminger
  2019-05-16 21:54 ` [PATCH net 1/3] netvsc: unshare skb in VF rx handler Stephen Hemminger
  2019-05-16 21:54 ` [PATCH net 2/3] net: core: generic XDP support for stacked device Stephen Hemminger
@ 2019-05-16 21:54 ` Stephen Hemminger
  2019-05-16 22:25   ` Jakub Kicinski
  2 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2019-05-16 21:54 UTC (permalink / raw)
  To: netdev, davem; +Cc: xdp-newbies, bpf, Stephen Hemminger

Make the language in comment about rx_handler_result clearer.
Especially the meaning of RX_HANDLER_ANOTHER.

Replace use of "should" with "must" to be in line with common
usage in standards documents.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 include/linux/netdevice.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 44b47e9df94a..56f613561909 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -374,10 +374,10 @@ typedef enum gro_result gro_result_t;
 
 /*
  * enum rx_handler_result - Possible return values for rx_handlers.
- * @RX_HANDLER_CONSUMED: skb was consumed by rx_handler, do not process it
- * further.
- * @RX_HANDLER_ANOTHER: Do another round in receive path. This is indicated in
- * case skb->dev was changed by rx_handler.
+ * @RX_HANDLER_CONSUMED: skb was consumed by rx_handler.
+ *  Do not process it further.
+ * @RX_HANDLER_ANOTHER: skb->dev was modified by rx_handler,
+ *  Do another round in receive path. This is indicated in
  * @RX_HANDLER_EXACT: Force exact delivery, no wildcard.
  * @RX_HANDLER_PASS: Do nothing, pass the skb as if no rx_handler was called.
  *
@@ -394,20 +394,20 @@ typedef enum gro_result gro_result_t;
  * Upon return, rx_handler is expected to tell __netif_receive_skb() what to
  * do with the skb.
  *
- * If the rx_handler consumed the skb in some way, it should return
+ * If the rx_handler consumed the skb in some way, it must return
  * RX_HANDLER_CONSUMED. This is appropriate when the rx_handler arranged for
  * the skb to be delivered in some other way.
  *
  * If the rx_handler changed skb->dev, to divert the skb to another
- * net_device, it should return RX_HANDLER_ANOTHER. The rx_handler for the
+ * net_device, it must return RX_HANDLER_ANOTHER. The rx_handler for the
  * new device will be called if it exists.
  *
- * If the rx_handler decides the skb should be ignored, it should return
+ * If the rx_handler decides the skb should be ignored, it must return
  * RX_HANDLER_EXACT. The skb will only be delivered to protocol handlers that
  * are registered on exact device (ptype->dev == skb->dev).
  *
  * If the rx_handler didn't change skb->dev, but wants the skb to be normally
- * delivered, it should return RX_HANDLER_PASS.
+ * delivered, it must return RX_HANDLER_PASS.
  *
  * A device without a registered rx_handler will behave as if rx_handler
  * returned RX_HANDLER_PASS.
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net 3/3] netdevice: clarify meaning of rx_handler_result
  2019-05-16 21:54 ` [PATCH net 3/3] netdevice: clarify meaning of rx_handler_result Stephen Hemminger
@ 2019-05-16 22:25   ` Jakub Kicinski
  2019-05-16 23:25     ` Stephen Hemminger
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2019-05-16 22:25 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, davem, xdp-newbies, bpf, Stephen Hemminger

On Thu, 16 May 2019 14:54:23 -0700, Stephen Hemminger wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..56f613561909 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -374,10 +374,10 @@ typedef enum gro_result gro_result_t;
>  
>  /*
>   * enum rx_handler_result - Possible return values for rx_handlers.
> - * @RX_HANDLER_CONSUMED: skb was consumed by rx_handler, do not process it
> - * further.
> - * @RX_HANDLER_ANOTHER: Do another round in receive path. This is indicated in
> - * case skb->dev was changed by rx_handler.
> + * @RX_HANDLER_CONSUMED: skb was consumed by rx_handler.
> + *  Do not process it further.
> + * @RX_HANDLER_ANOTHER: skb->dev was modified by rx_handler,
> + *  Do another round in receive path. This is indicated in

s/ This is indicated in//

>   * @RX_HANDLER_EXACT: Force exact delivery, no wildcard.
>   * @RX_HANDLER_PASS: Do nothing, pass the skb as if no rx_handler was called.
>   *


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net 3/3] netdevice: clarify meaning of rx_handler_result
  2019-05-16 22:25   ` Jakub Kicinski
@ 2019-05-16 23:25     ` Stephen Hemminger
  0 siblings, 0 replies; 7+ messages in thread
From: Stephen Hemminger @ 2019-05-16 23:25 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem, xdp-newbies, bpf, Stephen Hemminger

On Thu, 16 May 2019 15:25:43 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Thu, 16 May 2019 14:54:23 -0700, Stephen Hemminger wrote:
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 44b47e9df94a..56f613561909 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -374,10 +374,10 @@ typedef enum gro_result gro_result_t;
> >  
> >  /*
> >   * enum rx_handler_result - Possible return values for rx_handlers.
> > - * @RX_HANDLER_CONSUMED: skb was consumed by rx_handler, do not process it
> > - * further.
> > - * @RX_HANDLER_ANOTHER: Do another round in receive path. This is indicated in
> > - * case skb->dev was changed by rx_handler.
> > + * @RX_HANDLER_CONSUMED: skb was consumed by rx_handler.
> > + *  Do not process it further.
> > + * @RX_HANDLER_ANOTHER: skb->dev was modified by rx_handler,
> > + *  Do another round in receive path. This is indicated in  
> 
> s/ This is indicated in//

Thanks, left over from previous version forgot to trim.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net 2/3] net: core: generic XDP support for stacked device
  2019-05-16 21:54 ` [PATCH net 2/3] net: core: generic XDP support for stacked device Stephen Hemminger
@ 2019-05-17  1:09   ` Jason Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2019-05-17  1:09 UTC (permalink / raw)
  To: Stephen Hemminger, netdev, davem; +Cc: xdp-newbies, bpf, Stephen Hemminger


On 2019/5/17 上午5:54, Stephen Hemminger wrote:
> When a device is stacked like (team, bonding, failsafe or netvsc) the
> XDP generic program for the parent device is not called.  In these
> cases, the rx handler changes skb->dev to its own in the receive
> handler, and returns RX_HANDLER_ANOTHER.  Fix this by calling
> do_xdp_generic if necessary before starting another round.
>
> Review of all the places RX_HANDLER_ANOTHER is returned
> show that the current devices do correctly change skb->dev.
>
> There was an older patch that got abandoned that did the
> same thing, this is just a rewrite.
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Fixes: d445516966dc ("net: xdp: support xdp generic on virtual devices")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>   net/core/dev.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 108ac8137b9b..9165fd3c9e90 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4921,6 +4921,16 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
>   			ret = NET_RX_SUCCESS;
>   			goto out;
>   		case RX_HANDLER_ANOTHER:
> +			if (static_branch_unlikely(&generic_xdp_needed_key)) {
> +				struct bpf_prog *xdp_prog;
> +
> +				xdp_prog = rcu_dereference(skb->dev->xdp_prog);
> +				ret = do_xdp_generic(xdp_prog, skb);
> +				if (ret != XDP_PASS) {
> +					ret = NET_RX_SUCCESS;
> +					goto out;
> +				}
> +			}
>   			goto another_round;
>   		case RX_HANDLER_EXACT:
>   			deliver_exact = true;


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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16 21:54 [PATCH net 0/3] XDP generic related fixes Stephen Hemminger
2019-05-16 21:54 ` [PATCH net 1/3] netvsc: unshare skb in VF rx handler Stephen Hemminger
2019-05-16 21:54 ` [PATCH net 2/3] net: core: generic XDP support for stacked device Stephen Hemminger
2019-05-17  1:09   ` Jason Wang
2019-05-16 21:54 ` [PATCH net 3/3] netdevice: clarify meaning of rx_handler_result Stephen Hemminger
2019-05-16 22:25   ` Jakub Kicinski
2019-05-16 23:25     ` Stephen Hemminger

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org bpf@archiver.kernel.org
	public-inbox-index bpf


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/ public-inbox