| Message ID | 158757167661.1370371.5983006045491610549.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [net-next,01/33] xdp: add frame size to xdp_buff | expand |
Jesper Dangaard Brouer <brouer@redhat.com> writes: > 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@redhat.com> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
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@redhat.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); > >
On Sat, 25 Apr 2020 12:24:07 +0900 Toshiaki Makita <toshiaki.makita1@gmail.com> wrote: > > +/* 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... Well, in this use-case I think I want it be loud. I usually miss those WARN_ONCE messages, and I while extending and testing drivers, it was an advantage that is was loud, as it caught some of my own bugs.
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__) + 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);
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@redhat.com> --- include/net/xdp.h | 14 +++++++++++++- net/core/xdp.c | 7 +++++++ 2 files changed, 20 insertions(+), 1 deletion(-)