* Re: [V9fs-developer] [PATCH] version pointer uninitialized
From: piaojun @ 2018-07-10 1:02 UTC (permalink / raw)
To: Tomas Bortoli, ericvh, rminnich, lucho
Cc: netdev, linux-kernel, syzkaller, v9fs-developer, davem
In-Reply-To: <20180709222943.19503-1-tomasbortoli@gmail.com>
LGTM
On 2018/7/10 6:29, Tomas Bortoli wrote:
> The p9_client_version() does not initialize the version
> pointer. If the call to p9pdu_readf() returns an error and version has not
> been allocated in p9pdu_readf(), then the program will jump to the "error"
> label and will try to free the version pointer. If version is not
> initialized, free() will be called with uninitialized, garbage data and
> will provoke a crash.
>
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
Reviewed-by: Jun Piao <piaojun@huawei.com>
> ---
> net/9p/client.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 18c5271910dc..40f7c47f2f74 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -957,7 +957,7 @@ static int p9_client_version(struct p9_client *c)
> {
> int err = 0;
> struct p9_req_t *req;
> - char *version;
> + char *version = NULL;
> int msize;
>
> p9_debug(P9_DEBUG_9P, ">>> TVERSION msize %d protocol %d\n",
>
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v3 0/3] Use of unique identifier for pairing virtio and passthrough devices...
From: si-wei liu @ 2018-07-10 1:11 UTC (permalink / raw)
To: Roman Kagan, Venu Busireddy, Michael S . Tsirkin,
Marcel Apfelbaum, virtio-dev, qemu-devel
Cc: sridhar.samudrala, alexander.h.duyck, netdev
In-Reply-To: <20180709130035.GA6271@rkaganb.sw.ru>
On 7/9/2018 6:00 AM, Roman Kagan wrote:
> On Tue, Jul 03, 2018 at 03:27:23PM -0700, si-wei liu wrote:
>> On 7/3/2018 2:58 AM, Roman Kagan wrote:
>>> So how is this coordination going to work? One possibility is that the
>>> PV device emits a QMP event upon the guest driver confirming the support
>>> for failover, the management layer intercepts the event and performs
>>> device_add of the PT device. Another is that the PT device is added
>>> from the very beginning (e.g. on the QEMU command line) but its parent
>>> PCI bridge subscribes a callback with the PV device to "activate" the PT
>>> device upon negotiating the failover feature.
>>>
>>> I think this needs to be decided within the scope of this patchset.
>> As what had been discussed in previous thread below, we would go with the
>> approach that QEMU manages the visibility of the PT device automatically.
>> Management layer supplies PT device to QEMU from the very beginning. This PT
>> device won't be exposed to guest immediately, unless or until the guest
>> virtio driver acknowledges the backup feature already. Once virtio driver in
>> the guest initiates a device reset, the corresponding PT device must be
>> taken out from guest. Then add it back later on after guest virtio completes
>> negotiation for the backup feature.
> This means that the parent bridge of the PT device (or whatever else can
> control the visibility of the PT device to the guest) will need to
> cooperate with the PV device *within* QEMU. The most natural way to
> specify this connection is to have a property of one device to refer to
> the other by device-id.
This scheme has the problem that one device has to depend on the
presence of the other - QEMU has the implication of the enumeration
order if the two are not placed in the same birdge or PCI hierarchy. You
can't get it reliably working if the bridge is going to be realized
while the dependent PV device hadn't been yet, or vice versa.
> Another benefit of this approach is that it will allow to hide the
> (possibly transport-specific) device matching identifiers from the QEMU
> caller, as it won't need to be persistent nor visible to the management
> layer. In particular, this will allow to move forward with the
> implementation of this PT-PV cooperation while the discussion of the
> matching scheme is still ongoing, because matching by MAC will certainly
> work as a first approximation.
The plan is to enable group ID based matching in the first place rather
than match by MAC, the latter of which is fragile and problematic. I
have made the Linux side changes and will get it posted once the QEMU
discussion for grouping is finalized.
>
>>>>> Is the guest supposed to signal the datapath switch to the host?
>>>> No, guest doesn't need to be initiating datapath switch at all.
>>> What happens if the guest supports failover in its PV driver, but lacks
>>> the driver for the PT device?
>> The assumption of failover driver is that the primary (PT device) will be
>> able to get a datapath once it shows up in the guest .
> I wonder how universal this assumption is, given the variety of possible
> network configurations, including filters, VLANs, etc. For whatever
> reason Hyper-V defines a control message over the PV device from guest
> to host for that.
These scenarios are different than the case with no driver support at
all within guest. I think I particularly raised this as part of doing
proper error handling when reviewing the original virtio failover patch
- if failover module fails to enslave the VF due to guest network
configurations, it has to signal virtio-net to propagate the error back
to host. One way to handle that is to have virtio-net kick out a device
reset and clear the feature bit upon re-negotiation, such that VF will
be plugged out and won't get plugged in. I don't know for what reason
the patch submitter did not incorporate that change. But it's in our
plan to enhance that part, no worries.
>> If adding a PT device
>> to an unsupported guest, the result will be same as that without a standby
>> PV driver - basically got no networking as you don't get a working driver.
>>
>> Then perhaps don't add the PT device in the first place if guest lacks
>> driver support?
> You don't know this in advance.
From migration point of view, it does not matter if guest lacks driver
support for VF. I like to avoid duplicating hyper-v concept if at all
possible. What makes sense with Hyper-V's accelerated networking doesn't
have to work with KVM/QEMU SR-IOV live migration. Are you sure that the
Hyper-V control message was added for this sole purpose? Seems to me an
overkill for such an edge scenario.
>
>>>> However, QMP
>>>> events may be generated when exposing or hiding the PT device through hot
>>>> plug/unplug to facilitate host to switch datapath.
>>> The PT device hot plug/unplug are initiated by the host, aren't they? Why
>>> would it also need QMP events for them?
>> As indicated above, the hot plug/unplug are initiated by QEMU not the
>> management layer. Hence the QMP hot plug event is used as an indicator to
>> switch host datapath. Unlike Windows Hyper-V SR-IOV driver model, the Linux
>> host network stack does not offer a fine grained PF driver API to move
>> MAC/VLAN filter, and the VF driver has to start with some initial MAC
>> address filter programmed in when present in the guest. The QMP event is
>> served as a checkpoint to switch MAC filter and/or VLAN filter between the
>> PV and the VF.
>>
> I'd appreciate something like a sequence diagram to better understand
> the whole picture...
>
>>>>> Is the scheme going to be applied/extended to other transports (vmbus,
>>>>> virtio-ccw, etc.)?
>>>> Well, it depends on the use case, and how feasible it can be extended to
>>>> other transport due to constraints and transport specifics.
>>>>
>>>>> Is the failover group concept going to be used beyond PT-PV network
>>>>> device failover?
>>>> Although the concept of failover group is generic, the implementation itself
>>>> may vary.
>>> My point with these two questions is that since this patchset is
>>> defining external interfaces -- with guest OS, with management layer --
>>> which are not easy to change later, it might make sense to try and see
>>> if the interfaces map to other usecases. E.g. I think we can get enough
>>> information on how Hyper-V handles PT-PV network device failover from
>>> the current Linux implementation; it may be a good idea to share some
>>> concepts and workflows with virtio-pci.
>> As you may see from above, the handshake of virtio failover depends on hot
>> plug (PCI or ACPI) and virtio specifics (feature negotiation). So far as I
>> see the Hyper-V uses a completely different handshake protocol of its own
>> (guest initiated datapath switch, Serial number in VMBus PCI bridge) than
>> that of virtio. I can barely imagine how code could be implemented in a
>> shared manner, although I agree conceptually failover group between these
>> two is similar or the same.
> I actually think there must be a lot in common: the way for the
> management layer to specify the binding between the PT and PV devices;
> the overall sequence of state transitions of every component, the QMP
> events and the points in time when they are emitted, the way to adjust
> host-side network configuration and the time when to do it, and so on.
> It's unfortunate that the implementation of PV-PT failover in guest
> Linux happens to have diverged between virtio and hyperv, but I don't
> see any fundamental difference and I wouldn't be surprised if they
> eventually converged sooner rather than later.
(loop in Intel folks and Linux netdev)
Actually it's not without reason that Linux/virtio has to diverge from
Hyper-V. The Hyper-V SR-IOV driver model allows VF be plugged in and
registered with the stack without a MAC address filter programmed in the
NIC, while Windows Hyper-V at the host side is able to move a MAC filter
around from the NIC PV backend to VF upon receiving the DATAPATH_SWITCH
control message initiated by guest. Windows NDIS has
OID_RECEIVE_FILTER_MOVE_FILTER API to have PF update the MAC filter for
the VF without involving guest or VF. For all of these there are no
equivalent in the Linux SR-IOV driver model. How do you propose to have
guest initiate the datapath switching at any point in time when you're
dealing with Linux host network stack?
One may say we can plug in a VF with a random MAC filter programmed in
prior, and initially use that random MAC within guest. This would require:
a) not relying on permanent MAC address to do pairing during the initial
discovery, e.g. use the failover group ID as in this discussion
b) host to toggle the MAC address filter: which includes taking down the
tap device to return the MAC back to PF, followed by assigning that MAC
to VF using "ip link ... set vf ..."
c) notify guest to reload/reset VF driver for the change of hardware MAC
address
d) until VF reloads the driver it won't be able to use the datapath, so
very short period of network outage is (still) expected
But as you see this still diverges from the Hyper-V model. What do we
buy for using a random address during initial discovery and requiring VF
to complete the handshake? Less network downtime during datapath
switching? Sorry but that's not a key factor at all for our main goal -
live migration.
Regards,
-Siwei
>
> There are a few things that need to be specific for PT and/or PV
> transport, the matching identifier among them, but I guess a lot can
> still be in common.
>
> Roman.
>
^ permalink raw reply
* Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()
From: piaojun @ 2018-07-10 1:27 UTC (permalink / raw)
To: Tomas Bortoli, ericvh, rminnich, lucho
Cc: netdev, linux-kernel, syzkaller, v9fs-developer, davem
In-Reply-To: <20180709192651.28095-1-tomasbortoli@gmail.com>
Hi Tomas,
It looks like pdu->size should always be greater than pdu->offset, right?
My question may be very easy for you, please help explaining.
Thanks,
Jun
On 2018/7/10 3:26, Tomas Bortoli wrote:
> The pdu_read() function suffers from an integer underflow.
> When pdu->offset is greater than pdu->size, the length calculation will have
> a wrong result, resulting in an out-of-bound read.
> This patch modifies also pdu_write() in the same way to prevent the same
> issue from happening there and for consistency.
>
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
> ---
> net/9p/protocol.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> index 931ea00c4fed..f1e2425f920b 100644
> --- a/net/9p/protocol.c
> +++ b/net/9p/protocol.c
> @@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
>
> size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
> {
> - size_t len = min(pdu->size - pdu->offset, size);
> - memcpy(data, &pdu->sdata[pdu->offset], len);
> + size_t len = pdu->offset > pdu->size ? 0 :
> + min(pdu->size - pdu->offset, size);
> + if (len != 0)
> + memcpy(data, &pdu->sdata[pdu->offset], len);
> pdu->offset += len;
> return size - len;
> }
>
> static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
> {
> - size_t len = min(pdu->capacity - pdu->size, size);
> - memcpy(&pdu->sdata[pdu->size], data, len);
> + size_t len = pdu->size > pdu->capacity ? 0 :
> + min(pdu->capacity - pdu->size, size);
> + if (len != 0)
> + memcpy(&pdu->sdata[pdu->size], data, len);
> pdu->size += len;
> return size - len;
> }
>
^ permalink raw reply
* Re: [V9fs-developer] [PATCH] p9_check_errors() validate PDU length
From: piaojun @ 2018-07-10 1:31 UTC (permalink / raw)
To: Tomas Bortoli, ericvh, rminnich, lucho
Cc: netdev, linux-kernel, syzkaller, v9fs-developer, davem
In-Reply-To: <20180709224323.20597-1-tomasbortoli@gmail.com>
Hi Tomas,
On 2018/7/10 6:43, Tomas Bortoli wrote:
> p9_check_errors() does not validate the size of the PDU read
> in p9_parse_header(). Any size can be passed, provoking out-of-bound reads.
>
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
> ---
> As suggested by Dominique:
> https://lkml.org/lkml/2018/7/9/688
> Such check is not enough as it will prevent to read more than how it has
> been allocated but it won't prevent to read more than how it has been read
> So this patch will require some more changes to prevent bad sizes.
>
> net/9p/client.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 40f7c47f2f74..5b161b576b8a 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -520,10 +520,13 @@ EXPORT_SYMBOL(p9_parse_header);
> static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
> {
> int8_t type;
> + int32_t size;
> int err;
> int ecode;
>
> - err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
> + err = p9_parse_header(req->rc, &size, &type, NULL, 0);
> + if (size > req->rc->capacity)
> + return -EINVAL;
I think we should print some error logs here to remind uppper users.
Thanks,
Jun
> /*
> * dump the response from server
> * This should be after check errors which poplulate pdu_fcall.
>
^ permalink raw reply
* Re: [PATCH bpf-next v2 12/12] tools: bpftool: allow reuse of maps with bpftool prog load
From: Jakub Kicinski @ 2018-07-10 1:38 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: daniel, oss-drivers, netdev
In-Reply-To: <20180709194816.fmoohwnwfax5gxe6@ast-mbp.dhcp.thefacebook.com>
On Mon, 9 Jul 2018 12:48:20 -0700, Alexei Starovoitov wrote:
> On Mon, Jul 09, 2018 at 10:59:44AM -0700, Jakub Kicinski wrote:
> > Add map parameter to prog load which will allow reuse of existing
> > maps instead of creating new ones.
> >
> > We need feature detection and compat code for reallocarray, since
> > it's not available in many libc versions.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
>
> cmdline interface feels a bit awkward to use, but it's a nice improvement.
> Acked-by: Alexei Starovoitov <ast@kernel.org>
Thanks, what about the cmdline feels awkward? The syntax or having to
manipulate map reuse at cmdline level?
> any plans to extend bpf_map_def similar to iproute2 ?
> so things like pinned file name and map reuse can be specified in .c file
> instead of cmdline?
TBH for my purposes (testing, showcasing) being able to modify things
from command line is more convenient than baking such info in ELF files.
No plans to extend bpf_map_def at this point :(
^ permalink raw reply
* Re: [PATCH 0/5] m68k: IO Fixes and Cleanups
From: Greg Ungerer @ 2018-07-10 1:48 UTC (permalink / raw)
To: Geert Uytterhoeven, Dmitry Torokhov, Helge Deller
Cc: linux-m68k, netdev, linux-input, linux-kernel
In-Reply-To: <20180709093040.23597-1-geert@linux-m68k.org>
Hi Geert,
On 09/07/18 19:30, Geert Uytterhoeven wrote:
> Hi all,
>
> This patch series contains fixes and cleanups for I/O accessors on m68k
> platforms (with MMU).
>
> The first patch contains small fixes without any dependencies.
> Patches 2 and 3 make small adjustments to drivers that are dependencies
> for further cleanup.
> Patch 4 and 5 complete the cleanup.
>
> Changes compared to v1:
> - Move ARCH_HAS_IOREMAP_WT to fix "ioremap_wt redefined" warnings with
> m5475evb defconfig,
> - Add Acked-by.
>
> Given the dependencies, I think it's easiest if the respective
> maintainers would provide their Acked-by, so all patches can go in
> through the m68k tree.
Retested on ColdFire 5475, looks good.
For the whole series:
Acked-by: Greg Ungerer <gerg@linux-m68k.org>
Regards
Greg
> Thanks!
>
> Geert Uytterhoeven (5):
> m68k/io: Add missing ioremap define guards, fix typo
> net: mac8390: Use standard memcpy_{from,to}io()
> Input: hilkbd - Add casts to HP9000/300 I/O accessors
> m68k/io: Move mem*io define guards to <asm/kmap.h>
> m68k/io: Switch mmu variant to <asm-generic/io.h>
>
> arch/m68k/include/asm/io.h | 7 +++++
> arch/m68k/include/asm/io_mm.h | 42 +++--------------------------
> arch/m68k/include/asm/io_no.h | 12 ---------
> arch/m68k/include/asm/kmap.h | 9 ++++++-
> drivers/input/keyboard/hilkbd.c | 4 +--
> drivers/net/ethernet/8390/mac8390.c | 20 +++++++-------
> 6 files changed, 30 insertions(+), 64 deletions(-)
>
^ permalink raw reply
* Re: [RFC PATCH 0/1] net: phy: skip autoneg of ethernet(fec) on network boot
From: Masahiko KIMOTO @ 2018-07-10 1:52 UTC (permalink / raw)
To: andrew; +Cc: f.fainelli, fugang.duan, netdev, linux-kernel, hiraku.toyooka
In-Reply-To: <20180709144106.GJ10376@lunn.ch>
> > > I completely acknowledge and support the use case, but your
> > > implementation is definitively not the way to go. In my experience the
> > > problem is usually that there may be a disagreement on the
> > > Pause/Asym_Pause advertisement bits and that alone is responsible for
> > > triggering a re-negotiation. Can yo check if that is the case here?
> >
> > Thank you for your support.
> >
> > Concening the problem about PAUSE/Asym_PAUSE advertisement you pointed out,
> > I could not understand your consideration.
> >
> > Would you tell me more detail about it?
>
> Hi Masahiko
>
> You said that the PHY is being configured 3 times, and might be doing
> auto-neg three times. Often a phy will not actually perform auto-neg,
> or do it much faster, if the advertised bits do not change with each
> re-configuration.
>
> So check how u-boot is configuring what is advertised, and look at
> what Linux is configuring. In particularly, look at the pause
> configuration. If pause is configured different in u-boot and Linux,
> try to make it the same. Things might go faster than.
Thank you.
I'll try to check out u-boot side by mii command and Linux(server)
side by ethtool.
---------------------------------------------------------------------------
Masahiko KIMOTO,Ph.D. / Cybertrust Japan Co., Ltd.
mail: masahiko.kimoto@cybertrust.co.jp
web: https://www.cybertrust.co.jp/ https://www.miraclelinux.com/
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v3 0/3] Use of unique identifier for pairing virtio and passthrough devices...
From: Michael S. Tsirkin @ 2018-07-10 1:54 UTC (permalink / raw)
To: si-wei liu
Cc: Roman Kagan, Venu Busireddy, Marcel Apfelbaum, virtio-dev,
qemu-devel, sridhar.samudrala, alexander.h.duyck, netdev
In-Reply-To: <9136094e-a510-4201-7c71-d1c49226fa5f@oracle.com>
On Mon, Jul 09, 2018 at 06:11:53PM -0700, si-wei liu wrote:
> The plan is to enable group ID based matching in the first place rather than
> match by MAC, the latter of which is fragile and problematic.
It isn't all that fragile - hyperv used same for a while, so if someone
posts working patches with QEMU support but before this grouping stuff,
I'll happily apply them.
> I have made
> the Linux side changes and will get it posted once the QEMU discussion for
> grouping is finalized.
Go ahead but at this point I think we need to see actual QEMU support at
least for the basic functionality before we merge more code Linux-side.
--
MST
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v3 0/3] Use of unique identifier for pairing virtio and passthrough devices...
From: Michael S. Tsirkin @ 2018-07-10 1:58 UTC (permalink / raw)
To: si-wei liu
Cc: Roman Kagan, Venu Busireddy, Marcel Apfelbaum, virtio-dev,
qemu-devel, sridhar.samudrala, alexander.h.duyck, netdev
In-Reply-To: <9136094e-a510-4201-7c71-d1c49226fa5f@oracle.com>
On Mon, Jul 09, 2018 at 06:11:53PM -0700, si-wei liu wrote:
> What do we buy
> for using a random address during initial discovery and requiring VF to
> complete the handshake?
I don't see advantages to using a random address that is then
changed either: changing a MAC causes network downtime for most users.
> Less network downtime during datapath switching?
> Sorry but that's not a key factor at all for our main goal - live migration.
Isn't avoiding downtime what makes the migration "live"?
If you don't care about it at all just remove the device
and migrate without all these tricks.
--
MST
^ permalink raw reply
* [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c
From: Roman Gushchin @ 2018-07-10 1:59 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel-team, Roman Gushchin, Daniel Borkmann,
Alexei Starovoitov
Sparse shows some "incorrect type" warnings in the bpf core code.
They are caused by bad __rcu annotations:
1) bpf_prog_array_alloc() returns an __rcu pointer, which isn't true.
At that moment it's obviously an exclusive "owning" pointer,
which is valid for an infinite amount of time, so __rcu is
meaningless.
2) The progs local variable in compute_effective_progs should be
marked as __bpf too, it's a local variable, not shared with anyone
else at all. The real __rcu variable is array pointer, which should
be assigned with rcu_assign_pointer.
3) __rcu progs argument of bpf_prog_array_free() should be casted
to a simple pointer before calling kfree_rcu().
4) There is a missing rcu_dereference() annotation in
bpf_prog_array_copy_to_user().
5) old_array __rcu pointer in bpf_prog_array_copy() is used as
a "normal" non-__rcu pointer.
These changes remove the following sparse warnings:
kernel/bpf/core.c:1544:31: warning: incorrect type in return expression (different address spaces)
kernel/bpf/core.c:1544:31: expected struct bpf_prog_array [noderef] <asn:4>*
kernel/bpf/core.c:1544:31: got void *
kernel/bpf/core.c:1548:17: warning: incorrect type in return expression (different address spaces)
kernel/bpf/core.c:1548:17: expected struct bpf_prog_array [noderef] <asn:4>*
kernel/bpf/core.c:1548:17: got struct bpf_prog_array *<noident>
kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
kernel/bpf/core.c:1556:9: expected struct callback_head *head
kernel/bpf/core.c:1556:9: got struct callback_head [noderef] <asn:4>*<noident>
kernel/bpf/core.c:1629:34: warning: incorrect type in initializer (different address spaces)
kernel/bpf/core.c:1629:34: expected struct bpf_prog **prog
kernel/bpf/core.c:1629:34: got struct bpf_prog *[noderef] <asn:4>*<noident>
kernel/bpf/core.c:1653:31: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1653:31: expected struct bpf_prog **existing_prog
kernel/bpf/core.c:1653:31: got struct bpf_prog *[noderef] <asn:4>*<noident>
kernel/bpf/core.c:1681:15: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1681:15: expected struct bpf_prog_array *array
kernel/bpf/core.c:1681:15: got struct bpf_prog_array [noderef] <asn:4>*
kernel/bpf/core.c:1687:31: warning: incorrect type in assignment (different address spaces)
kernel/bpf/core.c:1687:31: expected struct bpf_prog **[assigned] existing_prog
kernel/bpf/core.c:1687:31: got struct bpf_prog *[noderef] <asn:4>*<noident>
Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 2 +-
kernel/bpf/cgroup.c | 7 +++----
kernel/bpf/core.c | 14 ++++++++------
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 8827e797ff97..943fb08d8287 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -352,7 +352,7 @@ struct bpf_prog_array {
struct bpf_prog *progs[0];
};
-struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
+struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
void bpf_prog_array_free(struct bpf_prog_array __rcu *progs);
int bpf_prog_array_length(struct bpf_prog_array __rcu *progs);
int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 3d83ee7df381..badabb0b435c 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -95,7 +95,7 @@ static int compute_effective_progs(struct cgroup *cgrp,
enum bpf_attach_type type,
struct bpf_prog_array __rcu **array)
{
- struct bpf_prog_array __rcu *progs;
+ struct bpf_prog_array *progs;
struct bpf_prog_list *pl;
struct cgroup *p = cgrp;
int cnt = 0;
@@ -120,13 +120,12 @@ static int compute_effective_progs(struct cgroup *cgrp,
&p->bpf.progs[type], node) {
if (!pl->prog)
continue;
- rcu_dereference_protected(progs, 1)->
- progs[cnt++] = pl->prog;
+ progs->progs[cnt++] = pl->prog;
}
p = cgroup_parent(p);
} while (p);
- *array = progs;
+ rcu_assign_pointer(*array, progs);
return 0;
}
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 1e5625d46414..f6e5b207a0d7 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1538,7 +1538,7 @@ static struct {
.null_prog = NULL,
};
-struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
+struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
{
if (prog_cnt)
return kzalloc(sizeof(struct bpf_prog_array) +
@@ -1550,10 +1550,11 @@ struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
void bpf_prog_array_free(struct bpf_prog_array __rcu *progs)
{
- if (!progs ||
- progs == (struct bpf_prog_array __rcu *)&empty_prog_array.hdr)
+ struct bpf_prog_array *array = rcu_access_pointer(progs);
+
+ if (!array || array == &empty_prog_array.hdr)
return;
- kfree_rcu(progs, rcu);
+ kfree_rcu(array, rcu);
}
int bpf_prog_array_length(struct bpf_prog_array __rcu *progs)
@@ -1626,7 +1627,7 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
struct bpf_prog *old_prog)
{
- struct bpf_prog **prog = progs->progs;
+ struct bpf_prog **prog = rcu_dereference(progs)->progs;
for (; *prog; prog++)
if (*prog == old_prog) {
@@ -1635,11 +1636,12 @@ void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
}
}
-int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
+int bpf_prog_array_copy(struct bpf_prog_array __rcu *__old_array,
struct bpf_prog *exclude_prog,
struct bpf_prog *include_prog,
struct bpf_prog_array **new_array)
{
+ struct bpf_prog_array *old_array = rcu_access_pointer(__old_array);
int new_prog_cnt, carry_prog_cnt = 0;
struct bpf_prog **existing_prog;
struct bpf_prog_array *array;
--
2.14.4
^ permalink raw reply related
* Re: [V9fs-developer] [PATCH] p9_check_errors() validate PDU length
From: Dominique Martinet @ 2018-07-10 2:28 UTC (permalink / raw)
To: Tomas Bortoli
Cc: ericvh, rminnich, lucho, netdev, linux-kernel, syzkaller,
v9fs-developer, davem, Al Viro
In-Reply-To: <20180709235403.GB19917@nautica>
Tomas Bortoli wrote on Tue, Jul 10, 2018:
> As suggested by Dominique:
> https://lkml.org/lkml/2018/7/9/688
> Such check is not enough as it will prevent to read more than how it has
> been allocated but it won't prevent to read more than how it has been read
> So this patch will require some more changes to prevent bad sizes.
Sorry, I'm the one who suggested to put a note after the commit message
and I didn't see it.
Let's get the proper fix right away, it's not much further.
> Also, they really need to check against the actual read size, not just
> capacity.
> For virtio/rdma, something like this ought to fix pdu->size, then
> p9_parse_header can just never overwrite it (untested but it's useless
> on its own, I'll test the full patch with the parse header change)
I actually took the time to test a bit; I had only suggested something
for virtio/rdma because I had assumed trans_fd (the socket transport
actually used by syzbot) was setting the length in the fcall, but I read
that code too fast this morning and it is not (it only sets the size in
its private struct)
Something like that ought to work for trans_fd:
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 588bf88c3305..9f3ce370c685 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -369,6 +370,7 @@ static void p9_read_work(struct work_struct *work)
*/
if ((m->req) && (m->rc.offset == m->rc.capacity)) {
p9_debug(P9_DEBUG_TRANS, "got new packet\n");
+ m->req->rc->size = m->rc.offset;
spin_lock(&m->client->lock);
if (m->req->status != REQ_STATUS_ERROR)
status = REQ_STATUS_RCVD;
---
This however gets more complicated once you start factoring in that
change I suggested about p9_parse_header not setting size (and checking
size) because trans_fd relies on it; so I'm not sure how we should
proceed.
Do you have a working 9p tcp server to test changes are valid, or are
you only working off the syzbot reproducer?
In the first place, are you willing to take the time to do that bigger
fix?
At this point I can either help you get a working setup and let you do
the rest, or just finish the bigger patch myself and add you as whatever
tag you feel comfortable with (persumably Signed-off-by)
Thanks again for starting this,
--
Dominique Martinet | Asmadeus
^ permalink raw reply related
* Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse
From: Jakub Kicinski @ 2018-07-10 2:49 UTC (permalink / raw)
To: Andrey Ignatov; +Cc: alexei.starovoitov, daniel, oss-drivers, netdev
In-Reply-To: <20180709202254.GA32302@rdna-mbp.dhcp.thefacebook.com>
On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
> > More advanced applications may want to only replace programs without
> > destroying associated maps. Allow libbpf users to achieve that.
> > Instead of always creating all of the maps at load time, expose to
> > users an API to reconstruct the map object from already existing
> > map.
> >
> > The map parameters are read from the kernel and replace the parameters
> > of the ELF map. libbpf does not restrict the map replacement, i.e.
> > the reused map does not have to be compatible with the ELF map
> > definition. We relay on the verifier for checking the compatibility
> > between maps and programs. The ELF map definition is completely
> > overwritten by the information read from the kernel, to make sure
> > libbpf's view of map object corresponds to the actual map.
>
> Thanks for working on this Jakub! I encountered this shortcoming of
> libbpf as well and was planning to fix it, but you beat me to it :)
Ah! I wish I didn't! :)
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> > ---
> > tools/lib/bpf/libbpf.c | 35 +++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 1 +
> > 2 files changed, 36 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index b653dbb266c7..c80033fe66c3 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -215,6 +215,7 @@ struct bpf_map {
> > int fd;
> > char *name;
> > size_t offset;
> > + bool fd_preset;
>
> Any reason not to use map->fd itself to identify if fd is present?
Note: pre-set, not present.
> fd of every map is set to -1 in bpf_object__init_maps() that, in turn, is
> called from __bpf_object__open():
>
> for (i = 0; i < nr_maps; i++)
> obj->maps[i].fd = -1;
>
> Later it will either contain valid fd that is >= 0, or that same -1, what
> should be enough to identify fd presence.
I thought it to be cleaner to indicate the fd has been pre-set, in case
things get more complicated in the future and fd >= 0 becomes ambiguous.
But no strong preference, should I change?
> > int map_ifindex;
> > struct bpf_map_def def;
> > uint32_t btf_key_type_id;
> > @@ -1082,6 +1083,34 @@ static int bpf_map_find_btf_info(struct bpf_map *map, const struct btf *btf)
> > return 0;
> > }
> >
> > +int bpf_map__reuse_fd(struct bpf_map *map, int fd)
> > +{
> > + struct bpf_map_info info = {};
> > + __u32 len = sizeof(info);
> > + int err;
> > +
> > + err = bpf_obj_get_info_by_fd(fd, &info, &len);
> > + if (err)
> > + return err;
> > +
>
> Should there be a check that map->fd doesn't contain any valid fd (>= 0)
> before rewriting it so that if it does (e.g. because the function is
> called after bpf_object__load() by mistake), current map->fd won't be
> leaked?
Hm. In my first implementation libbpf just took the passed fd and
didn't do a dup(), the lifetime of the fd remained with the caller.
Having a check will prevent changing the descriptor unless we add some
from of "un-reuse" as well. Perhaps I should just add a close() in
case fd >= 0? Or do you prefer a hard error?
> > + map->fd = dup(fd);
>
> Unfortunately, new descriptor created by dup(2) will not have O_CLOEXEC set, in
> contrast to original fd returned by kernel on map creation.
>
> libbpf has other interface shortcomings where it comes up. E.g. struct
> bpf_object owns all descriptors it contains (progs, maps) and closes them in
> bpf_object__close(). if one wants to open/load ELF, then close it but
> keep, say, prog fd to attach it to cgroup some time later, then fd
> should be duplicated as well to get a new one not owned by bpf_object.
>
> Currently I use this workaround to avoid time when new fd doesn't have
> O_CLOEXEC:
>
> int new_prog_fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
> if (new_prog_fd < 0 ||
> dup3(bpf_program__fd(prog), new_prog_fd, O_CLOEXEC) == -1) {
> /* .. handle error .. */
> close(new_prog_fd);
> }
> /* .. use new_prog_fd with O_CLOEXEC set */
>
> Not sure how to simplify it. dup2() has same problem with regard to
> O_CLOEXEC.
>
> Use-case: standalone server application that uses libbpf and does
> fork()/execve() a lot.
Good point! I have no better ideas. Although being slightly paranoid
I would perhaps use "/" instead of "/dev/null"? Shouldn't matter?
> > + if (map->fd < 0)
> > + return map->fd;
> > + map->fd_preset = true;
> > +
> > + free(map->name);
> > + map->name = strdup(info.name);
> > + map->def.type = info.type;
> > + map->def.key_size = info.key_size;
> > + map->def.value_size = info.value_size;
> > + map->def.max_entries = info.max_entries;
> > + map->def.map_flags = info.map_flags;
> > + map->btf_key_type_id = info.btf_key_type_id;
> > + map->btf_value_type_id = info.btf_value_type_id;
> > +
> > + return 0;
> > +}
> > +
> > static int
> > bpf_object__create_maps(struct bpf_object *obj)
> > {
> > @@ -1094,6 +1123,12 @@ bpf_object__create_maps(struct bpf_object *obj)
> > struct bpf_map_def *def = &map->def;
> > int *pfd = &map->fd;
> >
> > + if (map->fd_preset) {
> > + pr_debug("skip map create (preset) %s: fd=%d\n",
> > + map->name, map->fd);
> > + continue;
> > + }
> > +
> > create_attr.name = map->name;
> > create_attr.map_ifindex = map->map_ifindex;
> > create_attr.map_type = def->type;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 60593ac44700..8e709a74f47c 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -261,6 +261,7 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
> > int bpf_map__set_priv(struct bpf_map *map, void *priv,
> > bpf_map_clear_priv_t clear_priv);
> > void *bpf_map__priv(struct bpf_map *map);
> > +int bpf_map__reuse_fd(struct bpf_map *map, int fd);
> > bool bpf_map__is_offload_neutral(struct bpf_map *map);
> > void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex);
> > int bpf_map__pin(struct bpf_map *map, const char *path);
> > --
> > 2.17.1
> >
>
^ permalink raw reply
* Re: [PATCH bpf-next v3 2/3] bpf: btf: add btf print functionality
From: Jakub Kicinski @ 2018-07-10 3:56 UTC (permalink / raw)
To: Okash Khawaja
Cc: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
In-Reply-To: <20180708203336.433372958@fb.com>
On Sun, 8 Jul 2018 13:30:04 -0700, Okash Khawaja wrote:
> This consumes functionality exported in the previous patch. It does the
> main job of printing with BTF data. This is used in the following patch
> to provide a more readable output of a map's dump. It relies on
> json_writer to do json printing. Below is sample output where map keys
> are ints and values are of type struct A:
>
> typedef int int_type;
> enum E {
> E0,
> E1,
> };
>
> struct B {
> int x;
> int y;
> };
>
> struct A {
> int m;
> unsigned long long n;
> char o;
> int p[8];
> int q[4][8];
> enum E r;
> void *s;
> struct B t;
> const int u;
> int_type v;
> unsigned int w1: 3;
> unsigned int w2: 3;
> };
>
> $ sudo bpftool map dump id 14
> [{
> "key": 0,
> "value": {
> "m": 1,
> "n": 2,
> "o": "c",
> "p": [15,16,17,18,15,16,17,18
> ],
> "q": [[25,26,27,28,25,26,27,28
> ],[35,36,37,38,35,36,37,38
> ],[45,46,47,48,45,46,47,48
> ],[55,56,57,58,55,56,57,58
> ]
> ],
> "r": 1,
> "s": 0x7ffd80531cf8,
> "t": {
> "x": 5,
> "y": 10
> },
> "u": 100,
> "v": 20,
> "w1": 0x7,
> "w2": 0x3
> }
> }
> ]
>
> This patch uses json's {} and [] to imply struct/union and array. More
> explicit information can be added later. For example, a command line
> option can be introduced to print whether a key or value is struct
> or union, name of a struct etc. This will however come at the expense
> of duplicating info when, for example, printing an array of structs.
> enums are printed as ints without their names.
>
> Signed-off-by: Okash Khawaja <osk@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> ---
> tools/bpf/bpftool/btf_dumper.c | 253 +++++++++++++++++++++++++++++++++++++++++
> tools/bpf/bpftool/main.h | 15 ++
> 2 files changed, 268 insertions(+)
>
> --- /dev/null
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018 Facebook */
> +
> +#include <linux/btf.h>
> +#include <linux/err.h>
> +#include <stdio.h> /* for (FILE *) used by json_writer */
> +#include <linux/bitops.h>
> +#include <string.h>
> +#include <ctype.h>
fwiw: the preferred ordering would have been:
#include <ctype.h>
#include <stdio.h> /* for (FILE *) used by json_writer */
#include <string.h>
#include <linux/bitops.h>
#include <linux/btf.h>
#include <linux/err.h>
> +#include "btf.h"
> +#include "json_writer.h"
> +#include "main.h"
> +
> +#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
> +#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
> +#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> +#define BITS_ROUNDUP_BYTES(bits) \
> + (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> +const int one = 1;
> +#define is_big_endian() ((*(char *)&one) == 0)
Could we try to do this at compilation time? Without the variable? :(
#include <asm/byteorder.h>
#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
return true;
#else
return false;
#endif
We could also just include endian.h, but since it's a non-standard
extension perhaps using kernel header is a safer bet.
> +static int btf_dumper_do_type(const struct btf_dumper *d, __u32 type_id,
> + __u8 bit_offset, const void *data);
> +
> +static void btf_dumper_ptr(const void *data, json_writer_t *jw,
> + bool is_plain_text)
> +{
> + if (is_plain_text)
> + jsonw_printf(jw, "%p", *((unsigned long *)data));
> + else
> + jsonw_printf(jw, "%u", *((unsigned long *)data));
nit: I think you missed these parenthesis
> +}
> +
> +static void btf_dumper_int_bits(__u32 int_type, __u8 bit_offset,
> + const void *data, json_writer_t *jw,
> + bool is_plain_text)
> +{
> + int left_shift_bits, right_shift_bits;
> + int nr_bits = BTF_INT_BITS(int_type);
> + int total_bits_offset;
> + int bytes_to_copy;
> + int bits_to_copy;
> + __u64 print_num;
> +
> + total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
> + data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
> + bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
> + bits_to_copy = bit_offset + nr_bits;
> + bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
> +
> + print_num = 0;
> + memcpy(&print_num, data, bytes_to_copy);
> + if (is_big_endian()) {
> + left_shift_bits = bit_offset;
> + right_shift_bits = 64 - nr_bits;
> + } else {
> + left_shift_bits = 64 - bits_to_copy;
> + right_shift_bits = 64 - nr_bits;
> + }
Or you can just put the #if here, since it's the only use.
> + print_num <<= left_shift_bits;
> + print_num >>= right_shift_bits;
> + if (is_plain_text)
> + jsonw_printf(jw, "0x%llx", print_num);
> + else
> + jsonw_printf(jw, "%llu", print_num);
> +}
> +
> +static int btf_dumper_int(const struct btf_type *t, __u8 bit_offset,
> + const void *data, json_writer_t *jw,
> + bool is_plain_text)
> +{
> + __u32 *int_type;
> + __u32 nr_bits;
> +
> + int_type = (__u32 *)(t + 1);
> + nr_bits = BTF_INT_BITS(*int_type);
> + /* if this is bit field */
> + if (bit_offset || BTF_INT_OFFSET(*int_type) ||
> + BITS_PER_BYTE_MASKED(nr_bits)) {
> + btf_dumper_int_bits(*int_type, bit_offset, data, jw,
> + is_plain_text);
> + return 0;
> + }
> +
> + switch (BTF_INT_ENCODING(*int_type)) {
> + case 0:
> + if (BTF_INT_BITS(*int_type) == 64)
> + jsonw_printf(jw, "%lu", *((__u64 *)data));
nit: more parenthesis here
> + else if (BTF_INT_BITS(*int_type) == 32)
> + jsonw_printf(jw, "%u", *((__u32 *)data));
> + else if (BTF_INT_BITS(*int_type) == 16)
> + jsonw_printf(jw, "%hu", *((__u16 *)data));
> + else if (BTF_INT_BITS(*int_type) == 8)
> + jsonw_printf(jw, "%hhu", *((__u8 *)data));
> + else
> + btf_dumper_int_bits(*int_type, bit_offset, data, jw,
> + is_plain_text);
> + break;
> + case BTF_INT_SIGNED:
> + if (BTF_INT_BITS(*int_type) == 64)
> + jsonw_printf(jw, "%ld", *((long long *)data));
> + else if (BTF_INT_BITS(*int_type) == 32)
> + jsonw_printf(jw, "%d", *((int *)data));
> + else if (BTF_INT_BITS(*int_type) == 16)
> + jsonw_printf(jw, "%hd", *((short *)data));
> + else if (BTF_INT_BITS(*int_type) == 8)
> + jsonw_printf(jw, "%hhd", *((char *)data));
> + else
> + btf_dumper_int_bits(*int_type, bit_offset, data, jw,
> + is_plain_text);
> + break;
> + case BTF_INT_CHAR:
> + if (*((char *)data) == '\0')
nit: here too, etc..
> + jsonw_null(jw);
I don't think the null is good. I thought I mentioned that? Look for
example at Python:
>>> import json
>>> thing = json.loads('{"a": [97, 98, 99, 100]}')
>>> bytearray(thing["str"]).decode('utf-8')
'abcd'
>>> "".join(map(chr, thing["str"]))
'abcd'
>>> thing = json.loads('{"str": [97, 98, 99, 100, null]}')
>>> bytearray(thing["str"]).decode('utf-8')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: an integer is required
>>> "".join(map(chr, thing["str"]))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: an integer is required (got type NoneType)
If you start putting nulls into the array the conversion to a string
will become more difficult, won't it? Do you have a use case where
this helps? Maybe my Python-foo is not strong enough?
> + else if (isprint(*((char *)data)))
> + jsonw_printf(jw, "\"%c\"", *((char *)data));
> + else
> + if (is_plain_text)
> + jsonw_printf(jw, "0x%hhx", *((char *)data));
> + else
> + jsonw_printf(jw, "\"\\u00%02hhx\"",
> + *((char *)data));
> + break;
> + case BTF_INT_BOOL:
> + jsonw_bool(jw, *((int *)data));
> + break;
> + default:
> + /* shouldn't happen */
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int btf_dumper_struct(const struct btf_dumper *d, __u32 type_id,
> + const void *data)
> +{
> + const struct btf_type *t;
> + struct btf_member *m;
> + int ret = 0;
> + int i, vlen;
> +
> + t = btf__type_by_id(d->btf, type_id);
> + if (!t)
> + return -EINVAL;
> +
> + vlen = BTF_INFO_VLEN(t->info);
> + jsonw_start_object(d->jw);
> + m = (struct btf_member *)(t + 1);
> +
> + for (i = 0; i < vlen; i++) {
> + const void *data_off = data +
> + BITS_ROUNDDOWN_BYTES(m[i].offset);
nit: empty line between variable declaration and code, perhaps also
don't init inline since it doesn't fit that way?
> + jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
> + ret = btf_dumper_do_type(d, m[i].type,
> + BITS_PER_BYTE_MASKED(m[i].offset),
> + data_off);
> + if (ret)
> + break;
> + }
> +
> + jsonw_end_object(d->jw);
> +
> + return ret;
> +}
Thanks for all the changes you've made so far!
^ permalink raw reply
* Re: [PATCH bpf-next v3 3/3] bpf: btf: print map dump and lookup with btf info
From: Jakub Kicinski @ 2018-07-10 4:06 UTC (permalink / raw)
To: Okash Khawaja
Cc: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
In-Reply-To: <20180708203336.570589830@fb.com>
On Sun, 8 Jul 2018 13:30:05 -0700, Okash Khawaja wrote:
> This patch augments the output of bpftool's map dump and map lookup
> commands to print data along side btf info, if the correspondin btf
> info is available. The outputs for each of map dump and map lookup
> commands are augmented in two ways:
>
> 1. when neither of -j and -p are supplied, btf-ful map data is printed
> whose aim is human readability. This means no commitments for json- or
> backward- compatibility.
>
> 2. when either -j or -p are supplied, a new json object named
> "formatted" is added for each key-value pair. This object contains the
> same data as the key-value pair, but with btf info. "formatted" object
> promises json- and backward- compatibility. Below is a sample output.
>
> $ bpftool map dump -p id 8
> [{
> "key": ["0x0f","0x00","0x00","0x00"
> ],
> "value": ["0x03", "0x00", "0x00", "0x00", ...
> ],
> "formatted": {
> "key": 15,
> "value": {
> "int_field": 3,
> ...
> }
> }
> }
> ]
>
> This patch calls btf_dumper introduced in previous patch to accomplish
> the above. Indeed, btf-ful info is only displayed if btf data for the
> given map is available. Otherwise existing output is displayed as-is.
>
> Signed-off-by: Okash Khawaja <osk@fb.com>
> +static struct btf *get_btf(struct bpf_map_info *map_info)
> +{
> + struct bpf_btf_info btf_info = { 0 };
> + __u32 len = sizeof(btf_info);
> + struct btf *btf = NULL;
> + __u32 last_size;
> + int btf_fd;
> + void *ptr;
> + int err;
> +
> + btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
> + if (btf_fd < 0)
> + return NULL;
> +
> + /* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
> + * let's start with a sane default - 4KiB here - and resize it only if
> + * bpf_obj_get_info_by_fd() needs a bigger buffer.
> + */
> + btf_info.btf_size = 4096;
> + last_size = btf_info.btf_size;
> + ptr = malloc(last_size);
> + if (!ptr) {
> + p_err("unable to allocate memory for debug info");
> + goto exit_free;
I don't think we can continue working after a p_err() call :S
Or p_info() for that matter. Something else may call p_err() again and
we'll end up with multiple "error" members in JSON.
Could you return an error and make the callers fail where you do goto
exit_free? The case there is no BTF is okay to return NULL, but other
cases should really not happen, and I think it's OK to just error out
completely.
> + }
> +
> + bzero(ptr, last_size);
> + btf_info.btf = ptr_to_u64(ptr);
> + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> +
> + if (!err && btf_info.btf_size > last_size) {
> + void *temp_ptr;
> +
> + last_size = btf_info.btf_size;
> + temp_ptr = realloc(ptr, last_size);
> + if (!temp_ptr) {
> + p_err("unable to re-allocate memory for debug info");
> + goto exit_free;
> + }
> + ptr = temp_ptr;
> + bzero(ptr, last_size);
> + btf_info.btf = ptr_to_u64(ptr);
> + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> + }
> +
> + if (err || btf_info.btf_size > last_size) {
> + p_info("can't get btf info. debug info won't be displayed. error: %s",
> + err ? strerror(errno) : "exceeds size retry");
> + goto exit_free;
> + }
> +
> + btf = btf__new((__u8 *)btf_info.btf,
> + btf_info.btf_size, NULL);
> + if (IS_ERR(btf)) {
> + p_info("error when initialising btf: %s\n",
> + strerror(PTR_ERR(btf)));
> + btf = NULL;
> + }
> +
> +exit_free:
> + close(btf_fd);
> + free(ptr);
> +
> + return btf;
> +}
> @@ -549,9 +681,18 @@ static int do_dump(int argc, char **argv
>
> if (!bpf_map_lookup_elem(fd, key, value)) {
> if (json_output)
> - print_entry_json(&info, key, value);
> + print_entry_json(&info, key, value, btf);
> else
> - print_entry_plain(&info, key, value);
> + if (btf) {
> + struct btf_dumper d = {
> + .btf = btf,
> + .jw = btf_wtr,
> + .is_plain_text = true,
> + };
nit: new line missing here and in another place
> + do_dump_btf(&d, &info, key, value);
> + } else {
^ permalink raw reply
* Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse
From: Andrey Ignatov @ 2018-07-10 4:23 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: alexei.starovoitov, daniel, oss-drivers, netdev
In-Reply-To: <20180709194914.003ef519@cakuba.lan>
Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 19:49 -0700]:
> On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote:
> > Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
> > > More advanced applications may want to only replace programs without
> > > destroying associated maps. Allow libbpf users to achieve that.
> > > Instead of always creating all of the maps at load time, expose to
> > > users an API to reconstruct the map object from already existing
> > > map.
> > >
> > > The map parameters are read from the kernel and replace the parameters
> > > of the ELF map. libbpf does not restrict the map replacement, i.e.
> > > the reused map does not have to be compatible with the ELF map
> > > definition. We relay on the verifier for checking the compatibility
> > > between maps and programs. The ELF map definition is completely
> > > overwritten by the information read from the kernel, to make sure
> > > libbpf's view of map object corresponds to the actual map.
> >
> > Thanks for working on this Jakub! I encountered this shortcoming of
> > libbpf as well and was planning to fix it, but you beat me to it :)
>
> Ah! I wish I didn't! :)
>
> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> > > ---
> > > tools/lib/bpf/libbpf.c | 35 +++++++++++++++++++++++++++++++++++
> > > tools/lib/bpf/libbpf.h | 1 +
> > > 2 files changed, 36 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index b653dbb266c7..c80033fe66c3 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -215,6 +215,7 @@ struct bpf_map {
> > > int fd;
> > > char *name;
> > > size_t offset;
> > > + bool fd_preset;
> >
> > Any reason not to use map->fd itself to identify if fd is present?
>
> Note: pre-set, not present.
Oh, sorry, I'm blind :)
> > fd of every map is set to -1 in bpf_object__init_maps() that, in turn, is
> > called from __bpf_object__open():
> >
> > for (i = 0; i < nr_maps; i++)
> > obj->maps[i].fd = -1;
> >
> > Later it will either contain valid fd that is >= 0, or that same -1, what
> > should be enough to identify fd presence.
>
> I thought it to be cleaner to indicate the fd has been pre-set, in case
> things get more complicated in the future and fd >= 0 becomes ambiguous.
>
> But no strong preference, should I change?
My preference (not strong either) is to avoid a new field whenever it's
possible. Though if you have a use-case that can't be covered by
(fd >= 0) keeping the field is fine as well.
> > > int map_ifindex;
> > > struct bpf_map_def def;
> > > uint32_t btf_key_type_id;
> > > @@ -1082,6 +1083,34 @@ static int bpf_map_find_btf_info(struct bpf_map *map, const struct btf *btf)
> > > return 0;
> > > }
> > >
> > > +int bpf_map__reuse_fd(struct bpf_map *map, int fd)
> > > +{
> > > + struct bpf_map_info info = {};
> > > + __u32 len = sizeof(info);
> > > + int err;
> > > +
> > > + err = bpf_obj_get_info_by_fd(fd, &info, &len);
> > > + if (err)
> > > + return err;
> > > +
> >
> > Should there be a check that map->fd doesn't contain any valid fd (>= 0)
> > before rewriting it so that if it does (e.g. because the function is
> > called after bpf_object__load() by mistake), current map->fd won't be
> > leaked?
>
> Hm. In my first implementation libbpf just took the passed fd and
> didn't do a dup(), the lifetime of the fd remained with the caller.
> Having a check will prevent changing the descriptor unless we add some
> from of "un-reuse" as well. Perhaps I should just add a close() in
> case fd >= 0? Or do you prefer a hard error?
Agree, close() in case fd >= 0 should be fine since caller already made it
explicit that they don't care about current fd and there should not be a
reason to hard-fail.
> > > + map->fd = dup(fd);
> >
> > Unfortunately, new descriptor created by dup(2) will not have O_CLOEXEC set, in
> > contrast to original fd returned by kernel on map creation.
> >
> > libbpf has other interface shortcomings where it comes up. E.g. struct
> > bpf_object owns all descriptors it contains (progs, maps) and closes them in
> > bpf_object__close(). if one wants to open/load ELF, then close it but
> > keep, say, prog fd to attach it to cgroup some time later, then fd
> > should be duplicated as well to get a new one not owned by bpf_object.
> >
> > Currently I use this workaround to avoid time when new fd doesn't have
> > O_CLOEXEC:
> >
> > int new_prog_fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
> > if (new_prog_fd < 0 ||
> > dup3(bpf_program__fd(prog), new_prog_fd, O_CLOEXEC) == -1) {
> > /* .. handle error .. */
> > close(new_prog_fd);
> > }
> > /* .. use new_prog_fd with O_CLOEXEC set */
> >
> > Not sure how to simplify it. dup2() has same problem with regard to
> > O_CLOEXEC.
> >
> > Use-case: standalone server application that uses libbpf and does
> > fork()/execve() a lot.
>
> Good point! I have no better ideas. Although being slightly paranoid
> I would perhaps use "/" instead of "/dev/null"? Shouldn't matter?
No strong preferences, important thing is to create fd with O_CLOEXEC
set somehow.
Is it safer to use "/" than "/dev/null"? (trying to understand if I
should change my code as well)
> > > + if (map->fd < 0)
> > > + return map->fd;
> > > + map->fd_preset = true;
> > > +
> > > + free(map->name);
> > > + map->name = strdup(info.name);
> > > + map->def.type = info.type;
> > > + map->def.key_size = info.key_size;
> > > + map->def.value_size = info.value_size;
> > > + map->def.max_entries = info.max_entries;
> > > + map->def.map_flags = info.map_flags;
> > > + map->btf_key_type_id = info.btf_key_type_id;
> > > + map->btf_value_type_id = info.btf_value_type_id;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int
> > > bpf_object__create_maps(struct bpf_object *obj)
> > > {
> > > @@ -1094,6 +1123,12 @@ bpf_object__create_maps(struct bpf_object *obj)
> > > struct bpf_map_def *def = &map->def;
> > > int *pfd = &map->fd;
> > >
> > > + if (map->fd_preset) {
> > > + pr_debug("skip map create (preset) %s: fd=%d\n",
> > > + map->name, map->fd);
> > > + continue;
> > > + }
> > > +
> > > create_attr.name = map->name;
> > > create_attr.map_ifindex = map->map_ifindex;
> > > create_attr.map_type = def->type;
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 60593ac44700..8e709a74f47c 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -261,6 +261,7 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
> > > int bpf_map__set_priv(struct bpf_map *map, void *priv,
> > > bpf_map_clear_priv_t clear_priv);
> > > void *bpf_map__priv(struct bpf_map *map);
> > > +int bpf_map__reuse_fd(struct bpf_map *map, int fd);
> > > bool bpf_map__is_offload_neutral(struct bpf_map *map);
> > > void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex);
> > > int bpf_map__pin(struct bpf_map *map, const char *path);
> > > --
> > > 2.17.1
> > >
> >
>
--
Andrey Ignatov
^ permalink raw reply
* Re: [PATCH] samples/bpf: Fix tc and ip path in xdp2skb_meta.sh
From: Taeung Song @ 2018-07-10 4:28 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel
In-Reply-To: <20180709212003.10b8e490@redhat.com>
On 07/10/2018 04:20 AM, Jesper Dangaard Brouer wrote:
> On Tue, 10 Jul 2018 02:44:06 +0900
> Taeung Song <treeze.taeung@gmail.com> wrote:
>
>> Hi Jesper Dangaard Brouer,
>>
>> On 07/10/2018 12:40 AM, Jesper Dangaard Brouer wrote:
>>> On Tue, 10 Jul 2018 00:04:18 +0900
>>> Taeung Song <treeze.taeung@gmail.com> wrote:
>>>
>>>> The below path error can occur:
>>>>
>>>> # ./xdp2skb_meta.sh --dev eth0 --list
>>>> ./xdp2skb_meta.sh: line 61: /usr/sbin/tc: No such file or directory
>>>>
>>>> # which tc
>>>> /sbin/tc
>>>>
>>>> So use 'which' command instead of absolute path of tc and ip
>>>>
>>>> Fixes: 36e04a2d78d9 ("samples/bpf: xdp2skb_meta shows transferring info from XDP to SKB")
>>>> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
>>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>>> ---
>>>> samples/bpf/xdp2skb_meta.sh | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/samples/bpf/xdp2skb_meta.sh b/samples/bpf/xdp2skb_meta.sh
>>>> index b9c9549c4c27..67cf7b5f336d 100755
>>>> --- a/samples/bpf/xdp2skb_meta.sh
>>>> +++ b/samples/bpf/xdp2skb_meta.sh
>>>> @@ -16,8 +16,8 @@
>>>> BPF_FILE=xdp2skb_meta_kern.o
>>>> DIR=$(dirname $0)
>>>>
>>>> -export TC=/usr/sbin/tc
>>>> -export IP=/usr/sbin/ip
>>>> +export TC=`which tc`
>>>> +export IP=`which ip`
>>>
>>> This is not a good solution, as 'which' can return something else.
>>> E.g. on my system I've aliased 'tc' to 'sudo tc', and `which tc` returns:
>>>
>>> $ which tc
>>> alias tc='sudo tc'
>>> /usr/bin/sudo
>>>
>>> The easiest solution is to simply do:
>>>
>>> export TC=tc
>>> export IP=ip
>>>
>>> The more fancy solution is to allow callers to redefine $IP and $TC:
>>>
>>> [ -z "$TC" ] && TC=tc
>>> [ -z "$IP" ] && IP=ip
>>>
>>
>> Yep, you are right, I'll change it.
>>
>>> And then you should also fix the use of 'basename', see below patch...
>>>
>>
>> I thought it'd be fine to leave 'basename' as it is,
>> because if callers redefine TC=/home/taeung/tc and give
>> the options --verbose or --dry-run, 'basename' can more tidily show outputs.
>
> The 'basename' does not work correctly if e.g. TC='sudo tc'.
>
> Below output is with 'basename' removed, else it would say "sudo qdisc del ..."
>
> TC='sudo tc' IP='sudo ip' ./xdp2skb_meta.sh -v --dev mlx5p1 --dry-run
> # Device set to: DEV=mlx5p1
> # Dry-run mode: enable VERBOSE and don't call TC+IP
> sudo tc qdisc del dev mlx5p1 clsact
> sudo tc qdisc add dev mlx5p1 clsact
> sudo tc filter add dev mlx5p1 ingress prio 1 handle 1 bpf da obj ./xdp2skb_meta_kern.o sec tc_mark
> # Flush XDP on device: mlx5p1
> sudo ip link set dev mlx5p1 xdp off
> sudo ip link set dev mlx5p1 xdp obj ./xdp2skb_meta_kern.o sec xdp_mark
>
>
Oops, right. I missed the alias case.
>> But it seems to be trivial, I'll resend this patch as v2 based on your
>> comment !
>
> Thx, already ACKed it :-)
>
--
Thanks a lot !
Taeung
^ permalink raw reply
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
From: Joel Fernandes @ 2018-07-10 5:13 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Joel Fernandes, Alexei Starovoitov, Daniel Colascione,
Alexei Starovoitov, linux-kernel, Tim Murray, Daniel Borkmann,
netdev, fengc, paulmck
In-Reply-To: <951478560.1636.1531083278064.JavaMail.zimbra@efficios.com>
On Sun, Jul 08, 2018 at 04:54:38PM -0400, Mathieu Desnoyers wrote:
> ----- On Jul 7, 2018, at 4:33 PM, Joel Fernandes joelaf@google.com wrote:
>
> > On Fri, Jul 06, 2018 at 07:54:28PM -0700, Alexei Starovoitov wrote:
> >> On Fri, Jul 06, 2018 at 06:56:16PM -0700, Daniel Colascione wrote:
> >> > BPF_SYNCHRONIZE waits for any BPF programs active at the time of
> >> > BPF_SYNCHRONIZE to complete, allowing userspace to ensure atomicity of
> >> > RCU data structure operations with respect to active programs. For
> >> > example, userspace can update a map->map entry to point to a new map,
> >> > use BPF_SYNCHRONIZE to wait for any BPF programs using the old map to
> >> > complete, and then drain the old map without fear that BPF programs
> >> > may still be updating it.
> >> >
> >> > Signed-off-by: Daniel Colascione <dancol@google.com>
> >> > ---
> >> > include/uapi/linux/bpf.h | 1 +
> >> > kernel/bpf/syscall.c | 14 ++++++++++++++
> >> > 2 files changed, 15 insertions(+)
> >> >
> >> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >> > index b7db3261c62d..4365c50e8055 100644
> >> > --- a/include/uapi/linux/bpf.h
> >> > +++ b/include/uapi/linux/bpf.h
> >> > @@ -98,6 +98,7 @@ enum bpf_cmd {
> >> > BPF_BTF_LOAD,
> >> > BPF_BTF_GET_FD_BY_ID,
> >> > BPF_TASK_FD_QUERY,
> >> > + BPF_SYNCHRONIZE,
> >> > };
> >> >
> >> > enum bpf_map_type {
> >> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> > index d10ecd78105f..60ec7811846e 100644
> >> > --- a/kernel/bpf/syscall.c
> >> > +++ b/kernel/bpf/syscall.c
> >> > @@ -2272,6 +2272,20 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *,
> >> > uattr, unsigned int, siz
> >> > if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> >> > return -EPERM;
> >> >
> >> > + if (cmd == BPF_SYNCHRONIZE) {
> >> > + if (uattr != NULL || size != 0)
> >> > + return -EINVAL;
> >> > + err = security_bpf(cmd, NULL, 0);
> >> > + if (err < 0)
> >> > + return err;
> >> > + /* BPF programs are run with preempt disabled, so
> >> > + * synchronize_sched is sufficient even with
> >> > + * RCU_PREEMPT.
> >> > + */
> >> > + synchronize_sched();
> >> > + return 0;
> >>
> >> I don't think it's necessary. sys_membarrier() can do this already
> >> and some folks use it exactly for this use case.
> >
> > Alexei, the use of sys_membarrier for this purpose seems kind of weird to me
> > though. No where does the manpage say membarrier should be implemented this
> > way so what happens if the implementation changes?
> >
> > Further, membarrier manpage says that a memory barrier should be matched with
> > a matching barrier. In this use case there is no matching barrier, so it
> > makes it weirder.
> >
> > Lastly, sys_membarrier seems will not work on nohz-full systems, so its a bit
> > fragile to depend on it for this?
> >
> > case MEMBARRIER_CMD_GLOBAL:
> > /* MEMBARRIER_CMD_GLOBAL is not compatible with nohz_full. */
> > if (tick_nohz_full_enabled())
> > return -EINVAL;
> > if (num_online_cpus() > 1)
> > synchronize_sched();
> > return 0;
> >
> >
> > Adding Mathieu as well who I believe is author/maintainer of membarrier.
>
> See commit 907565337
> "Fix: Disable sys_membarrier when nohz_full is enabled"
>
> "Userspace applications should be allowed to expect the membarrier system
> call with MEMBARRIER_CMD_SHARED command to issue memory barriers on
> nohz_full CPUs, but synchronize_sched() does not take those into
> account."
>
> So AFAIU you'd want to re-use membarrier to issue synchronize_sched, and you
> only care about kernel preempt off critical sections.
Mathieu, Thanks a lot for your reply. I understand what you said and agree
with you. Slight OT, but I tried to go back to first principles and
understand how membarrier() uses synchronize_sched() for the "slow path" and
it didn't make immediate sense to me. Let me clarify my dillema..
My understanding is membarrier's MEMBARRIER_CMD_GLOBAL will employ
synchronize_sched to make sure all other CPUs aren't executing anymore in an
section of usercode that happen to be accessing memory that was written to
before the membarrier call was made. To do this, the system call will use
synchronize_sched to try to guarantee that all user-mode execution that
started before the membarrier call would be completed when the membarrier
call returns. This guarantees that without using a real memory barrier on the
"fast path", things work just fine and everyone wins.
But, going through RCU code, I see that a "RCU-sched quiecent state" on a CPU
may be reached when the CPU receives a timer tick while executing in user
mode:
void rcu_check_callbacks(int user)
{
trace_rcu_utilization(TPS("Start scheduler-tick"));
increment_cpu_stall_ticks();
if (user || rcu_is_cpu_rrupt_from_idle()) {
[...]
rcu_sched_qs();
rcu_bh_qs();
The problem I see is the CPU could be executing usermode code at the time of
the RCU sched-QS. This IMO is enough reason for synchronize_sched() to
return, because the CPU in question just reported a QS (assuming all other
CPUs also happen to do so if they needed to).
Then I am wondering how does the membarrier call even work, the tick could
very well have interrupted the CPU while it was executing usermode code in
the middle of a set of instructions performing memory accesses. Reporting a
quiescent state at such an inopportune time would cause the membarrier call
to prematurely return, no? Sorry if I missed something.
The other question I have is about the whole "nohz-full doesn't work" thing.
I didn't fully understand why. RCU is already tracking the state of nohz-full
CPUs because the rcu dynticks code in (kernel/rcu/tree.c) monitors
transitions to and from usermode even if the timer tick is turned off. So why
would it not work?
thanks a lot!
- Joel
^ permalink raw reply
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
From: Chenbo Feng @ 2018-07-10 5:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Colascione, mathieu.desnoyers, Joel Fernandes,
Alexei Starovoitov, linux-kernel, timmurray, Daniel Borkmann,
netdev, Lorenzo Colitti
In-Reply-To: <20180709223439.uc2a6hyic35inwye@ast-mbp.dhcp.thefacebook.com>
On Mon, Jul 9, 2018 at 3:34 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Jul 09, 2018 at 03:21:43PM -0700, Daniel Colascione wrote:
> > On Mon, Jul 9, 2018 at 3:10 PM, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > On Mon, Jul 09, 2018 at 02:36:37PM -0700, Daniel Colascione wrote:
> > >> On Mon, Jul 9, 2018 at 2:09 PM, Alexei Starovoitov
> > >> <alexei.starovoitov@gmail.com> wrote:
> > >> > On Sun, Jul 08, 2018 at 04:54:38PM -0400, Mathieu Desnoyers wrote:
> > >> >> ----- On Jul 7, 2018, at 4:33 PM, Joel Fernandes joelaf@google.com wrote:
> > >> >>
> > >> >> > On Fri, Jul 06, 2018 at 07:54:28PM -0700, Alexei Starovoitov wrote:
> > >> >> >> On Fri, Jul 06, 2018 at 06:56:16PM -0700, Daniel Colascione wrote:
> > >> >> >> > BPF_SYNCHRONIZE waits for any BPF programs active at the time of
> > >> >> >> > BPF_SYNCHRONIZE to complete, allowing userspace to ensure atomicity of
> > >> >> >> > RCU data structure operations with respect to active programs. For
> > >> >> >> > example, userspace can update a map->map entry to point to a new map,
> > >> >> >> > use BPF_SYNCHRONIZE to wait for any BPF programs using the old map to
> > >> >> >> > complete, and then drain the old map without fear that BPF programs
> > >> >> >> > may still be updating it.
> > >> >> >> >
> > >> >> >> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > >> >> >> > ---
> > >> >> >> > include/uapi/linux/bpf.h | 1 +
> > >> >> >> > kernel/bpf/syscall.c | 14 ++++++++++++++
> > >> >> >> > 2 files changed, 15 insertions(+)
> > >> >> >> >
> > >> >> >> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > >> >> >> > index b7db3261c62d..4365c50e8055 100644
> > >> >> >> > --- a/include/uapi/linux/bpf.h
> > >> >> >> > +++ b/include/uapi/linux/bpf.h
> > >> >> >> > @@ -98,6 +98,7 @@ enum bpf_cmd {
> > >> >> >> > BPF_BTF_LOAD,
> > >> >> >> > BPF_BTF_GET_FD_BY_ID,
> > >> >> >> > BPF_TASK_FD_QUERY,
> > >> >> >> > + BPF_SYNCHRONIZE,
> > >> >> >> > };
> > >> >> >> >
> > >> >> >> > enum bpf_map_type {
> > >> >> >> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > >> >> >> > index d10ecd78105f..60ec7811846e 100644
> > >> >> >> > --- a/kernel/bpf/syscall.c
> > >> >> >> > +++ b/kernel/bpf/syscall.c
> > >> >> >> > @@ -2272,6 +2272,20 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *,
> > >> >> >> > uattr, unsigned int, siz
> > >> >> >> > if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> > >> >> >> > return -EPERM;
> > >> >> >> >
> > >> >> >> > + if (cmd == BPF_SYNCHRONIZE) {
> > >> >> >> > + if (uattr != NULL || size != 0)
> > >> >> >> > + return -EINVAL;
> > >> >> >> > + err = security_bpf(cmd, NULL, 0);
> > >> >> >> > + if (err < 0)
> > >> >> >> > + return err;
> > >> >> >> > + /* BPF programs are run with preempt disabled, so
> > >> >> >> > + * synchronize_sched is sufficient even with
> > >> >> >> > + * RCU_PREEMPT.
> > >> >> >> > + */
> > >> >> >> > + synchronize_sched();
> > >> >> >> > + return 0;
> > >> >> >>
> > >> >> >> I don't think it's necessary. sys_membarrier() can do this already
> > >> >> >> and some folks use it exactly for this use case.
> > >> >> >
> > >> >> > Alexei, the use of sys_membarrier for this purpose seems kind of weird to me
> > >> >> > though. No where does the manpage say membarrier should be implemented this
> > >> >> > way so what happens if the implementation changes?
> > >> >> >
> > >> >> > Further, membarrier manpage says that a memory barrier should be matched with
> > >> >> > a matching barrier. In this use case there is no matching barrier, so it
> > >> >> > makes it weirder.
> > >> >> >
> > >> >> > Lastly, sys_membarrier seems will not work on nohz-full systems, so its a bit
> > >> >> > fragile to depend on it for this?
> > >> >> >
> > >> >> > case MEMBARRIER_CMD_GLOBAL:
> > >> >> > /* MEMBARRIER_CMD_GLOBAL is not compatible with nohz_full. */
> > >> >> > if (tick_nohz_full_enabled())
> > >> >> > return -EINVAL;
> > >> >> > if (num_online_cpus() > 1)
> > >> >> > synchronize_sched();
> > >> >> > return 0;
> > >> >> >
> > >> >> >
> > >> >> > Adding Mathieu as well who I believe is author/maintainer of membarrier.
> > >> >>
> > >> >> See commit 907565337
> > >> >> "Fix: Disable sys_membarrier when nohz_full is enabled"
> > >> >>
> > >> >> "Userspace applications should be allowed to expect the membarrier system
> > >> >> call with MEMBARRIER_CMD_SHARED command to issue memory barriers on
> > >> >> nohz_full CPUs, but synchronize_sched() does not take those into
> > >> >> account."
> > >> >>
> > >> >> So AFAIU you'd want to re-use membarrier to issue synchronize_sched, and you
> > >> >> only care about kernel preempt off critical sections.
> > >> >>
> > >> >> Clearly bpf code does not run in user-space, so it would "work".
> > >> >>
> > >> >> But the guarantees provided by membarrier are not to synchronize against
> > >> >> preempt off per se. It's just that the current implementation happens to
> > >> >> do that. The point of membarrier is to turn user-space memory barriers
> > >> >> into compiler barriers.
> > >> >>
> > >> >> If what you need is to wait for a RCU grace period for whatever RCU flavor
> > >> >> ebpf is using, I would against using membarrier for this. I would rather
> > >> >> recommend adding a dedicated BPF_SYNCHRONIZE so you won't leak
> > >> >> implementation details to user-space, *and* you can eventually change you
> > >> >> RCU implementation for e.g. SRCU in the future if needed.
> > >> >
> > >> > The point about future changes to underlying bpf mechanisms is valid.
> > >> > There is work already on the way to reduce the scope of preempt_off+rcu_lock
> > >> > that currently lasts the whole prog. We will have new prog types that won't
> > >> > have such wrappers and will do rcu_lock/unlock and preempt on/off only
> > >> > when necessary.
> > >> > So something like BPF_SYNCHRONIZE will break soon, since the kernel cannot have
> > >> > guarantees on when programs finish. Calling this command BPF_SYNCHRONIZE_PROG
> > >> > also won't make sense for the same reason.
> > >> > What we can do it instead is to define synchronization barrier for
> > >> > programs accessing maps. May be call it something like:
> > >> > BPF_SYNC_MAP_ACCESS ?
> > >>
> > >> I'm not sure what you're proposing. In the case the commit message
> > >> describes, a user-space program that wants to "drain" a map needs to
> > >> be confident that the map won't change under it, even across multiple
> > >> bpf system calls on that map. One way of doing that is to ensure that
> > >> nothing that could possibly hold a reference to that map is still
> > >> running. Are you proposing some kind of refcount-draining approach?
> > >> Simple locking won't work, since BPF programs can't block, and I don't
> > >> see right now how a simple barrier would help.
> > >
> > > I'm proposing few changes for your patch:
> > > s/BPF_SYNCHRONIZE/BPF_SYNC_MAP_ACCESS/
> > > and s/synchronize_sched/synchronize_rcu/
> > > with detailed comment in uapi/bpf.h that has an example why folks
> > > would want to use this new cmd.
> >
> > Thanks for clarifying.
> >
> > > I think the bpf maps will be rcu protected for foreseeable future
> > > even when rcu_read_lock/unlock will be done by the programs instead of
> > > kernel wrappers.
> >
> > Can we guarantee that we always obtain a map reference and dispose of
> > that reference inside the same critical section?
>
> yep. the verifier will guarantee that.
>
> > If so, can BPF
> > programs then disable preemption for as long as they'd like?
>
> you mean after the finish? no. only while running.
> The verifier will match things like lookup/release, lock/unlock, preempt on/off
> and will make sure there is no dangling preempt disable after program returns.
>
It might be a silly question but does this new bpf program type
provide a way to customize where to hold the rcu_lock/unlock and
enable/disable preemption when creating the program? Since the
BPF_SYNC_MAP_ACCESS cmd sounds not very useful if it only protect each
single map access instead of the whole program. For example, sometimes
we have to read the same map multiple times when running a eBPF
program and if the userspace changed the map value between two reads,
it may cause troubles. It would be great if we can have a RCU lock on
a block of eBPF program and in that way I think BPF_SYNC_MAP_ACCESS
cmd should be enough in handling userspace kernel racing problems.
Thanks
Chenbo Feng
^ permalink raw reply
* Re: [net-next,v2] tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy
From: Eric Dumazet @ 2018-07-10 5:03 UTC (permalink / raw)
To: Jonathan Maxwell, David Laight
Cc: Neal Cardwell, David Miller, Eric Dumazet, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Netdev, jmaxwell@redhat.com
In-Reply-To: <CAGHK07Afdys4WHEjsekSChbFVGwWVccofSS+Bk357m8=1BfJBw@mail.gmail.com>
On 07/04/2018 04:34 PM, Jonathan Maxwell wrote:
> Let's wait for Eric to review. Then I'll put together the next version.
Sorry for the delay (I was travelling last week) , please respin a v3, thanks !
^ permalink raw reply
* Re: [PATCH bpf-next v2 05/12] tools: libbpf: expose the prog type guessing from section name logic
From: Andrey Ignatov @ 2018-07-10 5:10 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: alexei.starovoitov, daniel, oss-drivers, netdev
In-Reply-To: <20180709175944.32265-6-jakub.kicinski@netronome.com>
Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
> libbpf can guess program type based on ELF section names. As libbpf
> becomes more popular its association between section name strings and
> types becomes more of a standard. Allow libbpf users to use the same
> logic for matching strings to types, e.g. when the string originates
> from command line.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> tools/lib/bpf/libbpf.c | 43 ++++++++++++++++++++++++------------------
> tools/lib/bpf/libbpf.h | 3 +++
> 2 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 38ed3e92e393..30f3e58bd563 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2081,25 +2081,33 @@ static const struct {
> #undef BPF_S_PROG_SEC
> #undef BPF_SA_PROG_SEC
>
> -static int bpf_program__identify_section(struct bpf_program *prog)
> +int libbpf_prog_type_by_string(const char *name, enum bpf_prog_type *prog_type,
> + enum bpf_attach_type *expected_attach_type)
> {
> int i;
>
> - if (!prog->section_name)
> - goto err;
> -
> - for (i = 0; i < ARRAY_SIZE(section_names); i++)
> - if (strncmp(prog->section_name, section_names[i].sec,
> - section_names[i].len) == 0)
> - return i;
> -
> -err:
> - pr_warning("failed to guess program type based on section name %s\n",
> - prog->section_name);
> + if (!name)
> + return -1;
Should it return -EINVAL? It can help in bpf_prog_load_xattr below:
err = bpf_program__identify_section(prog, &prog_type,
&expected_attach_type);
if (err < 0) {
...
return err;
}
>
> + for (i = 0; i < ARRAY_SIZE(section_names); i++) {
> + if (strncmp(name, section_names[i].sec, section_names[i].len))
> + continue;
> + *prog_type = section_names[i].prog_type;
> + *expected_attach_type = section_names[i].expected_attach_type;
> + return 0;
> + }
> return -1;
Same here.
> }
>
> +static int
> +bpf_program__identify_section(struct bpf_program *prog,
> + enum bpf_prog_type *prog_type,
> + enum bpf_attach_type *expected_attach_type)
> +{
> + return libbpf_prog_type_by_string(prog->section_name, prog_type,
> + expected_attach_type);
> +}
> +
> int bpf_map__fd(struct bpf_map *map)
> {
> return map ? map->fd : -EINVAL;
> @@ -2230,7 +2238,6 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> enum bpf_prog_type prog_type;
> struct bpf_object *obj;
> struct bpf_map *map;
> - int section_idx;
> int err;
>
> if (!attr)
> @@ -2252,14 +2259,14 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> prog->prog_ifindex = attr->ifindex;
> expected_attach_type = attr->expected_attach_type;
> if (prog_type == BPF_PROG_TYPE_UNSPEC) {
> - section_idx = bpf_program__identify_section(prog);
> - if (section_idx < 0) {
> + err = bpf_program__identify_section(prog, &prog_type,
> + &expected_attach_type);
> + if (err < 0) {
> + pr_warning("failed to guess program type based on section name %s\n",
> + prog->section_name);
> bpf_object__close(obj);
> return -EINVAL;
> }
> - prog_type = section_names[section_idx].prog_type;
> - expected_attach_type =
> - section_names[section_idx].expected_attach_type;
> }
>
> bpf_program__set_type(prog, prog_type);
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 564f4be9bae0..617dacfc6704 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -92,6 +92,9 @@ int bpf_object__set_priv(struct bpf_object *obj, void *priv,
> bpf_object_clear_priv_t clear_priv);
> void *bpf_object__priv(struct bpf_object *prog);
>
> +int libbpf_prog_type_by_string(const char *name, enum bpf_prog_type *prog_type,
Nit:
I think it should be either:
int libbpf_prog_type_by_title(const char *title, enum bpf_prog_type *prog_type,
(to be consistent with bpf_program__title())),
or:
int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
(to have function name consistent with argument name and with
bpf_program->name).
IMO "name" is better since it's used across API many times and will be
more consistent, when "title" is used just once (IMO
bpf_program__title() should have been called bpf_program__name() to be
consistent with bpf_map__name() and others, not sure if it's fine to
change now).
> + enum bpf_attach_type *expected_attach_type);
> +
> /* Accessors of bpf_program */
> struct bpf_program;
> struct bpf_program *bpf_program__next(struct bpf_program *prog,
> --
> 2.17.1
>
--
Andrey Ignatov
^ permalink raw reply
* Re: [PATCH bpf-next v2 08/12] tools: libbpf: add extended attributes version of bpf_object__open()
From: Andrey Ignatov @ 2018-07-10 5:39 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: alexei.starovoitov, daniel, oss-drivers, netdev
In-Reply-To: <20180709175944.32265-9-jakub.kicinski@netronome.com>
Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
> Similarly to bpf_prog_load() users of bpf_object__open() may need
> to specify the expected program type. Program type is needed at
> open to avoid the kernel version check for program types which don't
> require it.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> tools/lib/bpf/libbpf.c | 21 +++++++++++++++++----
> tools/lib/bpf/libbpf.h | 6 ++++++
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index edc3b0b3737d..5b0e84fbcf71 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1520,7 +1520,8 @@ __bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
> return ERR_PTR(err);
> }
>
> -struct bpf_object *bpf_object__open(const char *path)
> +struct bpf_object *bpf_object__open_xattr(const char *path,
> + struct bpf_object_open_attr *attr)
> {
> /* param validation */
> if (!path)
> @@ -1528,7 +1529,17 @@ struct bpf_object *bpf_object__open(const char *path)
>
> pr_debug("loading %s\n", path);
>
> - return __bpf_object__open(path, NULL, 0, true);
> + return __bpf_object__open(path, NULL, 0,
> + bpf_prog_type__needs_kver(attr->prog_type));
> +}
> +
> +struct bpf_object *bpf_object__open(const char *path)
> +{
> + struct bpf_object_open_attr attr = {
> + .prog_type = BPF_PROG_TYPE_UNSPEC,
> + };
> +
> + return bpf_object__open_xattr(path, &attr);
> }
>
> struct bpf_object *bpf_object__open_buffer(void *obj_buf,
> @@ -2238,6 +2249,9 @@ int bpf_prog_load(const char *file, enum bpf_prog_type type,
> int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> struct bpf_object **pobj, int *prog_fd)
> {
> + struct bpf_object_open_attr open_attr = {
> + .prog_type = attr->prog_type,
> + };
> struct bpf_program *prog, *first_prog = NULL;
> enum bpf_attach_type expected_attach_type;
> enum bpf_prog_type prog_type;
> @@ -2250,8 +2264,7 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> if (!attr->file)
> return -EINVAL;
>
> - obj = __bpf_object__open(attr->file, NULL, 0,
> - bpf_prog_type__needs_kver(attr->prog_type));
> + obj = bpf_object__open_xattr(attr->file, &open_attr);
> if (IS_ERR_OR_NULL(obj))
> return -ENOENT;
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 3122d74f2643..60593ac44700 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -66,7 +66,13 @@ void libbpf_set_print(libbpf_print_fn_t warn,
> /* Hide internal to user */
> struct bpf_object;
>
> +struct bpf_object_open_attr {
> + enum bpf_prog_type prog_type;
> +};
> +
> struct bpf_object *bpf_object__open(const char *path);
> +struct bpf_object *bpf_object__open_xattr(const char *path,
> + struct bpf_object_open_attr *attr);
Should the new bpf_object__open_xattr() API have _only_ attr argument?
Path, in turn, can become a member of attr.
That way it can be reused e.g. to load object from buffer (like
bpf_object__open_buffer() below), where path is not needed.
Otherwise, if bpf_object__open_buffer() has to be extended in the
future, another _xattr function will be needed (or caller would need to
pass NULL to path, what would make API less convenient).
> struct bpf_object *bpf_object__open_buffer(void *obj_buf,
> size_t obj_buf_sz,
> const char *name);
> --
> 2.17.1
>
--
Andrey Ignatov
^ permalink raw reply
* Re: [PATCH net-next v1 4/5] virtio_ring: add event idx support in packed ring
From: Jason Wang @ 2018-07-10 5:50 UTC (permalink / raw)
To: Tiwei Bie, mst, virtualization, linux-kernel, netdev, virtio-dev
Cc: wexu, jfreimann
In-Reply-To: <20180709072241.8886-5-tiwei.bie@intel.com>
On 2018年07月09日 15:22, Tiwei Bie wrote:
> @@ -1059,9 +1059,19 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> * suppressions. */
> virtio_mb(vq->weak_barriers);
>
> + old = vq->next_avail_idx - vq->num_added;
> + new = vq->next_avail_idx;
> + vq->num_added = 0;
> +
> snapshot = *(u32 *)vq->vring_packed.device;
I think we should use READ_ONCE() to prevent compiler from re-reading.
> + off_wrap = virtio16_to_cpu(_vq->vdev, (__virtio16)(snapshot & 0xffff));
> flags = virtio16_to_cpu(_vq->vdev, (__virtio16)(snapshot >> 16)) & 0x3;
>
> + wrap_counter = off_wrap >> 15;
> + event_idx = off_wrap & ~(1 << 15);
> + if (wrap_counter != vq->avail_wrap_counter)
> + event_idx -= vq->vring_packed.num;
Thanks
^ permalink raw reply
* Re: [PATCH net-next v1 5/5] virtio_ring: enable packed ring
From: Jason Wang @ 2018-07-10 5:51 UTC (permalink / raw)
To: Tiwei Bie, mst, virtualization, linux-kernel, netdev, virtio-dev
Cc: wexu, jfreimann
In-Reply-To: <20180709072241.8886-6-tiwei.bie@intel.com>
On 2018年07月09日 15:22, Tiwei Bie wrote:
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/s390/virtio/virtio_ccw.c | 8 ++++++++
> drivers/virtio/virtio_ring.c | 2 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 8f5c1d7f751a..ff5b85736d8d 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -765,6 +765,11 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev)
> return rc;
> }
>
> +static void ccw_transport_features(struct virtio_device *vdev)
> +{
> + __virtio_clear_bit(vdev, VIRTIO_F_RING_PACKED);
> +}
I think we need a better comment to explain why it was disabled here.
Thanks
> +
> static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> {
> struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> @@ -791,6 +796,9 @@ static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> /* Give virtio_ring a chance to accept features. */
> vring_transport_features(vdev);
>
> + /* Give virtio_ccw a chance to accept features. */
> + ccw_transport_features(vdev);
> +
> features->index = 0;
> features->features = cpu_to_le32((u32)vdev->features);
> /* Write the first half of the feature bits to the host. */
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 4b3f9e1a3cab..64f20023f088 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1968,6 +1968,8 @@ void vring_transport_features(struct virtio_device *vdev)
> break;
> case VIRTIO_F_IOMMU_PLATFORM:
> break;
> + case VIRTIO_F_RING_PACKED:
> + break;
> default:
> /* We don't understand this bit. */
> __virtio_clear_bit(vdev, i);
^ permalink raw reply
* [net-next,v3] tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy
From: Jon Maxwell @ 2018-07-10 6:51 UTC (permalink / raw)
To: davem
Cc: edumazet, eric.dumazet, ncardwell, David.Laight, kuznet, yoshfuji,
netdev, linux-kernel, jmaxwell
v3 contains the following suggestions by Neal Cardwell:
1) Fix up units mismatch regarding msec/jiffies.
2) Address possiblility of time_remaining being negative.
3) Add a helper routine tcp_clamp_rto_to_user_timeout() to do the rto
calculation.
4) Move start_ts logic into helper routine tcp_retrans_stamp() to
validate tcp_sk(sk)->retrans_stamp.
5) Some u32 declation and return refactoring.
6) Return 0 instead of false in tcp_retransmit_stamp(), it's not a bool.
Suggestions by David Laight:
1) Don't cache rto in tcp_clamp_rto_to_user_timeout().
2) Use conditional operator instead of min_t() in
tcp_clamp_rto_to_user_timeout()
Changes:
1) Call tcp_clamp_rto_to_user_timeout(sk) as an argument to
inet_csk_reset_xmit_timer() to save on rto declaration.
Every time the TCP retransmission timer fires. It checks to see if there is a
timeout before scheduling the next retransmit timer. The retransmit interval
between each retransmission increases exponentially. The issue is that in order
for the timeout to occur the retransmit timer needs to fire again. If the user
timeout check happens after the 9th retransmit for example. It needs to wait for
the 10th retransmit timer to fire in order to evaluate whether a timeout has
occurred or not. If the interval is large enough then the timeout will be
inaccurate.
For example with a TCP_USER_TIMEOUT of 10 seconds without patch:
1st retransmit:
22:25:18.973488 IP host1.49310 > host2.search-agent: Flags [.]
Last retransmit:
22:25:26.205499 IP host1.49310 > host2.search-agent: Flags [.]
Timeout:
send: Connection timed out
Sun Jul 1 22:25:34 EDT 2018
We can see that last retransmit took ~7 seconds. Which pushed the total
timeout to ~15 seconds instead of the expected 10 seconds. This gets more
inaccurate the larger the TCP_USER_TIMEOUT value. As the interval increases.
Add tcp_clamp_rto_to_user_timeout() to determine if the user rto has expired.
Or whether the rto interval needs to be recalculated. Use the original interval
if user rto is not set.
Test results with the patch is the expected 10 second timeout:
1st retransmit:
01:37:59.022555 IP host1.49310 > host2.search-agent: Flags [.]
Last retransmit:
01:38:06.486558 IP host1.49310 > host2.search-agent: Flags [.]
Timeout:
send: Connection timed out
Mon Jul 2 01:38:09 EDT 2018
Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
---
net/ipv4/tcp_timer.c | 49 +++++++++++++++++++++++++++++++++++++++----------
1 file changed, 39 insertions(+), 10 deletions(-)
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 3b3611729928..93239e58776d 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -22,6 +22,38 @@
#include <linux/gfp.h>
#include <net/tcp.h>
+u32 tcp_retransmit_stamp(struct sock *sk)
+{
+ u32 start_ts = tcp_sk(sk)->retrans_stamp;
+
+ if (unlikely(!start_ts)) {
+ struct sk_buff *head = tcp_rtx_queue_head(sk);
+
+ if (!head)
+ return 0;
+ start_ts = tcp_skb_timestamp(head);
+ }
+ return start_ts;
+}
+
+static __u32 tcp_clamp_rto_to_user_timeout(struct sock *sk)
+{
+ struct inet_connection_sock *icsk = inet_csk(sk);
+ __u32 elapsed, user_timeout;
+ u32 start_ts;
+
+ start_ts = tcp_retransmit_stamp(sk);
+ if (!icsk->icsk_user_timeout || !start_ts)
+ return icsk->icsk_rto;
+ elapsed = tcp_time_stamp(tcp_sk(sk)) - start_ts;
+ user_timeout = jiffies_to_msecs(icsk->icsk_user_timeout);
+ if (elapsed >= user_timeout)
+ return 1; /* user timeout has passed; fire ASAP */
+ else
+ return (icsk->icsk_rto < msecs_to_jiffies(user_timeout - elapsed)) ?
+ icsk->icsk_rto : msecs_to_jiffies(user_timeout - elapsed);
+}
+
/**
* tcp_write_err() - close socket and save error info
* @sk: The socket the error has appeared on.
@@ -161,19 +193,15 @@ static bool retransmits_timed_out(struct sock *sk,
unsigned int timeout)
{
const unsigned int rto_base = TCP_RTO_MIN;
- unsigned int linear_backoff_thresh, start_ts;
+ unsigned int linear_backoff_thresh;
+ u32 start_ts;
if (!inet_csk(sk)->icsk_retransmits)
return false;
- start_ts = tcp_sk(sk)->retrans_stamp;
- if (unlikely(!start_ts)) {
- struct sk_buff *head = tcp_rtx_queue_head(sk);
-
- if (!head)
- return false;
- start_ts = tcp_skb_timestamp(head);
- }
+ start_ts = tcp_retransmit_stamp(sk);
+ if (!start_ts)
+ return false;
if (likely(timeout == 0)) {
linear_backoff_thresh = ilog2(TCP_RTO_MAX/rto_base);
@@ -535,7 +563,8 @@ void tcp_retransmit_timer(struct sock *sk)
/* Use normal (exponential) backoff */
icsk->icsk_rto = min(icsk->icsk_rto << 1, TCP_RTO_MAX);
}
- inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, icsk->icsk_rto, TCP_RTO_MAX);
+ inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
+ tcp_clamp_rto_to_user_timeout(sk), TCP_RTO_MAX);
if (retransmits_timed_out(sk, net->ipv4.sysctl_tcp_retries1 + 1, 0))
__sk_dst_reset(sk);
--
2.13.6
^ permalink raw reply related
* [PATCH net-next 0/3] mlxsw: ERSPAN: Take LACP state into consideration
From: Ido Schimmel @ 2018-07-10 7:02 UTC (permalink / raw)
To: netdev; +Cc: davem, petrm, jiri, mlxsw, Ido Schimmel
Petr says:
When offloading mirror-to-gretap, mlxsw needs to preroute the path that
the encapsulated packet will take. That path may include a LAG device
above a front panel port. So far, mlxsw resolved the path to the first
up front panel slave of the LAG interface, but that only reflects
administrative state of the port. It neglects to consider whether the
port actually has a carrier, and what the LACP state is. This patch set
aims to address these problems.
Patch #1 publishes team_port_get_rcu().
Then in patch #2, a new function is introduced,
mlxsw_sp_port_dev_check(). That returns, for a given netdevice that is a
slave of a LAG device, whether that device is "txable", i.e. whether the
LAG master would send traffic through it. Since there's no good place to
put LAG-wide helpers, introduce a new header include/net/lag.h.
Finally in patch #3, fix the slave selection logic to take into
consideration whether a given slave has a carrier and whether it is
txable.
Petr Machata (3):
team: Publish team_port_get_rcu()
net: Add lag.h, net_lag_port_dev_txable()
mlxsw: spectrum_span: Change LAG lower selection
drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c | 5 ++++-
drivers/net/team/team.c | 5 -----
include/linux/if_team.h | 18 ++++++++++++++++++
include/net/bonding.h | 13 +++++++++++++
include/net/lag.h | 17 +++++++++++++++++
5 files changed, 52 insertions(+), 6 deletions(-)
create mode 100644 include/net/lag.h
--
2.14.4
^ 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