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...@intel.com>
Acked-by: Björn Töpel <bjorn.to...@intel.com>
Signed-off-by: Zhu Yanjun <yanjun....@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)
