[Intel-wired-lan] [bpf-next, 02/11] xsk: introduce xdp_umem_page

[Jiri Slaby]

On 04. 06. 18, 14:05, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel at 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