On 04. 06. 18, 14:05, Björn Töpel wrote:
> From: Björn Töpel <bjorn.to...@intel.com>
>
> The xdp_umem_page holds the address for a page. Trade memory for
> faster lookup. Later, we'll add DMA address here as well.
...
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
...
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -65,6 +65,9 @@ static void xdp_umem_release(struct xdp_umem *umem)
> goto out;
>
> mmput(mm);
> + kfree(umem->pages);
> + umem->pages = NULL;
> +
Are you sure about the placement of kfree here? Why is it dependent on
task && mm above?
IMO the kfree should be below "out:":
> xdp_umem_unaccount_pages(umem);
> out:
> kfree(umem);
Syzkaller reported this memleak:
r0 = socket$xdp(0x2c, 0x3, 0x0)
setsockopt$XDP_UMEM_REG(r0, 0x11b, 0x4,
&(0x7f0000000100)={&(0x7f0000000000)=""/210, 0x20000, 0x1000, 0x7}, 0x18)
BUG: memory leak
unreferenced object 0xffff88003648de68 (size 512):
comm "syz-executor.3", pid 11688, jiffies 4295555546 (age 15.752s)
hex dump (first 32 bytes):
00 00 40 23 00 88 ff ff 00 00 00 00 00 00 00 00 ..@#............
00 10 40 23 00 88 ff ff 00 00 00 00 00 00 00 00 ..@#............
backtrace:
[<ffffffffa9f8346c>] xsk_setsockopt+0x40c/0x510 net/xdp/xsk.c:539
[<ffffffffa9935c41>] SyS_setsockopt+0x171/0x370 net/socket.c:1862
[<ffffffffa800b28c>] do_syscall_64+0x26c/0x6e0
arch/x86/entry/common.c:284
[<ffffffffaa00009a>] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[<ffffffffffffffff>] 0xffffffffffffffff
Given the size of the leak, it looks like umem->pages is leaked:
mr->len/page_size*sizeof(*umem->pages)
0x20000/4096*16=512
So I added a check, and really, task is NULL in my testcase -- the
program is gone when the deferred work triggers. But umem->pages is not
freed.
Moving the free after "out:", no leaks happen anymore.
Easily reproducible with:
#include <err.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <linux/if_xdp.h>
void fun()
{
static char buffer[0x20000] __attribute__((aligned(4096)));
struct xdp_umem_reg mr = {
(unsigned long)buffer,
0x20000,
0x1000,
0x7,
//&(0x7f0000000100)={&(0x7f0000000000)=""/210, 0x20000, 0x1000,
0x7}
};
int r0;
r0 = socket(AF_XDP, SOCK_RAW, 0);
if (r0 < 0)
err(1, "socket");
if (setsockopt(r0, SOL_XDP, XDP_UMEM_REG, &mr, sizeof(mr)) < 0)
err(1, "setsockopt");
close(r0);
}
int main()
{
int a;
while (1) {
for (a = 0; a < 40; a++)
if (!fork()) {
fun();
exit(0);
}
for (a = 0; a < 100; a++)
wait(NULL);
}
}
thanks,
--
js
suse labs
