Message ID | 20201211042610.71081-1-yanjun.zhu@intel.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | [v3,1/1] xdp: avoid calling kfree twice | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Link |
netdev/fixes_present | success | Link |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Guessed tree name to be net-next |
netdev/subject_prefix | warning | Target tree name not specified in the subject |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Link |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Link |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
netdev/header_inline | success | Link |
netdev/stable | success | Stable not CCed |
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: 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)
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)