netdev - Re: [PATCH bpf-next 02/15] xsk: add user memory registration support sockopt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAF=yD-+VKKspFwPCXrX_U9_rVgAXrFkarFXu8mfLsL2=QuLdPg@mail.gmail.com>
Date:   Mon, 23 Apr 2018 19:04:25 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Björn Töpel <bjorn.topel@...il.com>
Cc:     "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Network Development <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        michael.lundkvist@...csson.com,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Singhai, Anjali" <anjali.singhai@...el.com>,
        "Zhang, Qi Z" <qi.z.zhang@...el.com>
Subject: Re: [PATCH bpf-next 02/15] xsk: add user memory registration support sockopt

On Mon, Apr 23, 2018 at 9:56 AM, Björn Töpel <bjorn.topel@...il.com> wrote:
> From: Björn Töpel <bjorn.topel@...el.com>
>
> In this commit the base structure of the AF_XDP address family is set
> up. Further, we introduce the abilty register a window of user memory
> to the kernel via the XDP_UMEM_REG setsockopt syscall. The memory
> window is viewed by an AF_XDP socket as a set of equally large
> frames. After a user memory registration all frames are "owned" by the
> user application, and not the kernel.
>
> Co-authored-by: Magnus Karlsson <magnus.karlsson@...el.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> Signed-off-by: Björn Töpel <bjorn.topel@...el.com>

> +static void xdp_umem_release(struct xdp_umem *umem)
> +{
> +       struct task_struct *task;
> +       struct mm_struct *mm;
> +       unsigned long diff;
> +
> +       if (umem->pgs) {
> +               xdp_umem_unpin_pages(umem);
> +
> +               task = get_pid_task(umem->pid, PIDTYPE_PID);
> +               put_pid(umem->pid);
> +               if (!task)
> +                       goto out;
> +               mm = get_task_mm(task);
> +               put_task_struct(task);
> +               if (!mm)
> +                       goto out;
> +
> +               diff = umem->size >> PAGE_SHIFT;

Need to round up or size must always be a multiple of PAGE_SIZE.

> +
> +               down_write(&mm->mmap_sem);
> +               mm->pinned_vm -= diff;
> +               up_write(&mm->mmap_sem);

When using user->locked_vm for resource limit checks, no need
to also update mm->pinned_vm?

> +static int __xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> +{
> +       u32 frame_size = mr->frame_size, frame_headroom = mr->frame_headroom;
> +       u64 addr = mr->addr, size = mr->len;
> +       unsigned int nframes;
> +       int size_chk, err;
> +
> +       if (frame_size < XDP_UMEM_MIN_FRAME_SIZE || frame_size > PAGE_SIZE) {
> +               /* Strictly speaking we could support this, if:
> +                * - huge pages, or*
> +                * - using an IOMMU, or
> +                * - making sure the memory area is consecutive
> +                * but for now, we simply say "computer says no".
> +                */
> +               return -EINVAL;
> +       }

Ideally, AF_XDP subsumes all packet socket use cases. It does not
have packet v3's small packet optimizations of variable sized frames
and block signaling.

I don't suggest adding that now. But for the non-zerocopy case, it may
make sense to ensure that nothing is blocking a later addition of these
features. Especially for header-only (snaplen) workloads. So far, I don't
see any issues.

> +       if (!is_power_of_2(frame_size))
> +               return -EINVAL;
> +
> +       if (!PAGE_ALIGNED(addr)) {
> +               /* Memory area has to be page size aligned. For
> +                * simplicity, this might change.
> +                */
> +               return -EINVAL;
> +       }
> +
> +       if ((addr + size) < addr)
> +               return -EINVAL;
> +
> +       nframes = size / frame_size;
> +       if (nframes == 0 || nframes > UINT_MAX)
> +               return -EINVAL;

You may also want a check here that nframes * frame_size is at least
PAGE_SIZE and probably a multiple of that.

> +       frame_headroom = ALIGN(frame_headroom, 64);
> +
> +       size_chk = frame_size - frame_headroom - XDP_PACKET_HEADROOM;
> +       if (size_chk < 0)
> +               return -EINVAL;
> +
> +       umem->pid = get_task_pid(current, PIDTYPE_PID);
> +       umem->size = (size_t)size;
> +       umem->address = (unsigned long)addr;
> +       umem->props.frame_size = frame_size;
> +       umem->props.nframes = nframes;
> +       umem->frame_headroom = frame_headroom;
> +       umem->npgs = size / PAGE_SIZE;
> +       umem->pgs = NULL;
> +       umem->user = NULL;
> +
> +       umem->frame_size_log2 = ilog2(frame_size);
> +       umem->nfpp_mask = (PAGE_SIZE / frame_size) - 1;
> +       umem->nfpplog2 = ilog2(PAGE_SIZE / frame_size);
> +       atomic_set(&umem->users, 1);
> +
> +       err = xdp_umem_account_pages(umem);
> +       if (err)
> +               goto out;
> +
> +       err = xdp_umem_pin_pages(umem);
> +       if (err)

need to call xdp_umem_unaccount_pages on error
> +               goto out;
> +       return 0;
> +
> +out:
> +       put_pid(umem->pid);
> +       return err;
> +}