netdev - Re: [RFC/RFT net-next] net: ethernet: ti: fix netdevice stats for XDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <562239c7-547c-fed6-e3f3-87752f6c7402@ti.com>
Date:   Mon, 16 Mar 2020 22:26:09 +0200
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>, <netdev@...r.kernel.org>
CC:     <ilias.apalodimas@...aro.org>, <davem@...emloft.net>,
        <brouer@...hat.com>, <lorenzo.bianconi@...hat.com>
Subject: Re: [RFC/RFT net-next] net: ethernet: ti: fix netdevice stats for XDP



On 21/02/2020 19:05, Lorenzo Bianconi wrote:
> Align netdevice statistics when the device is running in XDP mode
> to other upstream drivers. In particular reports to user-space rx
> packets even if they are not forwarded to the networking stack
> (XDP_PASS) but they are redirected (XDP_REDIRECT), dropped (XDP_DROP)
> or sent back using the same interface (XDP_TX). This patch allows the
> system administrator to very the device is receiving data correctly.

I've tested it with xdp-tutorial:
  drop: ip link set dev eth0 xdp obj ./packet01-parsing/xdp_prog_kern.o sec xdp_packet_parser
  tx: ip link set dev eth0 xdp obj ./packet-solutions/xdp_prog_kern_03.o sec xdp_icmp_echo

And see statistic incremented.

In my opinion, it looks a little bit inconsistent if RX/TX packet/bytes is updated,
but ndev->stats.rx_dropped is not.

> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
> - this patch is compile-only tested
> ---
>   drivers/net/ethernet/ti/cpsw.c      |  4 +---
>   drivers/net/ethernet/ti/cpsw_new.c  |  5 ++---
>   drivers/net/ethernet/ti/cpsw_priv.c | 13 +++++++++++--
>   drivers/net/ethernet/ti/cpsw_priv.h |  2 +-
>   4 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 6ae4a72e6f43..fe3fd33f56f7 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -408,12 +408,10 @@ static void cpsw_rx_handler(void *token, int len, int status)
>   		xdp.rxq = &priv->xdp_rxq[ch];
>   
>   		port = priv->emac_port + cpsw->data.dual_emac;
> -		ret = cpsw_run_xdp(priv, ch, &xdp, page, port);
> +		ret = cpsw_run_xdp(priv, ch, &xdp, page, port, &len);
>   		if (ret != CPSW_XDP_PASS)
>   			goto requeue;
>   
> -		/* XDP prog might have changed packet data and boundaries */
> -		len = xdp.data_end - xdp.data;
>   		headroom = xdp.data - xdp.data_hard_start;
>   
>   		/* XDP prog can modify vlan tag, so can't use encap header */
> diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
> index 71215db7934b..050496e814c3 100644
> --- a/drivers/net/ethernet/ti/cpsw_new.c
> +++ b/drivers/net/ethernet/ti/cpsw_new.c
> @@ -349,12 +349,11 @@ static void cpsw_rx_handler(void *token, int len, int status)
>   		xdp.data_hard_start = pa;
>   		xdp.rxq = &priv->xdp_rxq[ch];
>   
> -		ret = cpsw_run_xdp(priv, ch, &xdp, page, priv->emac_port);
> +		ret = cpsw_run_xdp(priv, ch, &xdp, page,
> +				   priv->emac_port, &len);
>   		if (ret != CPSW_XDP_PASS)
>   			goto requeue;
>   
> -		/* XDP prog might have changed packet data and boundaries */
> -		len = xdp.data_end - xdp.data;
>   		headroom = xdp.data - xdp.data_hard_start;
>   
>   		/* XDP prog can modify vlan tag, so can't use encap header */
> diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c
> index 97a058ca60ac..a41da48db40b 100644
> --- a/drivers/net/ethernet/ti/cpsw_priv.c
> +++ b/drivers/net/ethernet/ti/cpsw_priv.c
> @@ -1317,7 +1317,7 @@ int cpsw_xdp_tx_frame(struct cpsw_priv *priv, struct xdp_frame *xdpf,
>   }
>   
>   int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> -		 struct page *page, int port)
> +		 struct page *page, int port, int *len)
>   {
>   	struct cpsw_common *cpsw = priv->cpsw;
>   	struct net_device *ndev = priv->ndev;
> @@ -1335,10 +1335,13 @@ int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
>   	}
>   
>   	act = bpf_prog_run_xdp(prog, xdp);
> +	/* XDP prog might have changed packet data and boundaries */
> +	*len = xdp.data_end - xdp.data;

it should be

+       *len = xdp->data_end - xdp->data;


> +
>   	switch (act) {
>   	case XDP_PASS:
>   		ret = CPSW_XDP_PASS;
> -		break;
> +		goto out;
>   	case XDP_TX:
>   		xdpf = convert_to_xdp_frame(xdp);
>   		if (unlikely(!xdpf))
> @@ -1364,8 +1367,14 @@ int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
>   		trace_xdp_exception(ndev, prog, act);
>   		/* fall through -- handle aborts by dropping packet */
>   	case XDP_DROP:
> +		ndev->stats.rx_bytes += *len;
> +		ndev->stats.rx_packets++;
>   		goto drop;
>   	}
> +
> +	ndev->stats.rx_bytes += *len;
> +	ndev->stats.rx_packets++;
> +
>   out:
>   	rcu_read_unlock();
>   	return ret;
> diff --git a/drivers/net/ethernet/ti/cpsw_priv.h b/drivers/net/ethernet/ti/cpsw_priv.h
> index b8d7b924ee3d..54efd773e033 100644
> --- a/drivers/net/ethernet/ti/cpsw_priv.h
> +++ b/drivers/net/ethernet/ti/cpsw_priv.h
> @@ -439,7 +439,7 @@ int cpsw_ndo_bpf(struct net_device *ndev, struct netdev_bpf *bpf);
>   int cpsw_xdp_tx_frame(struct cpsw_priv *priv, struct xdp_frame *xdpf,
>   		      struct page *page, int port);
>   int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> -		 struct page *page, int port);
> +		 struct page *page, int port, int *len);
>   irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id);
>   irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id);
>   int cpsw_tx_mq_poll(struct napi_struct *napi_tx, int budget);
> 

-- 
Best regards,
grygorii