| Message ID | 7ff49193140f3cb5341732612c72bcc2c5fb3372.1597842004.git.lorenzo@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | mvneta: introduce XDP multi-buffer support | expand |
On Wed, 19 Aug 2020 15:13:49 +0200 Lorenzo Bianconi <lorenzo@kernel.org> wrote: > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 884f140fc3be..006b24b5d276 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -370,19 +370,55 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct) > > void xdp_return_frame(struct xdp_frame *xdpf) > { > + struct skb_shared_info *sinfo; > + int i; > + > __xdp_return(xdpf->data, &xdpf->mem, false); There is a use-after-free race here. The xdpf->data contains the shared_info (xdp_get_shared_info_from_frame(xdpf)). Thus you cannot free/return the page and use this data area below. > + if (!xdpf->mb) > + return; > + > + sinfo = xdp_get_shared_info_from_frame(xdpf); > + for (i = 0; i < sinfo->nr_frags; i++) { > + struct page *page = skb_frag_page(&sinfo->frags[i]); > + > + __xdp_return(page_address(page), &xdpf->mem, false); > + } > } > EXPORT_SYMBOL_GPL(xdp_return_frame); > > void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) > { > + struct skb_shared_info *sinfo; > + int i; > + > __xdp_return(xdpf->data, &xdpf->mem, true); Same issue. > + if (!xdpf->mb) > + return; > + > + sinfo = xdp_get_shared_info_from_frame(xdpf); > + for (i = 0; i < sinfo->nr_frags; i++) { > + struct page *page = skb_frag_page(&sinfo->frags[i]); > + > + __xdp_return(page_address(page), &xdpf->mem, true); > + } > } > EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi); > > void xdp_return_buff(struct xdp_buff *xdp) > { > + struct skb_shared_info *sinfo; > + int i; > + > __xdp_return(xdp->data, &xdp->rxq->mem, true); Same issue. > + if (!xdp->mb) > + return; > + > + sinfo = xdp_get_shared_info_from_buff(xdp); > + for (i = 0; i < sinfo->nr_frags; i++) { > + struct page *page = skb_frag_page(&sinfo->frags[i]); > + > + __xdp_return(page_address(page), &xdp->rxq->mem, true); > + } > }
> On Wed, 19 Aug 2020 15:13:49 +0200 > Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > diff --git a/net/core/xdp.c b/net/core/xdp.c > > index 884f140fc3be..006b24b5d276 100644 > > --- a/net/core/xdp.c > > +++ b/net/core/xdp.c > > @@ -370,19 +370,55 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct) > > > > void xdp_return_frame(struct xdp_frame *xdpf) > > { > > + struct skb_shared_info *sinfo; > > + int i; > > + > > __xdp_return(xdpf->data, &xdpf->mem, false); > > There is a use-after-free race here. The xdpf->data contains the > shared_info (xdp_get_shared_info_from_frame(xdpf)). Thus you cannot > free/return the page and use this data area below. right, thx for pointing this out. I will fix it in v2. Regards, Lorenzo > > > + if (!xdpf->mb) > > + return; > > + > > + sinfo = xdp_get_shared_info_from_frame(xdpf); > > + for (i = 0; i < sinfo->nr_frags; i++) { > > + struct page *page = skb_frag_page(&sinfo->frags[i]); > > + > > + __xdp_return(page_address(page), &xdpf->mem, false); > > + } > > } > > EXPORT_SYMBOL_GPL(xdp_return_frame); > > > > void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) > > { > > + struct skb_shared_info *sinfo; > > + int i; > > + > > __xdp_return(xdpf->data, &xdpf->mem, true); > > Same issue. > > > + if (!xdpf->mb) > > + return; > > + > > + sinfo = xdp_get_shared_info_from_frame(xdpf); > > + for (i = 0; i < sinfo->nr_frags; i++) { > > + struct page *page = skb_frag_page(&sinfo->frags[i]); > > + > > + __xdp_return(page_address(page), &xdpf->mem, true); > > + } > > } > > EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi); > > > > void xdp_return_buff(struct xdp_buff *xdp) > > { > > + struct skb_shared_info *sinfo; > > + int i; > > + > > __xdp_return(xdp->data, &xdp->rxq->mem, true); > > Same issue. > > > + if (!xdp->mb) > > + return; > > + > > + sinfo = xdp_get_shared_info_from_buff(xdp); > > + for (i = 0; i < sinfo->nr_frags; i++) { > > + struct page *page = skb_frag_page(&sinfo->frags[i]); > > + > > + __xdp_return(page_address(page), &xdp->rxq->mem, true); > > + } > > } > > > > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer >
diff --git a/include/net/xdp.h b/include/net/xdp.h index 42f439f9fcda..37c4522fc1bb 100644 --- a/include/net/xdp.h +++ b/include/net/xdp.h @@ -208,10 +208,23 @@ void __xdp_release_frame(void *data, struct xdp_mem_info *mem); static inline void xdp_release_frame(struct xdp_frame *xdpf) { struct xdp_mem_info *mem = &xdpf->mem; + struct skb_shared_info *sinfo; + int i; /* Curr only page_pool needs this */ - if (mem->type == MEM_TYPE_PAGE_POOL) - __xdp_release_frame(xdpf->data, mem); + if (mem->type != MEM_TYPE_PAGE_POOL) + return; + + __xdp_release_frame(xdpf->data, mem); + if (!xdpf->mb) + return; + + sinfo = xdp_get_shared_info_from_frame(xdpf); + for (i = 0; i < sinfo->nr_frags; i++) { + struct page *page = skb_frag_page(&sinfo->frags[i]); + + __xdp_release_frame(page_address(page), mem); + } } int xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq, diff --git a/net/core/xdp.c b/net/core/xdp.c index 884f140fc3be..006b24b5d276 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -370,19 +370,55 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct) void xdp_return_frame(struct xdp_frame *xdpf) { + struct skb_shared_info *sinfo; + int i; + __xdp_return(xdpf->data, &xdpf->mem, false); + if (!xdpf->mb) + return; + + sinfo = xdp_get_shared_info_from_frame(xdpf); + for (i = 0; i < sinfo->nr_frags; i++) { + struct page *page = skb_frag_page(&sinfo->frags[i]); + + __xdp_return(page_address(page), &xdpf->mem, false); + } } EXPORT_SYMBOL_GPL(xdp_return_frame); void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) { + struct skb_shared_info *sinfo; + int i; + __xdp_return(xdpf->data, &xdpf->mem, true); + if (!xdpf->mb) + return; + + sinfo = xdp_get_shared_info_from_frame(xdpf); + for (i = 0; i < sinfo->nr_frags; i++) { + struct page *page = skb_frag_page(&sinfo->frags[i]); + + __xdp_return(page_address(page), &xdpf->mem, true); + } } EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi); void xdp_return_buff(struct xdp_buff *xdp) { + struct skb_shared_info *sinfo; + int i; + __xdp_return(xdp->data, &xdp->rxq->mem, true); + if (!xdp->mb) + return; + + sinfo = xdp_get_shared_info_from_buff(xdp); + for (i = 0; i < sinfo->nr_frags; i++) { + struct page *page = skb_frag_page(&sinfo->frags[i]); + + __xdp_return(page_address(page), &xdp->rxq->mem, true); + } } /* Only called for MEM_TYPE_PAGE_POOL see xdp.h */
Take into account if the received xdp_buff/xdp_frame is non-linear recycling/returning the frame memory to the allocator Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- include/net/xdp.h | 17 +++++++++++++++-- net/core/xdp.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-)