netdev - Re: [PATCH bpf-next v2 0/5] xsk: fix bug when trying to use both copy and zero-copy mode

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] [day] [month] [year] [list]

Message-ID: <d17d6233-4b26-09c6-50bd-2a174b1c7d11@iogearbox.net>
Date:   Fri, 5 Oct 2018 09:35:38 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Magnus Karlsson <magnus.karlsson@...el.com>, bjorn.topel@...el.com,
        ast@...nel.org, netdev@...r.kernel.org,
        jakub.kicinski@...ronome.com
Subject: Re: [PATCH bpf-next v2 0/5] xsk: fix bug when trying to use both copy
 and zero-copy mode

On 10/01/2018 02:51 PM, Magnus Karlsson wrote:
> Previously, the xsk code did not record which umem was bound to a
> specific queue id. This was not required if all drivers were zero-copy
> enabled as this had to be recorded in the driver anyway. So if a user
> tried to bind two umems to the same queue, the driver would say
> no. But if copy-mode was first enabled and then zero-copy mode (or the
> reverse order), we mistakenly enabled both of them on the same umem
> leading to buggy behavior. The main culprit for this is that we did
> not store the association of umem to queue id in the copy case and
> only relied on the driver reporting this. As this relation was not
> stored in the driver for copy mode (it does not rely on the AF_XDP
> NDOs), this obviously could not work.
> 
> This patch fixes the problem by always recording the umem to queue id
> relationship in the netdev_queue and netdev_rx_queue structs. This way
> we always know what kind of umem has been bound to a queue id and can
> act appropriately at bind time. To make the bind semantics consistent
> with ethtool queue manipulations and to facilitate the implementation
> of drivers, we also forbid decreasing the number of queues/channels
> with ethtool if there is an active AF_XDP socket in the set of queues
> that are disabled.
> 
> Jakub, please take a look at your patches. The last one I had to
> change slightly to make it fit with the new interface
> xdp_get_umem_from_qid(). An added bonus with this function is that we,
> in the future, can also use it from the driver to get a umem, thus
> simplifying driver implementations (and later remove the umem from the
> NDO completely). Björn will mail patches, at a later point in time,
> using this in the i40e and ixgbe drivers, that removes a good chunk of
> code from the ZC implementations. I also made your code aware of Tx
> queues. If we create a socket that only has a Tx queue, then the queue
> id will refer to a Tx queue id only and could be larger than the
> available amount of Rx queues. Please take a look at it.
> 
> Differences against v1:
> * Included patches from Jakub that forbids decreasing the number of active
>   queues if a queue to be deactivated has an AF_XDP socket. These have
>   been adapted somewhat to the new interfaces in patch 2.
> * Removed redundant check against real_num_[rt]x_queue in xsk_bind
> * Only need to test against real_num_[rt]x_queues in
>   xdp_clear_umem_at_qid.
> 
> Patch 1: Introduces a umem reference in the netdev_rx_queue and
>          netdev_queue structs.
> Patch 2: Records which queue_id is bound to which umem and make sure
>          that you cannot bind two different umems to the same queue_id.
> Patch 3: Pre patch to ethtool_set_channels.
> Patch 4: Forbid decreasing the number of active queues if a deactivated
>          queue has an AF_XDP socket.
> Patch 5: Simplify xdp_clear_umem_at_qid now when ethtool cannot deactivate
>          the queue id we are running on.
> 
> I based this patch set on bpf-next commit 5bf7a60b8e70 ("bpf: permit
> CGROUP_DEVICE programs accessing helper bpf_get_current_cgroup_id()")
> 
> Thanks: Magnus
> 
> Jakub Kicinski (2):
>   ethtool: rename local variable max -> curr
>   ethtool: don't allow disabling queues with umem installed
> 
> Magnus Karlsson (3):
>   net: add umem reference in netdev{_rx}_queue
>   xsk: fix bug when trying to use both copy and zero-copy on one queue
>     id
>   xsk: simplify xdp_clear_umem_at_qid implementation
> 
>  include/linux/netdevice.h |  6 ++++
>  include/net/xdp_sock.h    |  7 ++++
>  net/core/ethtool.c        | 23 +++++++++----
>  net/xdp/xdp_umem.c        | 87 ++++++++++++++++++++++++++++++++---------------
>  net/xdp/xdp_umem.h        |  2 +-
>  net/xdp/xsk.c             |  7 ----
>  6 files changed, 91 insertions(+), 41 deletions(-)
> 
> --
> 2.7.4
> 

Applied to bpf-next, thanks everyone!