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: <20211118004852.tn2jewjm55dwwa5y@ast-mbp>
Date:   Wed, 17 Nov 2021 16:48:52 -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 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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ