netdev - Re: [PATCH net-next] net: add network device notifier trace points

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: <20181219083643.7f724e59@redhat.com>
Date:   Wed, 19 Dec 2018 08:36:43 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     brouer@...hat.com, netdev@...r.kernel.org,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH net-next] net: add network device notifier trace points

On Tue, 18 Dec 2018 18:27:06 -0800
Stephen Hemminger <stephen@...workplumber.org> wrote:

> This is the result of a conversation about monitoring of link
> state changes with BPF.

If you want to use this from BPF then you are in for a surprise.  As
tracepoints BPF cannot read these "__string" constructs, here the netdev
name.  I tried a lot of different tricks that didn't work, see [1],
until Alexei explained that it simply isn't supported.

I instead recommend adding the ifindex to the tracepoint.  The__string
and __assign_str is also a performance concern as it does strcpy behind
your back.

I have an year old TODO list item about improving this:
 ** TODO Make perf-script plugin for ifindex to name translation
    SCHEDULED: <2017-11-20 Mon>

Today, the existing network tracepoints using dev->name is not that
usable by BPF, as BPF cannot identify the interface.  Thus IMHO it would
make sense to convert the existing network tracepoints dev->name into
dev->ifindex, and then let perf-script convert this to the interface
name.  Either in userspace via if_indextoname(3), or (as ACME pointed
out at the time) we might want to have a lookup table stored together
with perf.data for later inspection (in-case ifindexes changed).


[1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/napi_monitor_kern.c#L34-L130

> Signed-off-by: Stephen Hemminger <stephen@...workplumber.org>
> ---
>  include/trace/events/net.h | 115 +++++++++++++++++++++++++++++++++++++
>  net/core/dev.c             |   9 ++-
>  2 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/net.h b/include/trace/events/net.h
> index 1efd7d9b25fe..141310d24610 100644
> --- a/include/trace/events/net.h
> +++ b/include/trace/events/net.h
[...]
> +TRACE_EVENT(net_dev_notifier_entry,
> +
> +	TP_PROTO(const struct netdev_notifier_info *info, unsigned long val),
> +
> +	TP_ARGS(info, val),
> +
> +	TP_STRUCT__entry(
> +		__string(	name,		 info->dev->name )
> +		__field(	enum netdev_cmd, event	         )
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(name, info->dev->name);
> +		__entry->event = val;
> +       ),

These __string and __assign_str are costly and behind the scenes does a
strcpy.

> +
> +	TP_printk("dev=%s event=%s",
> +		  __get_str(name), netdev_event_type(__entry->event))
> +);
> +
> +TRACE_EVENT(net_dev_notifier,
> +
> +	TP_PROTO(const struct netdev_notifier_info *info, int rc, unsigned long val),
> +
> +	TP_ARGS(info, rc, val),
> +
> +	TP_STRUCT__entry(
> +		__string(	name,		 info->dev->name   )
> +		__field(	enum netdev_cmd, event	           )
> +		__field(	int,		 rc		   )
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(name, info->dev->name);
> +		__entry->event = val;
> +		__entry->rc = rc;
> +       ),
> +
> +	TP_printk("dev=%s event=%s ret=%d",
> +		  __get_str(name), netdev_event_type(__entry->event),
> +		  __entry->rc)
> +);
> +

The bare minimum change is to _also_ add (info->dev->)ifindex to the
tracepoint, as this makes it usable from BPF.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer