| Message ID | 20201209050315.5864-1-yanjun.zhu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] xdp: avoid calling kfree twice | expand |
On 12/9/20 6:03 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, umem->pgs is freed twice. > > Acked-by: Björn Töpel <bjorn.topel@intel.com> > Signed-off-by: Zhu Yanjun <yanjun.zhu@intel.com> Please also fix up the commit log according to Bjorn's prior feedback [0]. If it's just a cleanup, it should state so, the commit message right now makes it sound like an actual double free bug. [0] https://lore.kernel.org/netdev/0fef898d-cf5e-ef1b-6c35-c98669e9e0ed@intel.com/
On Wed, Dec 9, 2020 at 1:12 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 12/9/20 6:03 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, umem->pgs is freed twice. > > > > Acked-by: Björn Töpel <bjorn.topel@intel.com> > > Signed-off-by: Zhu Yanjun <yanjun.zhu@intel.com> > > Please also fix up the commit log according to Bjorn's prior feedback [0]. > If it's just a cleanup, it should state so, the commit message right now > makes it sound like an actual double free bug. The umem->pgs is actually freed twice. Since umem->pgs is set to NULL after the first kfree, the second kfree would not trigger call trace. IMO, the commit log is very clear about this. Zhu Yanjun > > [0] https://lore.kernel.org/netdev/0fef898d-cf5e-ef1b-6c35-c98669e9e0ed@intel.com/
Zhu Yanjun <zyjzyj2000@gmail.com> writes: > On Wed, Dec 9, 2020 at 1:12 AM Daniel Borkmann <daniel@iogearbox.net> wrote: >> >> On 12/9/20 6:03 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, umem->pgs is freed twice. >> > >> > Acked-by: Björn Töpel <bjorn.topel@intel.com> >> > Signed-off-by: Zhu Yanjun <yanjun.zhu@intel.com> >> >> Please also fix up the commit log according to Bjorn's prior feedback [0]. >> If it's just a cleanup, it should state so, the commit message right now >> makes it sound like an actual double free bug. > > The umem->pgs is actually freed twice. Since umem->pgs is set to NULL > after the first kfree, > the second kfree would not trigger call trace. > IMO, the commit log is very clear about this. Yes, it is very clear; and also wrong. As someone already pointed out, passing a NULL pointer to kfree() doesn't actually lead to a double free: https://elixir.bootlin.com/linux/latest/source/mm/slub.c#L4106 -Toke
On Wed, Dec 9, 2020 at 6:44 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Zhu Yanjun <zyjzyj2000@gmail.com> writes: > > > On Wed, Dec 9, 2020 at 1:12 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> > >> On 12/9/20 6:03 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, umem->pgs is freed twice. > >> > > >> > Acked-by: Björn Töpel <bjorn.topel@intel.com> > >> > Signed-off-by: Zhu Yanjun <yanjun.zhu@intel.com> > >> > >> Please also fix up the commit log according to Bjorn's prior feedback [0]. > >> If it's just a cleanup, it should state so, the commit message right now > >> makes it sound like an actual double free bug. > > > > The umem->pgs is actually freed twice. Since umem->pgs is set to NULL > > after the first kfree, > > the second kfree would not trigger call trace. > > IMO, the commit log is very clear about this. > > Yes, it is very clear; and also wrong. As someone already pointed out, > passing a NULL pointer to kfree() doesn't actually lead to a double > free: In your commit, does "double free" mean the call trace bug? If so, I will correct it in my commit log. In my commit log, I just mean that kfree is called twice. And the second kfree is meaningless. And since NULL is passed to it, this will not trigger "double free" CallTrace. I will send the latest version soon. Zhu Yanjun > > https://elixir.bootlin.com/linux/latest/source/mm/slub.c#L4106 > > -Toke >
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)