netdev - Re: [PATCH net-next 2/2] bpf: let bpf_warn_invalid_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: <87tug4cto2.fsf@toke.dk>
Date:   Mon, 22 Nov 2021 12:32:29 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] bpf: let bpf_warn_invalid_xdp_action()
 report more info


You still have:

> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index 585b2b77ccc4..f7359bcb8fa3 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -195,7 +195,7 @@ static void cpu_map_bpf_prog_run_skb(struct bpf_cpu_map_entry *rcpu,
>  			}
>  			return;
>  		default:
> -			bpf_warn_invalid_xdp_action(act);
> +			bpf_warn_invalid_xdp_action(skb->dev, rcpu->prog, act);
>  			fallthrough;
>  		case XDP_ABORTED:
>  			trace_xdp_exception(skb->dev, rcpu->prog, act);
> @@ -254,7 +254,7 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
>  			}
>  			break;
>  		default:
> -			bpf_warn_invalid_xdp_action(act);
> +			bpf_warn_invalid_xdp_action(xdpf->dev_rx, rcpu->prog, act);
>  			fallthrough;
>  		case XDP_DROP:
>  			xdp_return_frame(xdpf);
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index f02d04540c0c..79bcf2169881 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -348,7 +348,7 @@ static int dev_map_bpf_prog_run(struct bpf_prog *xdp_prog,
>  				frames[nframes++] = xdpf;
>  			break;
>  		default:
> -			bpf_warn_invalid_xdp_action(act);
> +			bpf_warn_invalid_xdp_action(dev, xdp_prog, act);
>  			fallthrough;
>  		case XDP_ABORTED:
>  			trace_xdp_exception(dev, xdp_prog, act);
> @@ -507,7 +507,7 @@ static u32 dev_map_bpf_prog_run_skb(struct sk_buff *skb, struct bpf_dtab_netdev
>  		__skb_push(skb, skb->mac_len);
>  		break;
>  	default:
> -		bpf_warn_invalid_xdp_action(act);
> +		bpf_warn_invalid_xdp_action(dst->dev, dst->xdp_prog, act);
>  		fallthrough;
>  	case XDP_ABORTED:
>  		trace_xdp_exception(dst->dev, dst->xdp_prog, act);

... and ...

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3ba584bb23f8..658f7a84d9bc 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8179,13 +8179,13 @@ static bool xdp_is_valid_access(int off, int size,
>  	return __is_valid_xdp_access(off, size);
>  }
>  
> -void bpf_warn_invalid_xdp_action(u32 act)
> +void bpf_warn_invalid_xdp_action(struct net_device *dev, struct bpf_prog *prog, u32 act)
>  {
>  	const u32 act_max = XDP_REDIRECT;
>  
> -	pr_warn_once("%s XDP return value %u, expect packet loss!\n",
> +	pr_warn_once("%s XDP return value %u on prog %s (id %d) dev %s, expect packet loss!\n",
>  		     act > act_max ? "Illegal" : "Driver unsupported",
> -		     act);
> +		     act, prog->aux->name, prog->aux->id, dev->name);
>  }
>  EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);

This will still print the dev name for cpumap and devmap programs, which
is misleading - it will have people looking at the drivers when the
problem is somewhere else.

I pointed this out multiple times in comments on your last revision so
I'm a bit puzzled as to why you're still doing this? :/

Just pass NULL as the dev from cpumap/devmap and don't print the dev
name in this case...

-Toke