| 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
| ||
|
Date: Tue, 24 Apr 2018 09:30:30 +0200
From: Björn Töpel <bjorn.topel@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...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
2018-04-24 1:04 GMT+02:00 Willem de Bruijn <willemdebruijn.kernel@...il.com>:
> 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.
>
Yes, you're right! I'll add constraints to the umem setup. See further
down in the reply.
>> +
>> + 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?
>
Hmm, dug around in the code, and it looks like you're correct -- i.e.
if user->locked_vm is used, we shouldn't update the mm->pinned_vm.
I'll need to check a bit more, so that I'm certain, but if so, I'll
remove it in the next revision.
>> +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.
>
Ok. Block signaling is sort of ring batching, so I think we're good
for that case. As for variable sized frames *within* a umem, that's
trickier. To support different sizes, multiple umems (and multiple
queues) -- if that makes sense?
>> + 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.
>
Yup! I'll add those checks. This will make the "diff shift" in the
release code safe as well. Thanks!
>> + 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
Indeed! I'll fix that!
>> + goto out;
>> + return 0;
>> +
>> +out:
>> + put_pid(umem->pid);
>> + return err;
>> +}
Powered by blists - more mailing lists