| Message ID | 20201211042610.71081-1-yanjun.zhu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/1] xdp: avoid calling kfree twice | expand |
On Fri, Dec 11, 2020 at 11:53 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 12/11/20 5:26 AM, Zhu Yanjun wrote: > > In the function xdp_umem_pin_pages, if npgs != umem->npgs and > > npgs >= 0, the function xdp_umem_unpin_pages is called. In this > > function, kfree is called to handle umem->pgs, and then in the > > function xdp_umem_pin_pages, kfree is called again to handle > > umem->pgs. Eventually, to umem->pgs, kfree is called twice. > > > > Since umem->pgs is set to NULL after the first kfree, the second > > kfree would not trigger call trace. > > This can still be misinterpreted imho; maybe lets simplify, for example: > > [bpf-next] xdp: avoid unnecessary second call to kfree > > For the case when in xdp_umem_pin_pages() the call to pin_user_pages() > wasn't able to pin all the requested number of pages in memory (but some) > then we error out by cleaning up the ones that got pinned through a call > to xdp_umem_unpin_pages() and later on we free kfree(umem->pgs) itself. > > This is unneeded since xdp_umem_unpin_pages() itself already does the > kfree(umem->pgs) internally with subsequent setting umem->pgs to NULL, so > that in xdp_umem_pin_pages() the second kfree(umem->pgs) becomes entirely > unnecessary for this case. Therefore, clean the error handling up. > > > Fixes: c0c77d8fb787 ("xsk: add user memory registration support sockopt") > > Why do we need a Fixes tag here? It's a _cleanup_, not a bug/fix technically. > > > CC: Ye Dong <dong.ye@intel.com> > > Acked-by: Björn Töpel <bjorn.topel@intel.com> > > Signed-off-by: Zhu Yanjun <yanjun.zhu@intel.com> > > --- > > net/xdp/xdp_umem.c | 17 +++++------------ > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c > > index 56a28a686988..01b31c56cead 100644 > > --- a/net/xdp/xdp_umem.c > > +++ b/net/xdp/xdp_umem.c > > @@ -97,7 +97,6 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address) > > { > > unsigned int gup_flags = FOLL_WRITE; > > long npgs; > > - int err; > > > > umem->pgs = kcalloc(umem->npgs, sizeof(*umem->pgs), > > GFP_KERNEL | __GFP_NOWARN); > > @@ -112,20 +111,14 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address) > > if (npgs != umem->npgs) { > > if (npgs >= 0) { > > umem->npgs = npgs; > > - err = -ENOMEM; > > - goto out_pin; > > + xdp_umem_unpin_pages(umem); > > + return -ENOMEM; > > } > > - err = npgs; > > - goto out_pgs; > > + kfree(umem->pgs); > > + umem->pgs = NULL; > > + return (int)npgs; > > } > > return 0; > > - > > -out_pin: > > - xdp_umem_unpin_pages(umem); > > -out_pgs: > > - kfree(umem->pgs); > > - umem->pgs = NULL; > > - return err; > > } > > > > static int xdp_umem_account_pages(struct xdp_umem *umem) > > > > While at it, maybe we could also simplify the if (npgs != umem->npgs) a bit to > get rid of the indent, something like: The original patch is just to make some cleanups. Please do not make it so complicated. If you like it, please file an official patch for code review. > > diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c > index 56a28a686988..fa4dd16cced5 100644 > --- a/net/xdp/xdp_umem.c > +++ b/net/xdp/xdp_umem.c > @@ -97,7 +97,6 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address) > { > unsigned int gup_flags = FOLL_WRITE; > long npgs; > - int err; > > umem->pgs = kcalloc(umem->npgs, sizeof(*umem->pgs), > GFP_KERNEL | __GFP_NOWARN); > @@ -108,24 +107,17 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address) > npgs = pin_user_pages(address, umem->npgs, > gup_flags | FOLL_LONGTERM, &umem->pgs[0], NULL); > mmap_read_unlock(current->mm); > - > - if (npgs != umem->npgs) { > - if (npgs >= 0) { > - umem->npgs = npgs; > - err = -ENOMEM; > - goto out_pin; > - } > - err = npgs; > - goto out_pgs; > + if (npgs == umem->npgs) > + return 0; > + if (npgs >= 0) { > + umem->npgs = npgs; > + xdp_umem_unpin_pages(umem); > + return -ENOMEM; > } > - return 0; > > -out_pin: > - xdp_umem_unpin_pages(umem); > -out_pgs: > kfree(umem->pgs); > umem->pgs = NULL; > - return err; > + return npgs; > } > > static int xdp_umem_account_pages(struct xdp_umem *umem)
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c index 56a28a686988..01b31c56cead 100644 --- a/net/xdp/xdp_umem.c +++ b/net/xdp/xdp_umem.c @@ -97,7 +97,6 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address) { unsigned int gup_flags = FOLL_WRITE; long npgs; - int err; umem->pgs = kcalloc(umem->npgs, sizeof(*umem->pgs), GFP_KERNEL | __GFP_NOWARN); @@ -112,20 +111,14 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address) if (npgs != umem->npgs) { if (npgs >= 0) { umem->npgs = npgs; - err = -ENOMEM; - goto out_pin; + xdp_umem_unpin_pages(umem); + return -ENOMEM; } - err = npgs; - goto out_pgs; + kfree(umem->pgs); + umem->pgs = NULL; + return (int)npgs; } return 0; - -out_pin: - xdp_umem_unpin_pages(umem); -out_pgs: - kfree(umem->pgs); - umem->pgs = NULL; - return err; } static int xdp_umem_account_pages(struct xdp_umem *umem)
In the function xdp_umem_pin_pages, if npgs != umem->npgs and npgs >= 0, the function xdp_umem_unpin_pages is called. In this function, kfree is called to handle umem->pgs, and then in the function xdp_umem_pin_pages, kfree is called again to handle umem->pgs. Eventually, to umem->pgs, kfree is called twice. Since umem->pgs is set to NULL after the first kfree, the second kfree would not trigger call trace. Fixes: c0c77d8fb787 ("xsk: add user memory registration support sockopt") CC: Ye Dong <dong.ye@intel.com> Acked-by: Björn Töpel <bjorn.topel@intel.com> Signed-off-by: Zhu Yanjun <yanjun.zhu@intel.com> --- net/xdp/xdp_umem.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)