netdev - Re: [PATCH RFC-v5 bpf-next 05/12] net: core: rename netif_receive_generic_xdp to do_generic_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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87mu7bled5.fsf@toke.dk>
Date:   Thu, 16 Apr 2020 16:00:38 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, kuba@...nel.org,
        prashantbhole.linux@...il.com, jasowang@...hat.com,
        brouer@...hat.com, toshiaki.makita1@...il.com,
        daniel@...earbox.net, john.fastabend@...il.com, ast@...nel.org,
        kafai@...com, songliubraving@...com, yhs@...com, andriin@...com,
        dsahern@...il.com, David Ahern <dahern@...italocean.com>
Subject: Re: [PATCH RFC-v5 bpf-next 05/12] net: core: rename netif_receive_generic_xdp to do_generic_xdp_core

David Ahern <dsahern@...nel.org> writes:

> From: David Ahern <dahern@...italocean.com>
>
> In skb generic path, we need a way to run XDP program on skb but
> to have customized handling of XDP actions. netif_receive_generic_xdp
> will be more helpful in such cases than do_xdp_generic.
>
> This patch prepares netif_receive_generic_xdp() to be used as general
> purpose function for running xdp programs on skbs by renaming it to
> do_xdp_generic_core, moving skb_is_redirected and rxq settings as well
> as XDP return code checks to the callers.
>
> This allows this core function to be used from both Rx and Tx paths
> with rxq and txq set based on context.
>
> Signed-off-by: Jason Wang <jasowang@...hat.com>
> Signed-off-by: Prashant Bhole <prashantbhole.linux@...il.com>
> Signed-off-by: David Ahern <dahern@...italocean.com>
> ---
>  net/core/dev.c | 52 ++++++++++++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e763b6cea8ff..4f0c4fee1125 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4500,25 +4500,17 @@ static struct netdev_rx_queue *netif_get_rxqueue(struct sk_buff *skb)
>  	return rxqueue;
>  }
>  
> -static u32 netif_receive_generic_xdp(struct sk_buff *skb,
> -				     struct xdp_buff *xdp,
> -				     struct bpf_prog *xdp_prog)
> +static u32 do_xdp_generic_core(struct sk_buff *skb, struct xdp_buff *xdp,
> +			       struct bpf_prog *xdp_prog)
>  {
> -	struct netdev_rx_queue *rxqueue;
>  	void *orig_data, *orig_data_end;
> -	u32 metalen, act = XDP_DROP;
>  	__be16 orig_eth_type;
>  	struct ethhdr *eth;
> +	u32 metalen, act;
>  	bool orig_bcast;
>  	int hlen, off;
>  	u32 mac_len;
>  
> -	/* Reinjected packets coming from act_mirred or similar should
> -	 * not get XDP generic processing.
> -	 */
> -	if (skb_is_redirected(skb))
> -		return XDP_PASS;
> -
>  	/* XDP packets must be linear and must have sufficient headroom
>  	 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
>  	 * native XDP provides, thus we need to do it here as well.
> @@ -4534,9 +4526,9 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  		if (pskb_expand_head(skb,
>  				     hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
>  				     troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
> -			goto do_drop;
> +			return XDP_DROP;
>  		if (skb_linearize(skb))
> -			goto do_drop;
> +			return XDP_DROP;
>  	}
>  
>  	/* The XDP program wants to see the packet starting at the MAC
> @@ -4554,9 +4546,6 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  	orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest);
>  	orig_eth_type = eth->h_proto;
>  
> -	rxqueue = netif_get_rxqueue(skb);
> -	xdp->rxq = &rxqueue->xdp_rxq;
> -
>  	act = bpf_prog_run_xdp(xdp_prog, xdp);
>  
>  	/* check if bpf_xdp_adjust_head was used */
> @@ -4599,16 +4588,6 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  		if (metalen)
>  			skb_metadata_set(skb, metalen);
>  		break;
> -	default:
> -		bpf_warn_invalid_xdp_action(act);
> -		/* fall through */
> -	case XDP_ABORTED:
> -		trace_xdp_exception(skb->dev, xdp_prog, act);
> -		/* fall through */
> -	case XDP_DROP:
> -	do_drop:
> -		kfree_skb(skb);
> -		break;
>  	}
>  
>  	return act;
> @@ -4643,12 +4622,22 @@ static DEFINE_STATIC_KEY_FALSE(generic_xdp_needed_key);
>  
>  int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
>  {
> +	/* Reinjected packets coming from act_mirred or similar should
> +	 * not get XDP generic processing.
> +	 */

My immediate thought when reading this was "wait, we're doing TX now, is
this still true?". And then I saw the next patch where you're renaming
the function; so maybe switch those two patches, or merge them?

-Toke