netdev - RE: [PATCH bpf-next v2 1/6] xsk: add tracepoints for packet drops

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]

Date:   Wed, 27 Jan 2021 14:10:24 +0000
From:   "Loftus, Ciara" <ciara.loftus@...el.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "bjorn@...nel.org" <bjorn@...nel.org>,
        "Janjua, Weqaar A" <weqaar.a.janjua@...el.com>
Subject: RE: [PATCH bpf-next v2 1/6] xsk: add tracepoints for packet drops

> On 1/26/21 8:52 AM, Ciara Loftus wrote:
> > This commit introduces static perf tracepoints for AF_XDP which
> > report information about packet drops, such as the netdev, qid and
> > high level reason for the drop. The tracepoint can be
> > enabled/disabled by toggling
> > /sys/kernel/debug/tracing/events/xsk/xsk_packet_drop/enable
> 
> Could you add a rationale to the commit log on why xsk diag stats dump
> is insufficient here given you add tracepoints to most locations where
> we also bump the counters already? And given diag infra also exposes the
> ifindex, queue id, etc, why troubleshooting the xsk socket via ss tool
> is not sufficient?

Thanks for your feedback Daniel.

The stats tell us that there is *a* problem whereas the traces will shed
light on what that problem is. eg. The XSK_TRACE_DROP_PKT_TOO_BIG
trace tells us we dropped a packet on RX due to it being too big vs. ss
would just tell us the packet was dropped.
I will add this rationale in the v3.

> 
> [...]
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 4faabd1ecfd1..9b850716630b 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -11,6 +11,7 @@
> >
> >   #define pr_fmt(fmt) "AF_XDP: %s: " fmt, __func__
> >
> > +#include <linux/bpf_trace.h>
> >   #include <linux/if_xdp.h>
> >   #include <linux/init.h>
> >   #include <linux/sched/mm.h>
> > @@ -158,6 +159,7 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, struct
> xdp_buff *xdp, u32 len)
> >   	addr = xp_get_handle(xskb);
> >   	err = xskq_prod_reserve_desc(xs->rx, addr, len);
> >   	if (err) {
> > +		trace_xsk_packet_drop(xs->dev->name, xs->queue_id,
> XSK_TRACE_DROP_RXQ_FULL);
> >   		xs->rx_queue_full++;
> >   		return err;
> 
> I presume if this will be triggered under stress you'll likely also spam
> your trace event log w/ potentially mio of msgs per sec?

You are correct. After some consideration I'm going to drop this
trace and some others in the next rev which are not technically
guaranteed drops and could end up spamming the log under
stress as you mentioned.

> 
> >   	}
> > @@ -192,6 +194,7 @@ static int __xsk_rcv(struct xdp_sock *xs, struct
> xdp_buff *xdp)
> >
> >   	len = xdp->data_end - xdp->data;
> >   	if (len > xsk_pool_get_rx_frame_size(xs->pool)) {
> > +		trace_xsk_packet_drop(xs->dev->name, xs->queue_id,
> XSK_TRACE_DROP_PKT_TOO_BIG);
> >   		xs->rx_dropped++;
> >   		return -ENOSPC;
> >   	}
> > @@ -516,6 +519,8 @@ static int xsk_generic_xmit(struct sock *sk)
> >   		if (err == NET_XMIT_DROP) {
> >   			/* SKB completed but not sent */
> >   			err = -EBUSY;
> > +			trace_xsk_packet_drop(xs->dev->name, xs-
> >queue_id,
> > +					      XSK_TRACE_DROP_DRV_ERR_TX);
> 
> Is there a reason to not bump error counter here?

This too falls into the not-technically-a-drop category and will be
removed in the next rev. I think a stat would be useful though.
I'll draw up a separate patch. Thanks for the suggestion.

> 
> >   			goto out;
> >   		}
> >
> > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > index 8de01aaac4a0..d3c1ca83c75d 100644
> > --- a/net/xdp/xsk_buff_pool.c
> > +++ b/net/xdp/xsk_buff_pool.c
> > @@ -1,5 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >