On 2018/05/01 2:27, Jesper Dangaard Brouer wrote:

On Thu, 26 Apr 2018 19:52:40 +0900
Toshiaki Makita [off-list ref] wrote:

quoted

On 2018/04/26 5:24, Jesper Dangaard Brouer wrote:

quoted

On Tue, 24 Apr 2018 23:39:20 +0900
Toshiaki Makita [off-list ref] wrote:
  

quoted

+static int veth_xdp_xmit(struct net_device *dev, struct xdp_frame *frame)
+{
+	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
+	int headroom = frame->data - (void *)frame;
+	struct net_device *rcv;
+	int err = 0;
+
+	rcv = rcu_dereference(priv->peer);
+	if (unlikely(!rcv))
+		return -ENXIO;
+
+	rcv_priv = netdev_priv(rcv);
+	/* xdp_ring is initialized on receive side? */
+	if (rcu_access_pointer(rcv_priv->xdp_prog)) {
+		err = xdp_ok_fwd_dev(rcv, frame->len);
+		if (unlikely(err))
+			return err;
+
+		err = veth_xdp_enqueue(rcv_priv, veth_xdp_to_ptr(frame));
+	} else {
+		struct sk_buff *skb;
+
+		skb = veth_build_skb(frame, headroom, frame->len, 0);
+		if (unlikely(!skb))
+			return -ENOMEM;
+
+		/* Get page ref in case skb is dropped in netif_rx.
+		 * The caller is responsible for freeing the page on error.
+		 */
+		get_page(virt_to_page(frame->data));  

I'm not sure you can make this assumption, that xdp_frames coming from
another device driver uses a refcnt based memory model. But maybe I'm
confused, as this looks like an SKB receive path, but in the
ndo_xdp_xmit().  

I find this path similar to cpumap, which creates skb from redirected
xdp frame. Once it is converted to skb, skb head is freed by
page_frag_free, so anyway I needed to get the refcount here regardless
of memory model.

Yes I know, I wrote cpumap ;-)

First of all, I don't want to see such xdp_frame to SKB conversion code
in every driver.  Because that increase the chances of errors.  And
when looking at the details, then it seems that you have made the
mistake of making it possible to leak xdp_frame info to the SKB (which
cpumap takes into account).

Do you mean leaving xdp_frame in skb->head is leaking something? how?

Second, I think the refcnt scheme here is wrong. The xdp_frame should
be "owned" by XDP and have the proper refcnt to deliver it directly to
the network stack.

Third, if we choose that we want a fallback, in-case XDP is not enabled
on egress dev (but it have an ndo_xdp_xmit), then it should be placed
in the generic/core code.  E.g. __bpf_tx_xdp_map() could look at the
return code from dev->netdev_ops->ndo_xdp() and create an SKB.  (Hint,
this would make it easy to implement TX bulking towards the dev).

Right, this is a much cleaner way.
Although I feel like we should add this fallback for veth because it
requires something which is different from other drivers (enabling XDP
on the peer device of the egress device), I'll drop the part for now. It
should not be resolved in the driver code.

-- 
Toshiaki Makita