netdev - Re: [RFC PATCH 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] [day] [month] [year] [list]

Message-ID: <20211118215215.f4xnwwjhlydxjqnt@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 18 Nov 2021 13:52:15 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] bpf: let bpf_warn_invalid_xdp_action() report
 more info

On Thu, Nov 18, 2021 at 12:50:58PM +0100, Toke Høiland-Jørgensen wrote:
> Paolo Abeni <pabeni@...hat.com> writes:
> 
> > Hello,
> >
> > On Wed, 2021-11-17 at 16:48 -0800, Alexei Starovoitov wrote:
> >> On Mon, Nov 15, 2021 at 06:09:28PM +0100, Toke Høiland-Jørgensen wrote:
> >> > Paolo Abeni <pabeni@...hat.com> writes:
> >> > 
> >> > > > > -	pr_warn_once("%s XDP return value %u, expect packet loss!\n",
> >> > > > > +	pr_warn_once("%s XDP return value %u on prog %d dev %s attach type %d, expect packet loss!\n",
> >> > > > >  		     act > act_max ? "Illegal" : "Driver unsupported",
> >> > > > > -		     act);
> >> > > > > +		     act, prog->aux->id, dev->name, prog->expected_attach_type);
> >> > > > 
> >> > > > This will only ever trigger once per reboot even if the message differs,
> >> > > > right? Which makes it less useful as a debugging aid; so I'm not sure if
> >> > > > it's really worth it with this intrusive change unless we also do
> >> > > > something to add a proper debugging aid (like a tracepoint)...
> >> > > 
> >> > > Yes, the idea would be to add a tracepoint there, if there is general
> >> > > agreement about this change.
> >> > > 
> >> > > I think this patch is needed because the WARN_ONCE splat gives
> >> > > implicitly information about the related driver and attach type.
> >> > > Replacing with a simple printk we lose them.
> >> > 
> >> > Ah, right, good point. Pointing that out in the commit message might be
> >> > a good idea; otherwise people may miss that ;)
> >> 
> >> Though it's quite a churn across the drivers I think extra verbosity here is justified.
> >> I'd only suggest to print stable things. Like prog->aux->id probably has
> >> little value for the person looking at the logs. That prog id is likely gone.
> >> If it was prog->aux->name it would be more helpful.
> >> Same with expected_attach_type. Why print it at all?
> >> tracepoint is probably good idea too.
> >
> > Thanks for the feedback.
> >
> > I tried to select the additional arguments to allow the user/admin
> > tracking down which program is causing the issue and why. I'm a
> > complete newbe wrt XDP, so likely my choice were naive.
> >
> > I thought the id identifies the program in an unambiguous manner. I
> > understand the program could be unloaded meanwhile, but if that is not
> > the case the id should be quite useful. Perhaps we could dump both the
> > id and the name?
> >
> > I included the attach type as different types support/allow different
> > actions: the same program could cause the warning or not depending on
> > it. If that is not useful I can drop the attach type from the next
> > iteration.
> 
> The attach type identifies DEVMAP and CPUMAP programs, but just printing
> it as a number probably doesn't make sense. So maybe something like:
> 
> switch(prog->expected_attach_type) {
>     case BPF_XDP_DEVMAP:
>     case BPF_XDP_CPUMAP:
>       pr_warn_once("Illegal XDP return value %u from prog %s(%d) in %s!\n",
>                    act, prog->aux_name, prog->aux->id,
>                    prog->expected_attach_type == BPF_XDP_DEVMAP ? "devmap" : "cpumap");
>       break;
>     default:
>       pr_warn_once("%s XDP return value %u on prog %s(%d) dev %s, expect packet loss!\n",
>                    act > act_max ? "Illegal" : "Driver unsupported",
>                    act, prog->aux->name, prog->aux->id, dev->name);
>       break;

imo it's an overkill for a debug message that should not be seen.
Just attach bpf probe to this function and print whatever necessary.
Keep kernel message single line and short that is easily greppable.