On Mon, 2021-11-22 at 12:32 +0100, Toke Høiland-Jørgensen wrote:
> 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? :/
I'm sorry, quite a bit of relevant incoming messages keep landing in
the spam folder due to corporate/gmail filters.
> Just pass NULL as the dev from cpumap/devmap and don't print the dev
> name in this case...
No opposition on my side, but I think that is a bit conflicting with
Alexei suggestion of keeping the message handling as simple as
possible?!?
@Alexei, would something like the following:
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, prog->aux->name, prog->aux->id, dev ? dev->name, "");
fit you?
Thanks!
Paolo
