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  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:   Sat, 25 Apr 2020 12:24:07 +0900
From:   Toshiaki Makita <toshiaki.makita1@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     sameehj@...zon.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
        zorik@...zon.com, akiyano@...zon.com, gtzalik@...zon.com,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        David Ahern <dsahern@...il.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        steffen.klassert@...unet.com
Subject: Re: [PATCH net-next 07/33] xdp: xdp_frame add member frame_sz and
 handle in convert_to_xdp_frame

On 2020/04/23 1:07, Jesper Dangaard Brouer wrote:
> Use hole in struct xdp_frame, when adding member frame_sz, which keeps
> same sizeof struct (32 bytes)
> 
> Drivers ixgbe and sfc had bug cases where the necessary/expected
> tailroom was not reserved. This can lead to some hard to catch memory
> corruption issues. Having the drivers frame_sz this can be detected when
> packet length/end via xdp->data_end exceed the xdp_data_hard_end
> pointer, which accounts for the reserved the tailroom.
> 
> When detecting this driver issue, simply fail the conversion with NULL,
> which results in feedback to driver (failing xdp_do_redirect()) causing
> driver to drop packet. Given the lack of consistent XDP stats, this can
> be hard to troubleshoot. And given this is a driver bug, we want to
> generate some more noise in form of a WARN stack dump (to ID the driver
> code that inlined convert_to_xdp_frame).
> 
> Inlining the WARN macro is problematic, because it adds an asm
> instruction (on Intel CPUs ud2) what influence instruction cache
> prefetching. Thus, introduce xdp_warn and macro XDP_WARN, to avoid this
> and at the same time make identifying the function and line of this
> inlined function easier.
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>   include/net/xdp.h |   14 +++++++++++++-
>   net/core/xdp.c    |    7 +++++++
>   2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 99f4374f6214..55a885aa4e53 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -93,7 +93,8 @@ struct xdp_frame {
>   	void *data;
>   	u16 len;
>   	u16 headroom;
> -	u16 metasize;
> +	u32 metasize:8;
> +	u32 frame_sz:24;
>   	/* Lifetime of xdp_rxq_info is limited to NAPI/enqueue time,
>   	 * while mem info is valid on remote CPU.
>   	 */
> @@ -108,6 +109,10 @@ static inline void xdp_scrub_frame(struct xdp_frame *frame)
>   	frame->dev_rx = NULL;
>   }
>   
> +/* Avoids inlining WARN macro in fast-path */
> +void xdp_warn(const char* msg, const char* func, const int line);
> +#define XDP_WARN(msg) xdp_warn(msg, __func__, __LINE__)

Shouldn't this have WARN_ONCE()-like mechanism?
A buggy driver may generate massive amount of dump messages...

Toshiaki Makita

> +
>   struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp);
>   
>   /* Convert xdp_buff to xdp_frame */
> @@ -128,6 +133,12 @@ struct xdp_frame *convert_to_xdp_frame(struct xdp_buff *xdp)
>   	if (unlikely((headroom - metasize) < sizeof(*xdp_frame)))
>   		return NULL;
>   
> +	/* Catch if driver didn't reserve tailroom for skb_shared_info */
> +	if (unlikely(xdp->data_end > xdp_data_hard_end(xdp))) {
> +		XDP_WARN("Driver BUG: missing reserved tailroom");
> +		return NULL;
> +	}
> +
>   	/* Store info in top of packet */
>   	xdp_frame = xdp->data_hard_start;
>   
> @@ -135,6 +146,7 @@ struct xdp_frame *convert_to_xdp_frame(struct xdp_buff *xdp)
>   	xdp_frame->len  = xdp->data_end - xdp->data;
>   	xdp_frame->headroom = headroom - sizeof(*xdp_frame);
>   	xdp_frame->metasize = metasize;
> +	xdp_frame->frame_sz = xdp->frame_sz;
>   
>   	/* rxq only valid until napi_schedule ends, convert to xdp_mem_info */
>   	xdp_frame->mem = xdp->rxq->mem;
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 4c7ea85486af..4bc3026ae218 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -11,6 +11,7 @@
>   #include <linux/slab.h>
>   #include <linux/idr.h>
>   #include <linux/rhashtable.h>
> +#include <linux/bug.h>
>   #include <net/page_pool.h>
>   
>   #include <net/xdp.h>
> @@ -496,3 +497,9 @@ struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp)
>   	return xdpf;
>   }
>   EXPORT_SYMBOL_GPL(xdp_convert_zc_to_xdp_frame);
> +
> +/* Used by XDP_WARN macro, to avoid inlining WARN() in fast-path */
> +void xdp_warn(const char* msg, const char* func, const int line) {
> +	WARN(1, "XDP_WARN: %s(line:%d): %s\n", func, line, msg);
> +};
> +EXPORT_SYMBOL_GPL(xdp_warn);
> 
> 

Powered by blists - more mailing lists