* Re: [RFC v4 0/3] vhost: introduce mdev based hardware backend
From: Tiwei Bie @ 2019-09-20 2:16 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, alex.williamson, maxime.coquelin,
linux-kernel, kvm, virtualization, netdev, dan.daly,
cunming.liang, zhihong.wang, lingshan.zhu
In-Reply-To: <43aaf7dc-f08b-8898-3c55-908ff4d68866@redhat.com>
On Fri, Sep 20, 2019 at 09:30:58AM +0800, Jason Wang wrote:
> On 2019/9/19 下午11:45, Tiwei Bie wrote:
> > On Thu, Sep 19, 2019 at 09:08:11PM +0800, Jason Wang wrote:
> > > On 2019/9/18 下午10:32, Michael S. Tsirkin wrote:
> > > > > > > So I have some questions:
> > > > > > >
> > > > > > > 1) Compared to method 2, what's the advantage of creating a new vhost char
> > > > > > > device? I guess it's for keep the API compatibility?
> > > > > > One benefit is that we can avoid doing vhost ioctls on
> > > > > > VFIO device fd.
> > > > > Yes, but any benefit from doing this?
> > > > It does seem a bit more modular, but it's certainly not a big deal.
> > > Ok, if we go this way, it could be as simple as provide some callback to
> > > vhost, then vhost can just forward the ioctl through parent_ops.
> > >
> > > > > > > 2) For method 2, is there any easy way for user/admin to distinguish e.g
> > > > > > > ordinary vfio-mdev for vhost from ordinary vfio-mdev?
> > > > > > I think device-api could be a choice.
> > > > > Ok.
> > > > >
> > > > >
> > > > > > > I saw you introduce
> > > > > > > ops matching helper but it's not friendly to management.
> > > > > > The ops matching helper is just to check whether a given
> > > > > > vfio-device is based on a mdev device.
> > > > > >
> > > > > > > 3) A drawback of 1) and 2) is that it must follow vfio_device_ops that
> > > > > > > assumes the parameter comes from userspace, it prevents support kernel
> > > > > > > virtio drivers.
> > > > > > >
> > > > > > > 4) So comes the idea of method 3, since it register a new vhost-mdev driver,
> > > > > > > we can use device specific ops instead of VFIO ones, then we can have a
> > > > > > > common API between vDPA parent and vhost-mdev/virtio-mdev drivers.
> > > > > > As the above draft shows, this requires introducing a new
> > > > > > VFIO device driver. I think Alex's opinion matters here.
> > > Just to clarify, a new type of mdev driver but provides dummy
> > > vfio_device_ops for VFIO to make container DMA ioctl work.
> > I see. Thanks! IIUC, you mean we can provide a very tiny
> > VFIO device driver in drivers/vhost/mdev.c, e.g.:
> >
> > static int vfio_vhost_mdev_open(void *device_data)
> > {
> > if (!try_module_get(THIS_MODULE))
> > return -ENODEV;
> > return 0;
> > }
> >
> > static void vfio_vhost_mdev_release(void *device_data)
> > {
> > module_put(THIS_MODULE);
> > }
> >
> > static const struct vfio_device_ops vfio_vhost_mdev_dev_ops = {
> > .name = "vfio-vhost-mdev",
> > .open = vfio_vhost_mdev_open,
> > .release = vfio_vhost_mdev_release,
> > };
> >
> > static int vhost_mdev_probe(struct device *dev)
> > {
> > struct mdev_device *mdev = to_mdev_device(dev);
> >
> > ... Check the mdev device_id proposed in ...
> > ... https://lkml.org/lkml/2019/9/12/151 ...
>
>
> To clarify, this should be done through the id_table fields in
> vhost_mdev_driver, and it should claim it supports virtio-mdev device only:
>
>
> static struct mdev_class_id id_table[] = {
> { MDEV_ID_VIRTIO },
> { 0 },
> };
>
>
> static struct mdev_driver vhost_mdev_driver = {
> ...
> .id_table = id_table,
> }
In this way, both of virtio-mdev and vhost-mdev will try to
take this device. We may want a way to let vhost-mdev take this
device only when users explicitly ask it to do it. Or maybe we
can have a different MDEV_ID for vhost-mdev but share the device
ops with virtio-mdev.
>
>
> >
> > return vfio_add_group_dev(dev, &vfio_vhost_mdev_dev_ops, mdev);
>
>
> And in vfio_vhost_mdev_ops, all its need is to just implement vhost-net
> ioctl and translate them to virtio-mdev transport (e.g device_ops I proposed
> or ioctls other whatever other method) API.
I see, so my previous understanding is basically correct:
https://lkml.org/lkml/2019/9/17/332
I.e. we won't have a separate vhost fd and we will do all vhost
ioctls on the VFIO device fd backed by this new VFIO driver.
> And it could have a dummy ops
> implementation for the other device_ops.
>
>
> > }
> >
> > static void vhost_mdev_remove(struct device *dev)
> > {
> > vfio_del_group_dev(dev);
> > }
> >
> > static struct mdev_driver vhost_mdev_driver = {
> > .name = "vhost_mdev",
> > .probe = vhost_mdev_probe,
> > .remove = vhost_mdev_remove,
> > };
> >
> > So we can bind above mdev driver to the virtio-mdev compatible
> > mdev devices when we want to use vhost-mdev.
> >
> > After binding above driver to the mdev device, we can setup IOMMU
> > via VFIO and get VFIO device fd of this mdev device, and pass it
> > to vhost fd (/dev/vhost-mdev) with a SET_BACKEND ioctl.
>
>
> Then what vhost-mdev char device did is just forwarding ioctl back to this
> vfio device fd which seems a overkill. It's simpler that just do ioctl on
> the device ops directly.
Yes.
Thanks,
Tiwei
>
> Thanks
>
>
> >
> > Thanks,
> > Tiwei
> >
> > > Thanks
> > >
> > >
> > > > > Yes, it is.
> > > > >
> > > > > Thanks
> > > > >
> > > > >
^ permalink raw reply
* [PATCHv2 iproute2 master] bpf: Fix race condition with map pinning
From: Joe Stringer @ 2019-09-20 2:04 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, daniel
If two processes attempt to invoke bpf_map_attach() at the same time,
then they will both create maps, then the first will successfully pin
the map to the filesystem and the second will not pin the map, but will
continue operating with a reference to its own copy of the map. As a
result, the sharing of the same map will be broken from the two programs
that were concurrently loaded via loaders using this library.
Fix this by adding a retry in the case where the pinning fails because
the map already exists on the filesystem. In that case, re-attempt
opening a fd to the map on the filesystem as it shows that another
program already created and pinned a map at that location.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
v2: Fix close of created map in the EEXIST case.
v1: Original patch
---
lib/bpf.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/bpf.c b/lib/bpf.c
index 01152b26e54a..86ab0698660f 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -1707,7 +1707,9 @@ static int bpf_map_attach(const char *name, struct bpf_elf_ctx *ctx,
int *have_map_in_map)
{
int fd, ifindex, ret, map_inner_fd = 0;
+ bool retried = false;
+probe:
fd = bpf_probe_pinned(name, ctx, map->pinning);
if (fd > 0) {
ret = bpf_map_selfcheck_pinned(fd, map, ext,
@@ -1756,10 +1758,14 @@ static int bpf_map_attach(const char *name, struct bpf_elf_ctx *ctx,
}
ret = bpf_place_pinned(fd, name, ctx, map->pinning);
- if (ret < 0 && errno != EEXIST) {
+ if (ret < 0) {
+ close(fd);
+ if (!retried && errno == EEXIST) {
+ retried = true;
+ goto probe;
+ }
fprintf(stderr, "Could not pin %s map: %s\n", name,
strerror(errno));
- close(fd);
return ret;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH iproute2 master] bpf: Fix race condition with map pinning
From: Joe Stringer @ 2019-09-20 1:56 UTC (permalink / raw)
To: Joe Stringer; +Cc: Stephen Hemminger, netdev, Daniel Borkmann
In-Reply-To: <20190919220733.31206-1-joe@wand.net.nz>
On Thu, Sep 19, 2019 at 3:07 PM Joe Stringer <joe@wand.net.nz> wrote:
>
> If two processes attempt to invoke bpf_map_attach() at the same time,
> then they will both create maps, then the first will successfully pin
> the map to the filesystem and the second will not pin the map, but will
> continue operating with a reference to its own copy of the map. As a
> result, the sharing of the same map will be broken from the two programs
> that were concurrently loaded via loaders using this library.
>
> Fix this by adding a retry in the case where the pinning fails because
> the map already exists on the filesystem. In that case, re-attempt
> opening a fd to the map on the filesystem as it shows that another
> program already created and pinned a map at that location.
>
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
> ---
> lib/bpf.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index f64b58c3bb19..23eb8952cc28 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -1625,7 +1625,9 @@ static int bpf_map_attach(const char *name, struct bpf_elf_ctx *ctx,
> int *have_map_in_map)
> {
> int fd, ifindex, ret, map_inner_fd = 0;
> + bool retried = false;
>
> +probe:
> fd = bpf_probe_pinned(name, ctx, map->pinning);
> if (fd > 0) {
> ret = bpf_map_selfcheck_pinned(fd, map, ext,
> @@ -1674,7 +1676,11 @@ static int bpf_map_attach(const char *name, struct bpf_elf_ctx *ctx,
> }
>
> ret = bpf_place_pinned(fd, name, ctx, map->pinning);
> - if (ret < 0 && errno != EEXIST) {
> + if (ret < 0) {
> + if (!retried && errno == EEXIST) {
> + retried = true;
> + goto probe;
> + }
Ah, forgot to close 'fd' before the jump in this retry case. Will fix
that up in v2.
^ permalink raw reply
* Re: [RFC {net,iproute2}-next 0/2] Introduce an eBPF hookpoint for tx queue selection in the XPS (Transmit Packet Steering) code.
From: Jason Wang @ 2019-09-20 1:42 UTC (permalink / raw)
To: Matt Cover, davem, ast, daniel, kafai, songliubraving, yhs,
nikolay, sd, sbrivio, vincent, kda, Matthew Cover, jiri,
Eric Dumazet, pabeni, idosch, petrm, f.fainelli, stephen, dsahern,
christian, jakub.kicinski, Roopa Prabhu, johannes.berg, mkubecek,
netdev, linux-kernel, bpf
In-Reply-To: <CAGyo_hrkUPxBAOttpo+xqcUHr89i2N5BOV4_rYFh=dRPkYVPGA@mail.gmail.com>
On 2019/9/20 上午8:05, Matt Cover wrote:
> On Thu, Sep 19, 2019 at 3:45 PM Matthew Cover <werekraken@gmail.com> wrote:
>> WORK IN PROGRESS:
>> * bpf program loading works!
>> * txq steering via bpf program return code works!
>> * bpf program unloading not working.
>> * bpf program attached query not working.
>>
>> This patch set provides a bpf hookpoint with goals similar to, but a more
>> generic implementation than, TUNSETSTEERINGEBPF; userspace supplied tx queue
>> selection policy.
One point that I introduce TUNSETSTEERINGEBPF instead of using a generic
way like cls/act bpf is that I need make sure to have a consistent API
with macvtap.
In the case of macvtap, TX means transmit from userspace to kernel, but
for TUN, it means transmit from kernel to userspace.
>>
>> TUNSETSTEERINGEBPF is a useful bpf hookpoint, but has some drawbacks.
>>
>> First, it only works on tun/tap devices.
>>
>> Second, there is no way in the current TUNSETSTEERINGEBPF implementation
>> to bail out or load a noop bpf prog and fallback to the no prog tx queue
>> selection method.
I believe it expect that eBPF should take all the parts (even the
fallback part).
>>
>> Third, the TUNSETSTEERINGEBPF interface seems to require possession of existing
>> or creation of new queues/fds.
That's the way TUN work for past +10 years because ioctl is the only way
to do configuration and it requires a fd to carry that. David suggest to
implement netlink but nobody did that.
>>
>> This most naturally fits in the "wire" implementation since possession of fds
>> is ensured. However, it also means the various "wire" implementations (e.g.
>> qemu) have to all be made aware of TUNSETSTEERINGEBPF and expose an interface
>> to load/unload a bpf prog (or provide a mechanism to pass an fd to another
>> program).
The load/unload of ebpf program is standard bpf() syscall. Ioctl just
attach that to TUN. This idea is borrowed from packet socket which the
bpf program was attached through setsockopt().
>>
>> Alternatively, you can spin up an extra queue and immediately disable via
>> IFF_DETACH_QUEUE, but this seems unsafe; packets could be enqueued to this
>> extra file descriptor which is part of our bpf prog loader, not our "wire".
You can use you 'wire' queue to do ioctl, but we can invent other API.
>>
>> Placing this in the XPS code and leveraging iproute2 and rtnetlink to provide
>> our bpf prog loader in a similar manner to xdp gives us a nice way to separate
>> the tap "wire" and the loading of tx queue selection policy. It also lets us
>> use this hookpoint for any device traversing XPS.
>>
>> This patch only introduces the new hookpoint to the XPS code and will not yet
>> be used by tun/tap devices using the intree tun.ko (which implements an
>> .ndo_select_queue and does not traverse the XPS code).
>>
>> In a future patch set, we can optionally refactor tun.ko to traverse this call
>> to bpf_prog_run_clear_cb() and bpf prog storage. tun/tap devices could then
>> leverage iproute2 as a generic loader. The TUNSETSTEERINGEBPF interface could
>> at this point be optionally deprecated/removed.
As described above, we need it for macvtap and you propose here can not
work for that.
I'm not against this proposal, just want to clarify some considerations
when developing TUNSETSTEERINGEPF. The main goal is for VM to implement
sophisticated steering policy like RSS without touching kernel.
Thanks
>>
>> Both patches in this set have been tested using a rebuilt tun.ko with no
>> .ndo_select_queue.
>>
>> sed -i '/\.ndo_select_queue.*=/d' drivers/net/tun.c
>>
>> The tap device was instantiated using tap_mq_pong.c, supporting scripts, and
>> wrapping service found here:
>>
>> https://github.com/stackpath/rxtxcpu/tree/v1.2.6/helpers
>>
>> The bpf prog source and test scripts can be found here:
>>
>> https://github.com/werekraken/xps_ebpf
>>
>> In nstxq, netsniff-ng using PACKET_FANOUT_QM is leveraged to check the
>> queue_mapping.
>>
>> With no prog loaded, the tx queue selection is adhering our xps_cpus
>> configuration.
>>
>> [vagrant@localhost ~]$ grep . /sys/class/net/tap0/queues/tx-*/xps_cpus; ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe;
>> /sys/class/net/tap0/queues/tx-0/xps_cpus:1
>> /sys/class/net/tap0/queues/tx-1/xps_cpus:2
>> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.146 ms
>> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.121 ms
>> cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>>
>> With a return 0 bpg prog, our tx queue is 0 (despite xps_cpus).
>>
>> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello0.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
>> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.160 ms
>> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.124 ms
>> cpu1: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> ping-4852 [000] .... 2691.633260: 0: xps (RET 0): Hello, World!
>> ping-4869 [001] .... 2695.753588: 0: xps (RET 0): Hello, World!
>>
>> With a return 1 bpg prog, our tx queue is 1.
>>
>> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello1.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
>> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.193 ms
>> cpu0: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.135 ms
>> cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> ping-4894 [000] .... 2710.652080: 0: xps (RET 1): Hello, World!
>> ping-4911 [001] .... 2714.774608: 0: xps (RET 1): Hello, World!
>>
>> With a return 2 bpg prog, our tx queue is 0 (we only have 2 tx queues).
>>
>> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello2.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
>> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=1.20 ms
>> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.986 ms
>> cpu1: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> ping-4936 [000] .... 2729.442668: 0: xps (RET 2): Hello, World!
>> ping-4953 [001] .... 2733.614558: 0: xps (RET 2): Hello, World!
>>
>> With a return -1 bpf prog, our tx queue selection is once again determined by
>> xps_cpus. Any negative return should work the same and provides a nice
>> mechanism to bail out or have a noop bpf prog at this hookpoint.
>>
>> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello_neg1.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
>> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.628 ms
>> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.322 ms
>> cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>> ping-4981 [000] .... 2763.510760: 0: xps (RET -1): Hello, World!
>> ping-4998 [001] .... 2767.632583: 0: xps (RET -1): Hello, World!
>>
>> bpf prog unloading is not yet working and neither does `ip link show` report
>> when an "xps" bpf prog is attached. This is my first time touching iproute2 or
>> rtnetlink, so it may be something obvious to those more familiar.
> Adding Jason... sorry for missing that the first time.
^ permalink raw reply
* [PATCH] ath10k: fix memory leak
From: Navid Emamdoost @ 2019-09-20 1:36 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Kalle Valo,
David S. Miller, ath10k, linux-wireless, netdev, linux-kernel
In ath10k_usb_hif_tx_sg the allocated urb should be released if
usb_submit_urb fails.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/net/wireless/ath/ath10k/usb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
index e1420f67f776..730ed22e08a0 100644
--- a/drivers/net/wireless/ath/ath10k/usb.c
+++ b/drivers/net/wireless/ath/ath10k/usb.c
@@ -435,6 +435,7 @@ static int ath10k_usb_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
ath10k_dbg(ar, ATH10K_DBG_USB_BULK,
"usb bulk transmit failed: %d\n", ret);
usb_unanchor_urb(urb);
+ usb_free_urb(urb);
ret = -EINVAL;
goto err_free_urb_to_pipe;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] selftests: Add test cases for `ip nexthop flush proto XX`
From: Jakub Kicinski @ 2019-09-20 1:36 UTC (permalink / raw)
To: Donald Sharp; +Cc: netdev, dsahern
In-Reply-To: <20190916122650.24124-1-sharpd@cumulusnetworks.com>
On Mon, 16 Sep 2019 08:26:50 -0400, Donald Sharp wrote:
> Add some test cases to allow the fib_nexthops.sh test code
> to test the flushing of nexthops based upon the proto passed
> in upon creation of the nexthop group.
>
> Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH] net/ncsi: Disable global multicast filter
From: Jakub Kicinski @ 2019-09-20 1:32 UTC (permalink / raw)
To: Vijay Khemka
Cc: Samuel Mendoza-Jonas, David S. Miller, netdev, linux-kernel,
openbmc @ lists . ozlabs . org, joel, linux-aspeed, sdasari
In-Reply-To: <20190912190451.2362220-1-vijaykhemka@fb.com>
On Thu, 12 Sep 2019 12:04:50 -0700, Vijay Khemka wrote:
> Disabling multicast filtering from NCSI if it is supported. As it
> should not filter any multicast packets. In current code, multicast
> filter is enabled and with an exception of optional field supported
> by device are disabled filtering.
>
> Mainly I see if goal is to disable filtering for IPV6 packets then let
> it disabled for every other types as well. As we are seeing issues with
> LLDP not working with this enabled filtering. And there are other issues
> with IPV6.
>
> By Disabling this multicast completely, it is working for both IPV6 as
> well as LLDP.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> @@ -1033,23 +1030,23 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> } else if (nd->state == ncsi_dev_state_config_ebf) {
> nca.type = NCSI_PKT_CMD_EBF;
> nca.dwords[0] = nc->caps[NCSI_CAP_BC].cap;
> - if (ncsi_channel_is_tx(ndp, nc))
> + /* if multicast global filtering is supported then
> + * disable it so that all multicast packet will be
> + * forwarded to management controller
> + */
> + if (nc->caps[NCSI_CAP_GENERIC].cap &
> + NCSI_CAP_GENERIC_MC)
Applied, looks like an unnecessary space sneaked in here, I removed it.
^ permalink raw reply
* Re: [RFC v4 0/3] vhost: introduce mdev based hardware backend
From: Jason Wang @ 2019-09-20 1:30 UTC (permalink / raw)
To: Tiwei Bie
Cc: Michael S. Tsirkin, alex.williamson, maxime.coquelin,
linux-kernel, kvm, virtualization, netdev, dan.daly,
cunming.liang, zhihong.wang, lingshan.zhu
In-Reply-To: <20190919154552.GA27657@___>
On 2019/9/19 下午11:45, Tiwei Bie wrote:
> On Thu, Sep 19, 2019 at 09:08:11PM +0800, Jason Wang wrote:
>> On 2019/9/18 下午10:32, Michael S. Tsirkin wrote:
>>>>>> So I have some questions:
>>>>>>
>>>>>> 1) Compared to method 2, what's the advantage of creating a new vhost char
>>>>>> device? I guess it's for keep the API compatibility?
>>>>> One benefit is that we can avoid doing vhost ioctls on
>>>>> VFIO device fd.
>>>> Yes, but any benefit from doing this?
>>> It does seem a bit more modular, but it's certainly not a big deal.
>> Ok, if we go this way, it could be as simple as provide some callback to
>> vhost, then vhost can just forward the ioctl through parent_ops.
>>
>>>>>> 2) For method 2, is there any easy way for user/admin to distinguish e.g
>>>>>> ordinary vfio-mdev for vhost from ordinary vfio-mdev?
>>>>> I think device-api could be a choice.
>>>> Ok.
>>>>
>>>>
>>>>>> I saw you introduce
>>>>>> ops matching helper but it's not friendly to management.
>>>>> The ops matching helper is just to check whether a given
>>>>> vfio-device is based on a mdev device.
>>>>>
>>>>>> 3) A drawback of 1) and 2) is that it must follow vfio_device_ops that
>>>>>> assumes the parameter comes from userspace, it prevents support kernel
>>>>>> virtio drivers.
>>>>>>
>>>>>> 4) So comes the idea of method 3, since it register a new vhost-mdev driver,
>>>>>> we can use device specific ops instead of VFIO ones, then we can have a
>>>>>> common API between vDPA parent and vhost-mdev/virtio-mdev drivers.
>>>>> As the above draft shows, this requires introducing a new
>>>>> VFIO device driver. I think Alex's opinion matters here.
>> Just to clarify, a new type of mdev driver but provides dummy
>> vfio_device_ops for VFIO to make container DMA ioctl work.
> I see. Thanks! IIUC, you mean we can provide a very tiny
> VFIO device driver in drivers/vhost/mdev.c, e.g.:
>
> static int vfio_vhost_mdev_open(void *device_data)
> {
> if (!try_module_get(THIS_MODULE))
> return -ENODEV;
> return 0;
> }
>
> static void vfio_vhost_mdev_release(void *device_data)
> {
> module_put(THIS_MODULE);
> }
>
> static const struct vfio_device_ops vfio_vhost_mdev_dev_ops = {
> .name = "vfio-vhost-mdev",
> .open = vfio_vhost_mdev_open,
> .release = vfio_vhost_mdev_release,
> };
>
> static int vhost_mdev_probe(struct device *dev)
> {
> struct mdev_device *mdev = to_mdev_device(dev);
>
> ... Check the mdev device_id proposed in ...
> ... https://lkml.org/lkml/2019/9/12/151 ...
To clarify, this should be done through the id_table fields in
vhost_mdev_driver, and it should claim it supports virtio-mdev device only:
static struct mdev_class_id id_table[] = {
{ MDEV_ID_VIRTIO },
{ 0 },
};
static struct mdev_driver vhost_mdev_driver = {
...
.id_table = id_table,
}
>
> return vfio_add_group_dev(dev, &vfio_vhost_mdev_dev_ops, mdev);
And in vfio_vhost_mdev_ops, all its need is to just implement vhost-net
ioctl and translate them to virtio-mdev transport (e.g device_ops I
proposed or ioctls other whatever other method) API. And it could have a
dummy ops implementation for the other device_ops.
> }
>
> static void vhost_mdev_remove(struct device *dev)
> {
> vfio_del_group_dev(dev);
> }
>
> static struct mdev_driver vhost_mdev_driver = {
> .name = "vhost_mdev",
> .probe = vhost_mdev_probe,
> .remove = vhost_mdev_remove,
> };
>
> So we can bind above mdev driver to the virtio-mdev compatible
> mdev devices when we want to use vhost-mdev.
>
> After binding above driver to the mdev device, we can setup IOMMU
> via VFIO and get VFIO device fd of this mdev device, and pass it
> to vhost fd (/dev/vhost-mdev) with a SET_BACKEND ioctl.
Then what vhost-mdev char device did is just forwarding ioctl back to
this vfio device fd which seems a overkill. It's simpler that just do
ioctl on the device ops directly.
Thanks
>
> Thanks,
> Tiwei
>
>> Thanks
>>
>>
>>>> Yes, it is.
>>>>
>>>> Thanks
>>>>
>>>>
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: Correct the documentation of KSZ9021 skew values
From: Jakub Kicinski @ 2019-09-20 1:30 UTC (permalink / raw)
To: James Byrne
Cc: David Miller, robh+dt@kernel.org, mark.rutland@arm.com,
netdev@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <0102016d3b297538-fcca5199-6ad1-4625-b11c-3ad3919a0c48-000000@eu-west-1.amazonses.com>
On Mon, 16 Sep 2019 17:40:35 +0000, James Byrne wrote:
> On 16/09/2019 15:14, David Miller wrote:
> > From: James Byrne <james.byrne@origamienergy.com>
> > Date: Fri, 13 Sep 2019 16:46:35 +0000
> >
> >> The documentation of skew values for the KSZ9021 PHY was misleading
> >> because the driver implementation followed the erroneous information
> >> given in the original KSZ9021 datasheet before it was corrected in
> >> revision 1.2 (Feb 2014). It is probably too late to correct the driver
> >> now because of the many existing device trees, so instead this just
> >> corrects the documentation to explain that what you actually get is not
> >> what you might think when looking at the device tree.
> >>
> >> Signed-off-by: James Byrne <james.byrne@origamienergy.com>
> >
> > What tree should this go into?
>
> I believe this should go into the 'net' tree, but please let me know if
> I have submitted this patch incorrectly in some way.
Okay, applied, thanks.
^ permalink raw reply
* Re: [PATCH net v2 1/3] net: sched: sch_htb: don't call qdisc_put() while holding tree lock
From: Cong Wang @ 2019-09-20 1:05 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <20190919201438.2383-2-vladbu@mellanox.com>
On Thu, Sep 19, 2019 at 1:14 PM Vlad Buslov <vladbu@mellanox.com> wrote:
> Notes:
> Changes V1 -> V2:
>
> - Extend sch API with new qdisc_put_empty() function that has same
> implementation as regular qdisc_put() but skips parts that reset qdisc
> and free all packet buffers from gso_skb and skb_bad_txq queues.
I don't understand why you need a new API here, as long as qdisc_reset()
gets called before releasing sch tree lock, the ->reset() inside qdisc_put(),
after releasing sch tree lock, should be a nop, right?
^ permalink raw reply
* Re: [RFC v4 0/3] vhost: introduce mdev based hardware backend
From: Jason Wang @ 2019-09-20 0:59 UTC (permalink / raw)
To: Tiwei Bie
Cc: Michael S. Tsirkin, alex.williamson, maxime.coquelin,
linux-kernel, kvm, virtualization, netdev, dan.daly,
cunming.liang, zhihong.wang, lingshan.zhu
In-Reply-To: <20190919154552.GA27657@___>
On 2019/9/19 下午11:45, Tiwei Bie wrote:
> On Thu, Sep 19, 2019 at 09:08:11PM +0800, Jason Wang wrote:
>> On 2019/9/18 下午10:32, Michael S. Tsirkin wrote:
>>>>>> So I have some questions:
>>>>>>
>>>>>> 1) Compared to method 2, what's the advantage of creating a new vhost char
>>>>>> device? I guess it's for keep the API compatibility?
>>>>> One benefit is that we can avoid doing vhost ioctls on
>>>>> VFIO device fd.
>>>> Yes, but any benefit from doing this?
>>> It does seem a bit more modular, but it's certainly not a big deal.
>> Ok, if we go this way, it could be as simple as provide some callback to
>> vhost, then vhost can just forward the ioctl through parent_ops.
>>
>>>>>> 2) For method 2, is there any easy way for user/admin to distinguish e.g
>>>>>> ordinary vfio-mdev for vhost from ordinary vfio-mdev?
>>>>> I think device-api could be a choice.
>>>> Ok.
>>>>
>>>>
>>>>>> I saw you introduce
>>>>>> ops matching helper but it's not friendly to management.
>>>>> The ops matching helper is just to check whether a given
>>>>> vfio-device is based on a mdev device.
>>>>>
>>>>>> 3) A drawback of 1) and 2) is that it must follow vfio_device_ops that
>>>>>> assumes the parameter comes from userspace, it prevents support kernel
>>>>>> virtio drivers.
>>>>>>
>>>>>> 4) So comes the idea of method 3, since it register a new vhost-mdev driver,
>>>>>> we can use device specific ops instead of VFIO ones, then we can have a
>>>>>> common API between vDPA parent and vhost-mdev/virtio-mdev drivers.
>>>>> As the above draft shows, this requires introducing a new
>>>>> VFIO device driver. I think Alex's opinion matters here.
>> Just to clarify, a new type of mdev driver but provides dummy
>> vfio_device_ops for VFIO to make container DMA ioctl work.
> I see. Thanks! IIUC, you mean we can provide a very tiny
> VFIO device driver in drivers/vhost/mdev.c, e.g.:
>
> static int vfio_vhost_mdev_open(void *device_data)
> {
> if (!try_module_get(THIS_MODULE))
> return -ENODEV;
> return 0;
> }
>
> static void vfio_vhost_mdev_release(void *device_data)
> {
> module_put(THIS_MODULE);
> }
>
> static const struct vfio_device_ops vfio_vhost_mdev_dev_ops = {
> .name = "vfio-vhost-mdev",
> .open = vfio_vhost_mdev_open,
> .release = vfio_vhost_mdev_release,
> };
>
> static int vhost_mdev_probe(struct device *dev)
> {
> struct mdev_device *mdev = to_mdev_device(dev);
>
> ... Check the mdev device_id proposed in ...
> ... https://lkml.org/lkml/2019/9/12/151 ...
>
> return vfio_add_group_dev(dev, &vfio_vhost_mdev_dev_ops, mdev);
> }
>
> static void vhost_mdev_remove(struct device *dev)
> {
> vfio_del_group_dev(dev);
> }
>
> static struct mdev_driver vhost_mdev_driver = {
> .name = "vhost_mdev",
> .probe = vhost_mdev_probe,
> .remove = vhost_mdev_remove,
> };
>
> So we can bind above mdev driver to the virtio-mdev compatible
> mdev devices when we want to use vhost-mdev.
>
> After binding above driver to the mdev device, we can setup IOMMU
> via VFIO and get VFIO device fd of this mdev device, and pass it
> to vhost fd (/dev/vhost-mdev) with a SET_BACKEND ioctl.
>
> Thanks,
> Tiwei
Yes, something like this.
Thanks
>> Thanks
>>
>>
>>>> Yes, it is.
>>>>
>>>> Thanks
>>>>
>>>>
^ permalink raw reply
* Re: ionic build issues?
From: Jakub Kicinski @ 2019-09-20 0:50 UTC (permalink / raw)
To: Randy Dunlap; +Cc: Shannon Nelson, netdev
In-Reply-To: <a99cf2d8-2479-2003-ac0b-10d44363c872@infradead.org>
On Thu, 19 Sep 2019 17:29:28 -0700, Randy Dunlap wrote:
> On 9/19/19 5:27 PM, Jakub Kicinski wrote:
> > Hi Shannon!
> >
> > I've enabled ionic to do some build testing before pushing patches
> > today, and with my tree which I build with W=1 C=1 I run into this:
> >
> > ../drivers/net/ethernet/pensando/ionic/ionic_main.c: In function ‘ionic_adminq_cb’:
> > ../drivers/net/ethernet/pensando/ionic/ionic_main.c:229:2: error: implicit declaration of function ‘dynamic_hex_dump’ [-Werror=implicit-function-declaration]
> > 229 | dynamic_hex_dump("comp ", DUMP_PREFIX_OFFSET, 16, 1,
> > | ^~~~~~~~~~~~~~~~
> > ../drivers/net/ethernet/pensando/ionic/ionic_lif.c: In function ‘ionic_notifyq_service’:
> > ../drivers/net/ethernet/pensando/ionic/ionic_lif.c:673:2: error: implicit declaration of function ‘dynamic_hex_dump’ [-Werror=implicit-function-declaration]
> > 673 | dynamic_hex_dump("event ", DUMP_PREFIX_OFFSET, 16, 1,
> > | ^~~~~~~~~~~~~~~~
> > cc1: some warnings being treated as errors
> >
> > Config attached, could you please take a look?
> >
>
> Patch is already posted.
> See https://lore.kernel.org/netdev/20190918195607.2080036-1-arnd@arndb.de/
Ah, thanks for pointing that out.
^ permalink raw reply
* Re: [PATCH net] ibmvnic: Warn unknown speed message only when carrier is present
From: Jakub Kicinski @ 2019-09-20 0:37 UTC (permalink / raw)
To: Murilo Fossa Vicentini; +Cc: netdev, tlfalcon, muvic, abdhalee
In-Reply-To: <20190916145037.77376-1-muvic@linux.ibm.com>
On Mon, 16 Sep 2019 11:50:37 -0300, Murilo Fossa Vicentini wrote:
> With commit 0655f9943df2 ("net/ibmvnic: Update carrier state after link
> state change") we are now able to detect when the carrier is properly
> present in the device, so only report an unexpected unknown speed when it
> is properly detected. Unknown speed is expected to be seen by the device
> in case the backing device has no link detected.
>
> Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
> Tested-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
> Signed-off-by: Murilo Fossa Vicentini <muvic@linux.ibm.com>
Applied, thanks!
^ permalink raw reply
* Re: ionic build issues?
From: Randy Dunlap @ 2019-09-20 0:29 UTC (permalink / raw)
To: Jakub Kicinski, Shannon Nelson; +Cc: netdev
In-Reply-To: <20190919172739.0c6b4bc4@cakuba.netronome.com>
On 9/19/19 5:27 PM, Jakub Kicinski wrote:
> Hi Shannon!
>
> I've enabled ionic to do some build testing before pushing patches
> today, and with my tree which I build with W=1 C=1 I run into this:
>
> ../drivers/net/ethernet/pensando/ionic/ionic_main.c: In function ‘ionic_adminq_cb’:
> ../drivers/net/ethernet/pensando/ionic/ionic_main.c:229:2: error: implicit declaration of function ‘dynamic_hex_dump’ [-Werror=implicit-function-declaration]
> 229 | dynamic_hex_dump("comp ", DUMP_PREFIX_OFFSET, 16, 1,
> | ^~~~~~~~~~~~~~~~
> ../drivers/net/ethernet/pensando/ionic/ionic_lif.c: In function ‘ionic_notifyq_service’:
> ../drivers/net/ethernet/pensando/ionic/ionic_lif.c:673:2: error: implicit declaration of function ‘dynamic_hex_dump’ [-Werror=implicit-function-declaration]
> 673 | dynamic_hex_dump("event ", DUMP_PREFIX_OFFSET, 16, 1,
> | ^~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
>
> Config attached, could you please take a look?
>
Patch is already posted.
See https://lore.kernel.org/netdev/20190918195607.2080036-1-arnd@arndb.de/
--
~Randy
^ permalink raw reply
* ionic build issues?
From: Jakub Kicinski @ 2019-09-20 0:27 UTC (permalink / raw)
To: Shannon Nelson; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 937 bytes --]
Hi Shannon!
I've enabled ionic to do some build testing before pushing patches
today, and with my tree which I build with W=1 C=1 I run into this:
../drivers/net/ethernet/pensando/ionic/ionic_main.c: In function ‘ionic_adminq_cb’:
../drivers/net/ethernet/pensando/ionic/ionic_main.c:229:2: error: implicit declaration of function ‘dynamic_hex_dump’ [-Werror=implicit-function-declaration]
229 | dynamic_hex_dump("comp ", DUMP_PREFIX_OFFSET, 16, 1,
| ^~~~~~~~~~~~~~~~
../drivers/net/ethernet/pensando/ionic/ionic_lif.c: In function ‘ionic_notifyq_service’:
../drivers/net/ethernet/pensando/ionic/ionic_lif.c:673:2: error: implicit declaration of function ‘dynamic_hex_dump’ [-Werror=implicit-function-declaration]
673 | dynamic_hex_dump("event ", DUMP_PREFIX_OFFSET, 16, 1,
| ^~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
Config attached, could you please take a look?
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27441 bytes --]
^ permalink raw reply
* Re: [PATCH] ionic: remove useless return code
From: Jakub Kicinski @ 2019-09-20 0:21 UTC (permalink / raw)
To: Shannon Nelson
Cc: Arnd Bergmann, Pensando Drivers, David S. Miller, netdev,
linux-kernel, clang-built-linux
In-Reply-To: <6cdb1e21-44d9-bba9-1931-78f7109bff2b@pensando.io>
On Wed, 18 Sep 2019 13:46:34 -0700, Shannon Nelson wrote:
> On 9/18/19 12:57 PM, Arnd Bergmann wrote:
> > The debugfs function was apparently changed from returning an error code
> > to a void return, but the return code left in place, causing a warning
> > from clang:
> >
> > drivers/net/ethernet/pensando/ionic/ionic_debugfs.c:60:37: error: expression result unused [-Werror,-Wunused-value]
> > ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
> > ^~~~~~~~~~~
> >
> > Fixes: fbfb8031533c ("ionic: Add hardware init and device commands")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > drivers/net/ethernet/pensando/ionic/ionic_debugfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> > index 7afc4a365b75..bc03cecf80cc 100644
> > --- a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> > @@ -57,7 +57,7 @@ DEFINE_SHOW_ATTRIBUTE(identity);
> > void ionic_debugfs_add_ident(struct ionic *ionic)
> > {
> > debugfs_create_file("identity", 0400, ionic->dentry,
> > - ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
> > + ionic, &identity_fops);
> > }
> >
> > void ionic_debugfs_add_sizes(struct ionic *ionic)
>
> This has just recently been addressed by Nathan Chancellor
> <natechancellor@gmail.com>
Yup, should be in the net tree now.
> Either way,
>
> Acked-by: Shannon Nelson <snelson@pensando.io>
Thanks for quick reviews!
^ permalink raw reply
* Re: [PATCH] ionic: Remove unnecessary ternary operator in ionic_debugfs_add_ident
From: Jakub Kicinski @ 2019-09-20 0:21 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Shannon Nelson, Pensando Drivers, David S. Miller, netdev,
linux-kernel, clang-built-linux, Greg Kroah-Hartman
In-Reply-To: <20190917232616.125261-1-natechancellor@gmail.com>
On Tue, 17 Sep 2019 16:26:16 -0700, Nathan Chancellor wrote:
> clang warns:
>
> ../drivers/net/ethernet/pensando/ionic/ionic_debugfs.c:60:37: warning:
> expression result unused [-Wunused-value]
> ionic, &identity_fops) ? 0 : -EOPNOTSUPP;
> ^~~~~~~~~~~
> 1 warning generated.
>
> The return value of debugfs_create_file does not need to be checked [1]
> and the function returns void so get rid of the ternary operator, it is
> unnecessary.
>
> [1]: https://lore.kernel.org/linux-mm/20150815160730.GB25186@kroah.com/
>
> Fixes: fbfb8031533c ("ionic: Add hardware init and device commands")
> Link: https://github.com/ClangBuiltLinux/linux/issues/658
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Applied, thank you!
^ permalink raw reply
* Re: [RFC {net,iproute2}-next 0/2] Introduce an eBPF hookpoint for tx queue selection in the XPS (Transmit Packet Steering) code.
From: Matt Cover @ 2019-09-20 0:05 UTC (permalink / raw)
To: davem, ast, daniel, kafai, songliubraving, yhs, nikolay, sd,
sbrivio, vincent, kda, Matthew Cover, jiri, Eric Dumazet, pabeni,
idosch, petrm, f.fainelli, stephen, dsahern, christian,
jakub.kicinski, Roopa Prabhu, johannes.berg, mkubecek, netdev,
linux-kernel, bpf, Jason Wang
In-Reply-To: <20190919224458.91422-1-matthew.cover@stackpath.com>
On Thu, Sep 19, 2019 at 3:45 PM Matthew Cover <werekraken@gmail.com> wrote:
>
> WORK IN PROGRESS:
> * bpf program loading works!
> * txq steering via bpf program return code works!
> * bpf program unloading not working.
> * bpf program attached query not working.
>
> This patch set provides a bpf hookpoint with goals similar to, but a more
> generic implementation than, TUNSETSTEERINGEBPF; userspace supplied tx queue
> selection policy.
>
> TUNSETSTEERINGEBPF is a useful bpf hookpoint, but has some drawbacks.
>
> First, it only works on tun/tap devices.
>
> Second, there is no way in the current TUNSETSTEERINGEBPF implementation
> to bail out or load a noop bpf prog and fallback to the no prog tx queue
> selection method.
>
> Third, the TUNSETSTEERINGEBPF interface seems to require possession of existing
> or creation of new queues/fds.
>
> This most naturally fits in the "wire" implementation since possession of fds
> is ensured. However, it also means the various "wire" implementations (e.g.
> qemu) have to all be made aware of TUNSETSTEERINGEBPF and expose an interface
> to load/unload a bpf prog (or provide a mechanism to pass an fd to another
> program).
>
> Alternatively, you can spin up an extra queue and immediately disable via
> IFF_DETACH_QUEUE, but this seems unsafe; packets could be enqueued to this
> extra file descriptor which is part of our bpf prog loader, not our "wire".
>
> Placing this in the XPS code and leveraging iproute2 and rtnetlink to provide
> our bpf prog loader in a similar manner to xdp gives us a nice way to separate
> the tap "wire" and the loading of tx queue selection policy. It also lets us
> use this hookpoint for any device traversing XPS.
>
> This patch only introduces the new hookpoint to the XPS code and will not yet
> be used by tun/tap devices using the intree tun.ko (which implements an
> .ndo_select_queue and does not traverse the XPS code).
>
> In a future patch set, we can optionally refactor tun.ko to traverse this call
> to bpf_prog_run_clear_cb() and bpf prog storage. tun/tap devices could then
> leverage iproute2 as a generic loader. The TUNSETSTEERINGEBPF interface could
> at this point be optionally deprecated/removed.
>
> Both patches in this set have been tested using a rebuilt tun.ko with no
> .ndo_select_queue.
>
> sed -i '/\.ndo_select_queue.*=/d' drivers/net/tun.c
>
> The tap device was instantiated using tap_mq_pong.c, supporting scripts, and
> wrapping service found here:
>
> https://github.com/stackpath/rxtxcpu/tree/v1.2.6/helpers
>
> The bpf prog source and test scripts can be found here:
>
> https://github.com/werekraken/xps_ebpf
>
> In nstxq, netsniff-ng using PACKET_FANOUT_QM is leveraged to check the
> queue_mapping.
>
> With no prog loaded, the tx queue selection is adhering our xps_cpus
> configuration.
>
> [vagrant@localhost ~]$ grep . /sys/class/net/tap0/queues/tx-*/xps_cpus; ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe;
> /sys/class/net/tap0/queues/tx-0/xps_cpus:1
> /sys/class/net/tap0/queues/tx-1/xps_cpus:2
> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.146 ms
> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.121 ms
> cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
>
> With a return 0 bpg prog, our tx queue is 0 (despite xps_cpus).
>
> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello0.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.160 ms
> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.124 ms
> cpu1: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> ping-4852 [000] .... 2691.633260: 0: xps (RET 0): Hello, World!
> ping-4869 [001] .... 2695.753588: 0: xps (RET 0): Hello, World!
>
> With a return 1 bpg prog, our tx queue is 1.
>
> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello1.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.193 ms
> cpu0: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.135 ms
> cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> ping-4894 [000] .... 2710.652080: 0: xps (RET 1): Hello, World!
> ping-4911 [001] .... 2714.774608: 0: xps (RET 1): Hello, World!
>
> With a return 2 bpg prog, our tx queue is 0 (we only have 2 tx queues).
>
> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello2.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=1.20 ms
> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.986 ms
> cpu1: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> ping-4936 [000] .... 2729.442668: 0: xps (RET 2): Hello, World!
> ping-4953 [001] .... 2733.614558: 0: xps (RET 2): Hello, World!
>
> With a return -1 bpf prog, our tx queue selection is once again determined by
> xps_cpus. Any negative return should work the same and provides a nice
> mechanism to bail out or have a noop bpf prog at this hookpoint.
>
> [vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello_neg1.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
> cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.628 ms
> cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.322 ms
> cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
> ping-4981 [000] .... 2763.510760: 0: xps (RET -1): Hello, World!
> ping-4998 [001] .... 2767.632583: 0: xps (RET -1): Hello, World!
>
> bpf prog unloading is not yet working and neither does `ip link show` report
> when an "xps" bpf prog is attached. This is my first time touching iproute2 or
> rtnetlink, so it may be something obvious to those more familiar.
Adding Jason... sorry for missing that the first time.
^ permalink raw reply
* Re: [PATCH iproute2 master] bpf: Fix race condition with map pinning
From: Daniel Borkmann @ 2019-09-19 23:25 UTC (permalink / raw)
To: Joe Stringer; +Cc: Stephen Hemminger, netdev
In-Reply-To: <20190919220733.31206-1-joe@wand.net.nz>
On Thu, Sep 19, 2019 at 03:07:33PM -0700, Joe Stringer wrote:
> If two processes attempt to invoke bpf_map_attach() at the same time,
> then they will both create maps, then the first will successfully pin
> the map to the filesystem and the second will not pin the map, but will
> continue operating with a reference to its own copy of the map. As a
> result, the sharing of the same map will be broken from the two programs
> that were concurrently loaded via loaders using this library.
>
> Fix this by adding a retry in the case where the pinning fails because
> the map already exists on the filesystem. In that case, re-attempt
> opening a fd to the map on the filesystem as it shows that another
> program already created and pinned a map at that location.
>
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: [PATCH net v2 1/3] net: sched: sch_htb: don't call qdisc_put() while holding tree lock
From: Eric Dumazet @ 2019-09-19 22:49 UTC (permalink / raw)
To: Vlad Buslov, netdev; +Cc: jhs, xiyou.wangcong, jiri, davem
In-Reply-To: <20190919201438.2383-2-vladbu@mellanox.com>
On 9/19/19 1:14 PM, Vlad Buslov wrote:
> Recent changes that removed rtnl dependency from rules update path of tc
> also made tcf_block_put() function sleeping. This function is called from
> ops->destroy() of several Qdisc implementations, which in turn is called by
> qdisc_put(). Some Qdiscs call qdisc_put() while holding sch tree spinlock,
> which results sleeping-while-atomic BUG.
>
Note that calling qdisc_put() while holding sch tree lock can also
trigger deadlocks.
For example sch_pie.c has a del_timer_sync() in pie_destroy(),
while the pie_timer() timer handler acquires the root_lock.
(there are other cases like that, SFQ for example)
^ permalink raw reply
* [RFC iproute2-next 2/2] Introduce an eBPF hookpoint for tx queue selection in the XPS (Transmit Packet Steering) code.
From: Matthew Cover @ 2019-09-19 22:46 UTC (permalink / raw)
To: davem, ast, daniel, kafai, songliubraving, yhs, nikolay, sd,
sbrivio, vincent, kda, matthew.cover, jiri, edumazet, pabeni,
idosch, petrm, f.fainelli, stephen, dsahern, christian,
jakub.kicinski, roopa, johannes.berg, mkubecek, netdev,
linux-kernel, bpf
WORK IN PROGRESS:
* bpf program loading works!
* txq steering via bpf program return code works!
* bpf program unloading not working.
* bpf program attached query not working.
---
include/bpf_api.h | 5 +++
include/uapi/linux/if_link.h | 12 ++++++
ip/Makefile | 2 +-
ip/ip_common.h | 4 ++
ip/iplink.c | 7 ++++
ip/iplink_xps.c | 88 ++++++++++++++++++++++++++++++++++++++++++++
lib/bpf.c | 6 +++
7 files changed, 123 insertions(+), 1 deletion(-)
create mode 100644 ip/iplink_xps.c
diff --git a/include/bpf_api.h b/include/bpf_api.h
index 89d3488..d1a2d90 100644
--- a/include/bpf_api.h
+++ b/include/bpf_api.h
@@ -78,6 +78,11 @@
__section(ELF_SECTION_PROG)
#endif
+#ifndef __section_xps_entry
+# define __section_xps_entry \
+ __section(ELF_SECTION_PROG)
+#endif
+
#ifndef __section_cls_entry
# define __section_cls_entry \
__section(ELF_SECTION_CLASSIFIER)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index d36919f..9efd686 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -167,6 +167,7 @@ enum {
IFLA_NEW_IFINDEX,
IFLA_MIN_MTU,
IFLA_MAX_MTU,
+ IFLA_XPS,
__IFLA_MAX
};
@@ -977,6 +978,17 @@ enum {
#define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
+/* XPS section */
+
+enum {
+ IFLA_XPS_UNSPEC,
+ IFLA_XPS_FD,
+ IFLA_XPS_ATTACHED,
+ __IFLA_XPS_MAX,
+};
+
+#define IFLA_XPS_MAX (__IFLA_XPS_MAX - 1)
+
enum {
IFLA_EVENT_NONE,
IFLA_EVENT_REBOOT, /* internal reset / reboot */
diff --git a/ip/Makefile b/ip/Makefile
index 5ab78d7..9ad1c53 100644
--- a/ip/Makefile
+++ b/ip/Makefile
@@ -5,7 +5,7 @@ IPOBJ=ip.o ipaddress.o ipaddrlabel.o iproute.o iprule.o ipnetns.o \
ipxfrm.o xfrm_state.o xfrm_policy.o xfrm_monitor.o iplink_dummy.o \
iplink_ifb.o iplink_nlmon.o iplink_team.o iplink_vcan.o iplink_vxcan.o \
iplink_vlan.o link_veth.o link_gre.o iplink_can.o iplink_xdp.o \
- iplink_macvlan.o ipl2tp.o link_vti.o link_vti6.o link_xfrm.o \
+ iplink_macvlan.o ipl2tp.o link_vti.o link_vti6.o link_xfrm.o iplink_xps.o \
iplink_vxlan.o tcp_metrics.o iplink_ipoib.o ipnetconf.o link_ip6tnl.o \
link_iptnl.o link_gre6.o iplink_bond.o iplink_bond_slave.o iplink_hsr.o \
iplink_bridge.o iplink_bridge_slave.o ipfou.o iplink_ipvlan.o \
diff --git a/ip/ip_common.h b/ip/ip_common.h
index cd916ec..805d7d2 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -145,6 +145,10 @@ int xdp_parse(int *argc, char ***argv, struct iplink_req *req, const char *ifnam
bool generic, bool drv, bool offload);
void xdp_dump(FILE *fp, struct rtattr *tb, bool link, bool details);
+/* iplink_xps.c */
+int xps_parse(int *argc, char ***argv, struct iplink_req *req);
+void xps_dump(FILE *fp, struct rtattr *tb);
+
/* iplink_vrf.c */
__u32 ipvrf_get_table(const char *name);
int name_is_vrf(const char *name);
diff --git a/ip/iplink.c b/ip/iplink.c
index 212a088..4d6d557 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -101,6 +101,9 @@ void iplink_usage(void)
" [ { xdp | xdpgeneric | xdpdrv | xdpoffload } { off |\n"
" object FILE [ section NAME ] [ verbose ] |\n"
" pinned FILE } ]\n"
+ " [ xps { off |\n"
+ " object FILE [ section NAME ] [ verbose ] |\n"
+ " pinned FILE } ]\n"
" [ master DEVICE ][ vrf NAME ]\n"
" [ nomaster ]\n"
" [ addrgenmode { eui64 | none | stable_secret | random } ]\n"
@@ -668,6 +671,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
if (offload && name == dev)
dev = NULL;
+ } else if (strcmp(*argv, "xps") == 0) {
+ NEXT_ARG();
+ if (xps_parse(&argc, &argv, req))
+ exit(-1);
} else if (strcmp(*argv, "netns") == 0) {
NEXT_ARG();
if (netns != -1)
diff --git a/ip/iplink_xps.c b/ip/iplink_xps.c
new file mode 100644
index 0000000..7e94164
--- /dev/null
+++ b/ip/iplink_xps.c
@@ -0,0 +1,88 @@
+/*
+ * iplink_xps.c XPS program loader
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * Authors: Matthew Cover <matthew.cover@stackpath.com>
+ *
+ * Based on iplink_xdp.c by Daniel Borkmann <daniel@iogearbox.net>
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <linux/bpf.h>
+
+#include "bpf_util.h"
+#include "ip_common.h"
+
+extern int force;
+
+struct xps_req {
+ struct iplink_req *req;
+ __u32 flags;
+};
+
+static void xps_ebpf_cb(void *raw, int fd, const char *annotation)
+{
+ struct xps_req *xps = raw;
+ struct iplink_req *req = xps->req;
+ struct rtattr *xps_attr;
+
+ xps_attr = addattr_nest(&req->n, sizeof(*req), IFLA_XPS);
+ addattr32(&req->n, sizeof(*req), IFLA_XPS_FD, fd);
+ addattr_nest_end(&req->n, xps_attr);
+}
+
+static const struct bpf_cfg_ops bpf_cb_ops = {
+ .ebpf_cb = xps_ebpf_cb,
+};
+
+static int xps_delete(struct iplink_req *req)
+{
+ xps_ebpf_cb(req, -1, NULL);
+ return 0;
+}
+
+int xps_parse(int *argc, char ***argv, struct iplink_req *req)
+{
+
+ struct bpf_cfg_in cfg = {
+ .type = BPF_PROG_TYPE_SOCKET_FILTER,
+ .argc = *argc,
+ .argv = *argv,
+ };
+
+ struct xps_req xps = {
+ .req = req,
+ };
+
+ if (*argc == 1) {
+ if (strcmp(**argv, "none") == 0 ||
+ strcmp(**argv, "off") == 0)
+ return xps_delete(req);
+ }
+
+ if (bpf_parse_and_load_common(&cfg, &bpf_cb_ops, &xps))
+ return -1;
+
+ *argc = cfg.argc;
+ *argv = cfg.argv;
+ return 0;
+}
+
+void xps_dump(FILE *fp, struct rtattr *xps)
+{
+ struct rtattr *tb[IFLA_XPS_MAX + 1];
+
+ parse_rtattr_nested(tb, IFLA_XPS_MAX, xps);
+ if (!tb[IFLA_XPS_ATTACHED] ||
+ !rta_getattr_u8(tb[IFLA_XPS_ATTACHED]))
+ return;
+
+ fprintf(fp, "xps ");
+ /* More to come here in future for 'ip -d link' (digest, etc) ... */
+}
diff --git a/lib/bpf.c b/lib/bpf.c
index 7d2a322..e883afb 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -60,6 +60,7 @@ static const enum bpf_prog_type __bpf_types[] = {
BPF_PROG_TYPE_LWT_IN,
BPF_PROG_TYPE_LWT_OUT,
BPF_PROG_TYPE_LWT_XMIT,
+ BPF_PROG_TYPE_SOCKET_FILTER,
};
static const struct bpf_prog_meta __bpf_prog_meta[] = {
@@ -100,6 +101,11 @@ static const struct bpf_prog_meta __bpf_prog_meta[] = {
.subdir = "ip",
.section = ELF_SECTION_PROG,
},
+ [BPF_PROG_TYPE_SOCKET_FILTER] = {
+ .type = "xps",
+ .subdir = "xps",
+ .section = ELF_SECTION_PROG,
+ },
};
static const char *bpf_prog_to_subdir(enum bpf_prog_type type)
--
1.8.3.1
^ permalink raw reply related
* [RFC net-next 1/2] Introduce an eBPF hookpoint for tx queue selection in the XPS (Transmit Packet Steering) code.
From: Matthew Cover @ 2019-09-19 22:45 UTC (permalink / raw)
To: davem, ast, daniel, kafai, songliubraving, yhs, nikolay, sd,
sbrivio, vincent, kda, matthew.cover, jiri, edumazet, pabeni,
idosch, petrm, f.fainelli, stephen, dsahern, christian,
jakub.kicinski, roopa, johannes.berg, mkubecek, netdev,
linux-kernel, bpf
WORK IN PROGRESS:
* bpf program loading works!
* txq steering via bpf program return code works!
* bpf program unloading not working.
* bpf program attached query not working.
---
include/linux/netdevice.h | 3 +++
include/uapi/linux/if_link.h | 12 +++++++++
net/core/dev.c | 61 ++++++++++++++++++++++++++++++++++++-------
net/core/rtnetlink.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 129 insertions(+), 9 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9eda1c3..88e37d5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1966,6 +1966,7 @@ struct net_device {
#ifdef CONFIG_XPS
struct xps_dev_maps __rcu *xps_cpus_map;
struct xps_dev_maps __rcu *xps_rxqs_map;
+ struct bpf_prog __rcu *xps_prog;
#endif
#ifdef CONFIG_NET_CLS_ACT
struct mini_Qdisc __rcu *miniq_egress;
@@ -2147,6 +2148,8 @@ struct netdev_queue *netdev_core_pick_tx(struct net_device *dev,
struct sk_buff *skb,
struct net_device *sb_dev);
+int dev_change_xps_fd(struct net_device *dev, int fd);
+
/* returns the headroom that the master device needs to take in account
* when forwarding to this dev
*/
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 4a8c02c..a23d241 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -167,6 +167,7 @@ enum {
IFLA_NEW_IFINDEX,
IFLA_MIN_MTU,
IFLA_MAX_MTU,
+ IFLA_XPS,
__IFLA_MAX
};
@@ -979,6 +980,17 @@ enum {
#define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
+/* XPS section */
+
+enum {
+ IFLA_XPS_UNSPEC,
+ IFLA_XPS_FD,
+ IFLA_XPS_ATTACHED,
+ __IFLA_XPS_MAX,
+};
+
+#define IFLA_XPS_MAX (__IFLA_XPS_MAX - 1)
+
enum {
IFLA_EVENT_NONE,
IFLA_EVENT_REBOOT, /* internal reset / reboot */
diff --git a/net/core/dev.c b/net/core/dev.c
index 71b18e8..a46d42b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3663,26 +3663,34 @@ static int get_xps_queue(struct net_device *dev, struct net_device *sb_dev,
{
#ifdef CONFIG_XPS
struct xps_dev_maps *dev_maps;
+ struct bpf_prog *prog;
struct sock *sk = skb->sk;
+ int bpf_ret = -1;
int queue_index = -1;
if (!static_key_false(&xps_needed))
return -1;
rcu_read_lock();
- if (!static_key_false(&xps_rxqs_needed))
- goto get_cpus_map;
- dev_maps = rcu_dereference(sb_dev->xps_rxqs_map);
- if (dev_maps) {
- int tci = sk_rx_queue_get(sk);
+ prog = rcu_dereference(dev->xps_prog);
+ if (prog) {
+ bpf_ret = bpf_prog_run_clear_cb(prog, skb);
+ if (bpf_ret >= 0)
+ queue_index = bpf_ret % dev->num_tx_queues;
+ }
- if (tci >= 0 && tci < dev->num_rx_queues)
- queue_index = __get_xps_queue_idx(dev, skb, dev_maps,
- tci);
+ if (queue_index < 0 && static_key_false(&xps_rxqs_needed)) {
+ dev_maps = rcu_dereference(sb_dev->xps_rxqs_map);
+ if (dev_maps) {
+ int tci = sk_rx_queue_get(sk);
+
+ if (tci >= 0 && tci < dev->num_rx_queues)
+ queue_index = __get_xps_queue_idx(dev, skb,
+ dev_maps, tci);
+ }
}
-get_cpus_map:
if (queue_index < 0) {
dev_maps = rcu_dereference(sb_dev->xps_cpus_map);
if (dev_maps) {
@@ -8170,6 +8178,41 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
return err;
}
+static void dev_xps_install(struct net_device *dev, struct bpf_prog *prog)
+{
+#ifdef CONFIG_XPS
+ struct bpf_prog *old = rtnl_dereference(dev->xps_prog);
+ struct bpf_prog *new = prog;
+
+ rcu_assign_pointer(dev->xps_prog, new);
+ if (old)
+ bpf_prog_put(old);
+#endif
+}
+
+/**
+ * dev_change_xps_fd - set or clear a bpf program for tx queue selection for a device
+ * @dev: device
+ * @fd: new program fd or negative value to clear
+ *
+ * Set or clear a bpf program for a device
+ */
+int dev_change_xps_fd(struct net_device *dev, int fd)
+{
+ struct bpf_prog *prog = NULL;
+
+ ASSERT_RTNL();
+
+ prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER);
+
+ if (IS_ERR(prog))
+ return PTR_ERR(prog);
+
+ dev_xps_install(dev, prog);
+
+ return 0;
+}
+
/**
* dev_new_index - allocate an ifindex
* @net: the applicable net namespace
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1ee6460..202b59a 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -980,6 +980,15 @@ static size_t rtnl_xdp_size(void)
return xdp_size;
}
+static size_t rtnl_xps_size(void)
+{
+ size_t xps_size = nla_total_size(0) + /* nest IFLA_XPS */
+ nla_total_size(1) + /* XPS_ATTACHED */
+ nla_total_size(4); /* XPS_PROG_ID */
+
+ return xps_size;
+}
+
static noinline size_t if_nlmsg_size(const struct net_device *dev,
u32 ext_filter_mask)
{
@@ -1018,6 +1027,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
+ nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
+ nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
+ rtnl_xdp_size() /* IFLA_XDP */
+ + rtnl_xps_size() /* IFLA_XPS */
+ nla_total_size(4) /* IFLA_EVENT */
+ nla_total_size(4) /* IFLA_NEW_NETNSID */
+ nla_total_size(4) /* IFLA_NEW_IFINDEX */
@@ -1455,6 +1465,31 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
return err;
}
+static int rtnl_xps_fill(struct sk_buff *skb, struct net_device *dev)
+{
+ struct nlattr *xps;
+ struct bpf_prog *xps_prog;
+ int err;
+
+ ASSERT_RTNL();
+
+ xps = nla_nest_start(skb, IFLA_XPS);
+ if (!xps)
+ return -EMSGSIZE;
+
+ xps_prog = rtnl_dereference(dev->xps_prog);
+ if (xps_prog) {
+ err = nla_put_u8(skb, IFLA_XPS_ATTACHED, xps_prog->aux->id);
+ if (err) {
+ nla_nest_cancel(skb, xps);
+ return err;
+ }
+ }
+
+ nla_nest_end(skb, xps);
+ return 0;
+}
+
static u32 rtnl_get_event(unsigned long event)
{
u32 rtnl_event_type = IFLA_EVENT_NONE;
@@ -1697,6 +1732,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
goto nla_put_failure_rcu;
rcu_read_unlock();
+ if (rtnl_xps_fill(skb, dev))
+ goto nla_put_failure;
+
nlmsg_end(skb, nlh);
return 0;
@@ -1750,6 +1788,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
[IFLA_CARRIER_DOWN_COUNT] = { .type = NLA_U32 },
[IFLA_MIN_MTU] = { .type = NLA_U32 },
[IFLA_MAX_MTU] = { .type = NLA_U32 },
+ [IFLA_XPS] = { .type = NLA_NESTED },
};
static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
@@ -1801,6 +1840,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
[IFLA_XDP_PROG_ID] = { .type = NLA_U32 },
};
+static const struct nla_policy ifla_xps_policy[IFLA_XPS_MAX + 1] = {
+ [IFLA_XPS_FD] = { .type = NLA_S32 },
+ [IFLA_XPS_ATTACHED] = { .type = NLA_U8 },
+};
+
static const struct rtnl_link_ops *linkinfo_to_kind_ops(const struct nlattr *nla)
{
const struct rtnl_link_ops *ops = NULL;
@@ -2709,6 +2753,24 @@ static int do_setlink(const struct sk_buff *skb,
}
}
+ if (tb[IFLA_XPS]) {
+ struct nlattr *xps[IFLA_XPS_MAX + 1];
+
+ err = nla_parse_nested_deprecated(xps, IFLA_XPS_MAX,
+ tb[IFLA_XPS],
+ ifla_xps_policy, NULL);
+ if (err < 0)
+ goto errout;
+
+ if (xps[IFLA_XPS_FD]) {
+ err = dev_change_xps_fd(dev,
+ nla_get_s32(xps[IFLA_XPS_FD]));
+ if (err)
+ goto errout;
+ status |= DO_SETLINK_NOTIFY;
+ }
+ }
+
errout:
if (status & DO_SETLINK_MODIFIED) {
if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
--
1.8.3.1
^ permalink raw reply related
* [RFC {net,iproute2}-next 0/2] Introduce an eBPF hookpoint for tx queue selection in the XPS (Transmit Packet Steering) code.
From: Matthew Cover @ 2019-09-19 22:44 UTC (permalink / raw)
To: davem, ast, daniel, kafai, songliubraving, yhs, nikolay, sd,
sbrivio, vincent, kda, matthew.cover, jiri, edumazet, pabeni,
idosch, petrm, f.fainelli, stephen, dsahern, christian,
jakub.kicinski, roopa, johannes.berg, mkubecek, netdev,
linux-kernel, bpf
WORK IN PROGRESS:
* bpf program loading works!
* txq steering via bpf program return code works!
* bpf program unloading not working.
* bpf program attached query not working.
This patch set provides a bpf hookpoint with goals similar to, but a more
generic implementation than, TUNSETSTEERINGEBPF; userspace supplied tx queue
selection policy.
TUNSETSTEERINGEBPF is a useful bpf hookpoint, but has some drawbacks.
First, it only works on tun/tap devices.
Second, there is no way in the current TUNSETSTEERINGEBPF implementation
to bail out or load a noop bpf prog and fallback to the no prog tx queue
selection method.
Third, the TUNSETSTEERINGEBPF interface seems to require possession of existing
or creation of new queues/fds.
This most naturally fits in the "wire" implementation since possession of fds
is ensured. However, it also means the various "wire" implementations (e.g.
qemu) have to all be made aware of TUNSETSTEERINGEBPF and expose an interface
to load/unload a bpf prog (or provide a mechanism to pass an fd to another
program).
Alternatively, you can spin up an extra queue and immediately disable via
IFF_DETACH_QUEUE, but this seems unsafe; packets could be enqueued to this
extra file descriptor which is part of our bpf prog loader, not our "wire".
Placing this in the XPS code and leveraging iproute2 and rtnetlink to provide
our bpf prog loader in a similar manner to xdp gives us a nice way to separate
the tap "wire" and the loading of tx queue selection policy. It also lets us
use this hookpoint for any device traversing XPS.
This patch only introduces the new hookpoint to the XPS code and will not yet
be used by tun/tap devices using the intree tun.ko (which implements an
.ndo_select_queue and does not traverse the XPS code).
In a future patch set, we can optionally refactor tun.ko to traverse this call
to bpf_prog_run_clear_cb() and bpf prog storage. tun/tap devices could then
leverage iproute2 as a generic loader. The TUNSETSTEERINGEBPF interface could
at this point be optionally deprecated/removed.
Both patches in this set have been tested using a rebuilt tun.ko with no
.ndo_select_queue.
sed -i '/\.ndo_select_queue.*=/d' drivers/net/tun.c
The tap device was instantiated using tap_mq_pong.c, supporting scripts, and
wrapping service found here:
https://github.com/stackpath/rxtxcpu/tree/v1.2.6/helpers
The bpf prog source and test scripts can be found here:
https://github.com/werekraken/xps_ebpf
In nstxq, netsniff-ng using PACKET_FANOUT_QM is leveraged to check the
queue_mapping.
With no prog loaded, the tx queue selection is adhering our xps_cpus
configuration.
[vagrant@localhost ~]$ grep . /sys/class/net/tap0/queues/tx-*/xps_cpus; ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe;
/sys/class/net/tap0/queues/tx-0/xps_cpus:1
/sys/class/net/tap0/queues/tx-1/xps_cpus:2
cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.146 ms
cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.121 ms
cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
With a return 0 bpg prog, our tx queue is 0 (despite xps_cpus).
[vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello0.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.160 ms
cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.124 ms
cpu1: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
ping-4852 [000] .... 2691.633260: 0: xps (RET 0): Hello, World!
ping-4869 [001] .... 2695.753588: 0: xps (RET 0): Hello, World!
With a return 1 bpg prog, our tx queue is 1.
[vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello1.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.193 ms
cpu0: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.135 ms
cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
ping-4894 [000] .... 2710.652080: 0: xps (RET 1): Hello, World!
ping-4911 [001] .... 2714.774608: 0: xps (RET 1): Hello, World!
With a return 2 bpg prog, our tx queue is 0 (we only have 2 tx queues).
[vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello2.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=1.20 ms
cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.986 ms
cpu1: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
ping-4936 [000] .... 2729.442668: 0: xps (RET 2): Hello, World!
ping-4953 [001] .... 2733.614558: 0: xps (RET 2): Hello, World!
With a return -1 bpf prog, our tx queue selection is once again determined by
xps_cpus. Any negative return should work the same and provides a nice
mechanism to bail out or have a noop bpf prog at this hookpoint.
[vagrant@localhost ~]$ sudo ip link set dev tap0 xps obj hello_neg1.o sec hello && { ./nstxq; sudo timeout 1 cat /sys/kernel/debug/tracing/trace_pipe; }
cpu0: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.628 ms
cpu0: qm0: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
cpu1: ping: 64 bytes from 169.254.254.1: icmp_seq=1 ttl=64 time=0.322 ms
cpu1: qm1: > tap0 98 Unknown => Unknown IPv4 169.254.254.2/169.254.254.1 Len 84 Type 8 Code 0
ping-4981 [000] .... 2763.510760: 0: xps (RET -1): Hello, World!
ping-4998 [001] .... 2767.632583: 0: xps (RET -1): Hello, World!
bpf prog unloading is not yet working and neither does `ip link show` report
when an "xps" bpf prog is attached. This is my first time touching iproute2 or
rtnetlink, so it may be something obvious to those more familiar.
^ permalink raw reply
* Re: [PATCH v1 1/3] seccomp: add SECCOMP_USER_NOTIF_FLAG_CONTINUE
From: Christian Brauner @ 2019-09-19 22:22 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, Andy Lutomirski, Will Drewry, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, kafai, Song Liu, yhs,
kernel list, open list:KERNEL SELFTEST FRAMEWORK,
Network Development, bpf, Tycho Andersen, Tyler Hicks
In-Reply-To: <CAG48ez1QkJAMgTpqv4EqbDmYPPpxuB8cR=XhUAr1fHZOBY_DHg@mail.gmail.com>
On Thu, Sep 19, 2019 at 09:37:06PM +0200, Jann Horn wrote:
> On Thu, Sep 19, 2019 at 11:59 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> > This allows the seccomp notifier to continue a syscall.
> [...]
> > Recently we landed seccomp support for SECCOMP_RET_USER_NOTIF (cf. [4])
> > which enables a process (watchee) to retrieve an fd for its seccomp
> > filter. This fd can then be handed to another (usually more privileged)
> > process (watcher). The watcher will then be able to receive seccomp
> > messages about the syscalls having been performed by the watchee.
> [...]
> > This can be solved by
> > telling seccomp to resume the syscall.
> [...]
> > @@ -780,8 +783,14 @@ static void seccomp_do_user_notification(int this_syscall,
> > list_del(&n.list);
> > out:
> > mutex_unlock(&match->notify_lock);
> > +
> > + /* Userspace requests to continue the syscall. */
> > + if (flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE)
> > + return 0;
> > +
> > syscall_set_return_value(current, task_pt_regs(current),
> > err, ret);
> > + return -1;
> > }
>
> Seccomp currently expects the various seccomp return values to be
> fully ordered based on how much action the kernel should take against
> the requested syscall. Currently, the range of return values is
> basically divided into three regions: "block syscall in some way"
> (from SECCOMP_RET_KILL_PROCESS to SECCOMP_RET_USER_NOTIF), "let ptrace
> decide" (SECCOMP_RET_TRACE) and "allow" (SECCOMP_RET_LOG and
> SECCOMP_RET_ALLOW). If SECCOMP_RET_USER_NOTIF becomes able to allow
> syscalls, it will be able to override a negative decision from
> SECCOMP_RET_TRACE.
>
> In practice, that's probably not a big deal, since I'm not aware of
> anyone actually using SECCOMP_RET_TRACE for security purposes, and on
> top of that, you'd have to allow ioctl(..., SECCOMP_IOCTL_NOTIF_SEND,
> ...) and seccomp() with SECCOMP_FILTER_FLAG_NEW_LISTENER in your
> seccomp policy for this to work.
>
> More interestingly, what about the case where two
> SECCOMP_RET_USER_NOTIF filters are installed? The most recently
> installed filter takes precedence if the return values's action parts
> are the same (and this is also documented in the manpage); so if a
> container engine installs a filter that always intercepts sys_foobar()
> (and never uses SECCOMP_USER_NOTIF_FLAG_CONTINUE), and then something
> inside the container also installs a filter that always intercepts
> sys_foobar() (and always uses SECCOMP_USER_NOTIF_FLAG_CONTINUE), the
> container engine's filter will become ineffective.
Excellent point. We discussed the nested container case today.
>
> With my tendency to overcomplicate things, I'm thinking that maybe it
> might be a good idea to:
> - collect a list of all filters that returned SECCOMP_RET_USER_NOTIF,
> as well as the highest-precedence return value that was less strict
> than SECCOMP_RET_USER_NOTIF
> - sequentially send notifications to all of the
> SECCOMP_RET_USER_NOTIF filters until one doesn't return
> SECCOMP_USER_NOTIF_FLAG_CONTINUE
> - if all returned SECCOMP_USER_NOTIF_FLAG_CONTINUE, go with the
> highest-precedence return value that was less strict than
> SECCOMP_RET_USER_NOTIF, or allow if no such return value was
> encountered
>
> But perhaps, for now, it would also be enough to just expand the big
> fat warning note and tell people that if they allow the use of
> SECCOMP_IOCTL_NOTIF_SEND and SECCOMP_FILTER_FLAG_NEW_LISTENER in their
> filter, SECCOMP_RET_USER_NOTIF is bypassable. And if someone actually
> has a usecase where SECCOMP_RET_USER_NOTIF should be secure and nested
> SECCOMP_RET_USER_NOTIF support is needed, that more complicated logic
> could be added later?
Yes, I think that is the correct approach for now.
Realistically, the most useful scenario is a host-privileged supervisor
process and a user-namespaced supervised process (or to use a concrete
example, a host-privileged container manager and an unprivileged
container). Having a user-namespaced supervisor process supervising
another nested user-namespaced process is for the most part useless
because the supervisor can't do any of the interesting syscalls (e.g.
mounting block devices that are deemed safe, faking mknod() etc.). So I
expect seccomp with USER_NOTIF to be blocked just for good measure.
Also - maybe I'm wrong - the warning we added points out that this is
only safe if the supervised process can already rely on kernel (or
other) restrictions, i.e. even if an attacker overwrites pointer syscall
arguments with harmful ones the supervisor must be sure that they are
already blocked anyway. Which can be generalized to: if an unwanted
syscall goes through in _some_ way then the supervisor must be sure that
it is blocked.
Iiuc, for your specific attack all the nested attacker can do is to
never actually get the (outer) supervisor to fake the syscall for it.
A more interesting case might be where the host-privileged supervising
process wants to deny a syscall that would otherwise succeed. But if
that's the case then the outer supervisor is trying to implement a
security policy. But we explicitly point out that this is not possible
with the notifier in general.
But honestly, that is very advanced and it seems unlikely that someone
would want this. So I'd say let's just point this out.
Christian
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring
From: Florian Fainelli @ 2019-09-19 22:12 UTC (permalink / raw)
To: Jason Cobham, Iwan R Timmer
Cc: Andrew Lunn, Vivien Didelot, David S. Miller, netdev
In-Reply-To: <CAKu_b=+0=KXnT-b8L2qkUxT2jSMAJaiMBNAeSjJ3hPqZgx4PGw@mail.gmail.com>
On 9/19/19 2:30 PM, Jason Cobham wrote:
> Hi Iwan,
>
>> Hi Andrew,
>>
>> I only own a simple 5 ports switch (88E6176) which has no problem of mirroring the other ports to a single port. Except for a bandwith shortage ofcourse. While I thought I checked adding and removing ports, I seemed to forgot to check removing ingress traffic as it will now >disable mirroring egress traffic. Searching for how I can distinct ingress from egress mirroring in port_mirror_del, I saw there is a variable in the mirror struct called ingress. Which seems strange, because why is it a seperate argument to the port_mirror_add function?
>>
>> Origally I planned to be able to set the egress and ingress mirror seperatly. But in my laziness when I saw there already was a function to configure the destination port this functionality was lost.
>>
>> Because the other drivers which implemented the port_mirror_add (b53 and
>> ksz9477) also lacks additional checks to prevent new mirror filters from breaking previous ones I assumed they were not necessary.
>>
>> At least I will soon sent a new version with at least the issue of removing mirror ingress traffic fixed and the ability to define a seperate ingress and egress port.
>>
>> Regards,
>> Iwan
>
> I have a similar patch set for port mirror from a few years ago. I'd
> also like to see this functionality in mainline. One issue I ran into
> is when doing port mirror in a cross-chip dsa configuration. If the
> ingress and egress ports are on different chips, the ingress chip
> needs to set the egress to the cross-chip dsa port and the cross-chip
> egress port needs to be set appropriately. I also had the
> functionality to mirror egress from a port to a destination port.
>
> Is it appropriate to send my patch to the mailing list for review or
> should we work on this off-line?
Given that the net-next tree is closed at the moment, working offline
and posting a combined version of a patch that supports port mirroring
for cross chip configurations as well as standalone sounds good to me.
Thanks!
--
Florian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox