Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-09-01 16:17 UTC (permalink / raw)
  To: Jason Wang
  Cc: Michael S. Tsirkin, Koichiro Den, virtualization,
	Network Development
In-Reply-To: <CAF=yD-JREJqpsRdCi31qo1QNFEE2DDCaTGYw1f1waPgtov8WEg@mail.gmail.com>

>>> This is not a 50/50 split, which impliesTw that some packets from the
>>> large
>>> packet flow are still converted to copying. Without the change the rate
>>> without queue was 80k zerocopy vs 80k copy, so this choice of
>>> (vq->num >> 2) appears too conservative.
>>>
>>> However, testing with (vq->num >> 1) was not as effective at mitigating
>>> stalls. I did not save that data, unfortunately. Can run more tests on
>>> fine
>>> tuning this variable, if the idea sounds good.
>>
>>
>> Looks like there're still two cases were left:
>
> To be clear, this patch is not intended to fix all issues. It is a small
> improvement to avoid HoL blocking due to queued zerocopy skbs.
>
> The trade-off is that reverting to copying in these cases increases
> cycle cost. I think that that is a trade-off worth making compared to
> the alternative drop in throughput. It probably would be good to be
> able to measure this without kernel instrumentation: export
> counters similar to net->tx_zcopy_err and net->tx_packets (though
> without reset to zero, as in vhost_net_tx_packet).
>
>> 1) sndbuf is not INT_MAX
>
> You mean the case where the device stalls, later zerocopy notifications
> are queued, but these are never cleaned in free_old_xmit_skbs,
> because it requires a start_xmit and by now the (only) socket is out of
> descriptors?

Typo, sorry. I meant out of sndbuf.

> A watchdog would help somewhat. With tx-napi, this case cannot occur,
> either, as free_old_xmit_skbs no longer depends on a call to start_xmit.
>
>> 2) tx napi is used for virtio-net
>
> I am not aware of any issue specific to the use of tx-napi?
>
>> 1) could be a corner case, and for 2) what your suggest here may not solve
>> the issue since it still do in order completion.
>
> Somewhat tangential, but it might also help to break the in-order
> completion processing in vhost_zerocopy_signal_used. Complete
> all descriptors between done_idx and upend_idx. done_idx should
> then only be forward to the oldest still not-completed descriptor.
>
> In the test I ran, where the oldest descriptors are held in a queue and
> all newer ones are tail-dropped, this would avoid blocking a full ring
> of completions, when only a small number (or 1) is actually delayed.
>
> Dynamic switching between copy and zerocopy using zcopy_used
> already returns completions out-of-order, so this is not a huge leap.

^ permalink raw reply

* Re: [RFC net-next 0/5] TSN: Add qdisc-based config interfaces for traffic shapers
From: Jesus Sanchez-Palencia @ 2017-09-01 16:12 UTC (permalink / raw)
  To: Richard Cochran, Vinicius Costa Gomes
  Cc: netdev, jhs, xiyou.wangcong, jiri, intel-wired-lan, andre.guedes,
	ivan.briano, boon.leong.ong
In-Reply-To: <20170901130311.v5nyx6e6gfats5dg@localhost>

Hi Richard,


On 09/01/2017 06:03 AM, Richard Cochran wrote:
> 
> I happy to see this posted.  At first glance, it seems like a step in
> the right direction.
> 
> On Thu, Aug 31, 2017 at 06:26:20PM -0700, Vinicius Costa Gomes wrote:
>>  * Time-aware shaper (802.1Qbv):
> ...
>>    S 0x01 300
>>    S 0x03 500
>>
>>    This means that there are two intervals, the first will have the gate
>>    for traffic class 0 open for 300 nanoseconds, the second will have
>>    both traffic classes open for 500 nanoseconds.
> 
> The i210 doesn't support this in HW, or does it?


No, it does not. i210 only provides support for a per-packet feature called
LaunchTime that can be used control both the fetch and the transmission time of
packets.


> 
>>  * Frame Preemption (802.1Qbu):
>>
>>    To control even further the latency, it may prove useful to signal which
>>    traffic classes are marked as preemptable. For that, 'taprio' provides the
>>    preemption command so you set each traffic class as preemptable or not:
>>
>>    $ tc qdisc (...) \
>>         preemption 0 1 1 1
> 
> Neither can the i210 preempt frames, or what am I missing?

No, it does not.

But when we started working on the shapers we decided to look ahead and try to
come up with interfaces that could cover beyond 802.1Qav. These are just some
ideas we've been prototyping here together with the 'cbs' qdisc.


> 
> The timing of this RFC is good, as I am just finishing up an RFC that
> implements time-based transmit using the i210.  I'll try and get that
> out ASAP.


Is it correct to assume you are referring to an interface for Launchtime here?


Thanks,
Jesus

^ permalink raw reply

* Re: virtio_net: ethtool supported link modes
From: Radu Rendec @ 2017-09-01 16:19 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev
In-Reply-To: <20170901183517-mutt-send-email-mst@kernel.org>

On Fri, 2017-09-01 at 18:43 +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> > Looking at the code in virtnet_set_link_ksettings, it seems the speed
> > and duplex can be set to any valid value. The driver will "remember"
> > them and report them back in virtnet_get_link_ksettings.
> > 
> > However, the supported link modes (link_modes.supported in struct
> > ethtool_link_ksettings) is always 0, indicating that no speed/duplex
> > setting is supported.
> > 
> > Does it make more sense to set (at least a few of) the supported link
> > modes, such as 10baseT_Half ... 10000baseT_Full?
> > 
> > I would expect to see consistency between what is reported in
> > link_modes.supported and what can actually be set. Could you please
> > share your opinion on this?
> 
> I would like to know more about why this is desirable.
> 
> We used not to support the modes at all, but it turned out
> some tools are confused by this: e.g. people would try to
> bond virtio with a hardware device, tools would see
> a mismatch in speed and features between bonded devices
> and get confused.
> 
> See
> 
> 	commit 16032be56c1f66770da15cb94f0eb366c37aff6e
> 	Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> 	Date:   Wed Feb 3 04:04:37 2016 +0100
> 
> 	    virtio_net: add ethtool support for set and get of settings
> 
> 
> as well as the discussion around it
> 	https://www.spinics.net/lists/netdev/msg362111.html

Thanks for pointing these out. It is much more clear now why modes
support is implemented the way it is and what the expectations are.

> If you think we need to add more hacks like this, a stronger
> motivation than "to see consistency" would be needed.

The use case behind my original question is very simple:
 * Net device is queried via ethtool for supported modes.
 * Supported modes are presented to user.
 * User can configure any of the supported modes.

This is done transparently to the net device type (driver), so it
actually makes sense for physical NICs.

This alone of course is not a good enough motivation to modify the
driver. And it can be easily addressed in user-space at the application
level by testing for the driver.

I was merely trying to avoid driver-specific workarounds (i.e. keep the
application driver agnostic) and wondered if "advertising" supported
modes through ethtool made any sense and/or would be a desirable change
from the driver perspective. I believe I have my answers now.

Thanks,
Radu

^ permalink raw reply

* [PATCH net-next] selftests: correct define in msg_zerocopy.c
From: Willem de Bruijn @ 2017-09-01 16:31 UTC (permalink / raw)
  To: netdev; +Cc: davem, ast, dmm, Willem de Bruijn

From: Willem de Bruijn <willemb@google.com>

The msg_zerocopy test defines SO_ZEROCOPY if necessary, but its value
is inconsistent with the one in asm-generic.h. Correct that.

Also convert one error to a warning. When the test is complete, report
throughput and close cleanly even if the process did not wait for all
completions.

Reported-by: Dan Melnic <dmm@fb.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 tools/testing/selftests/net/msg_zerocopy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/msg_zerocopy.c b/tools/testing/selftests/net/msg_zerocopy.c
index 448c69a8af74..40232af5b023 100644
--- a/tools/testing/selftests/net/msg_zerocopy.c
+++ b/tools/testing/selftests/net/msg_zerocopy.c
@@ -59,7 +59,7 @@
 #endif
 
 #ifndef SO_ZEROCOPY
-#define SO_ZEROCOPY	59
+#define SO_ZEROCOPY	60
 #endif
 
 #ifndef SO_EE_CODE_ZEROCOPY_COPIED
@@ -382,8 +382,8 @@ static void do_recv_remaining_completions(int fd)
 	}
 
 	if (completions < expected_completions)
-		error(1, 0, "missing notifications: %lu < %lu\n",
-		      completions, expected_completions);
+		fprintf(stderr, "missing notifications: %lu < %lu\n",
+			completions, expected_completions);
 }
 
 static void do_tx(int domain, int type, int protocol)
-- 
2.14.1.581.gf28d330327-goog

^ permalink raw reply related

* Re: [PATCH net-next] selftests: correct define in msg_zerocopy.c
From: Alexei Starovoitov @ 2017-09-01 16:33 UTC (permalink / raw)
  To: Willem de Bruijn, netdev; +Cc: davem, dmm, Willem de Bruijn
In-Reply-To: <20170901163151.106867-1-willemdebruijn.kernel@gmail.com>

On 9/1/17 9:31 AM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> The msg_zerocopy test defines SO_ZEROCOPY if necessary, but its value
> is inconsistent with the one in asm-generic.h. Correct that.
>
> Also convert one error to a warning. When the test is complete, report
> throughput and close cleanly even if the process did not wait for all
> completions.
>
> Reported-by: Dan Melnic <dmm@fb.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Thanks for quick fixes!
Acked-by: Alexei Starovoitov <ast@kernel.org>

^ permalink raw reply

* Re: [PATCH net-next] bpf: Collapse offset checks in sock_filter_is_valid_access
From: Alexei Starovoitov @ 2017-09-01 16:35 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, daniel
In-Reply-To: <1504279087-11379-1-git-send-email-dsahern@gmail.com>

On Fri, Sep 01, 2017 at 08:18:07AM -0700, David Ahern wrote:
> Make sock_filter_is_valid_access consistent with other is_valid_access
> helpers.
> 
> Requested-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: David Ahern <dsahern@gmail.com>

Acked-by: Alexei Starovoitov <ast@kernel.org>

^ permalink raw reply

* Re: [PATCH net-next 2/2] flow_dissector: Add limits for encapsulation and EH
From: Hannes Frederic Sowa @ 2017-09-01 16:35 UTC (permalink / raw)
  To: Tom Herbert; +Cc: David S . Miller, Linux Kernel Network Developers, alex.popov
In-Reply-To: <CAPDqMeq9=DrGCfkH-EdpXDGqtzzKun4UAv6T5h354VHH-TaAqg@mail.gmail.com>

Hello Tom,

Tom Herbert <tom@quantonium.net> writes:

> On Fri, Sep 1, 2017 at 6:32 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Tom Herbert <tom@quantonium.net> writes:
>>
>>> In flow dissector there are no limits to the number of nested
>>> encapsulations that might be dissected which makes for a nice DOS
>>> attack. This patch limits for dissecting nested encapsulations
>>> as well as for dissecting over extension headers.
>>
>> I was actually more referring to your patch, because the flow dissector
>> right now is not stack recursive. Your changes would make it doing
>> recursion on the stack.
>
> I don't believe those patches had any recursion.

I was wrong with stack recursion, you handle that using the
FLOW_DISSECT_RET_PROTO_AGAIN return value thus leaving the stack frame
again, sorry.

But otherwise the walk would be unlimited (based on the packet size) in
your first patchset, correct? See this malicious example:

| IP1 | UDP1 | VXLAN1 | Ethernet | IP2 | UDP2 | VXLAN2 | ...

where IP1 == IP2, UDP1 == UDP2 and VXLAN1 != VXLAN2?

Notice that because IP1 == IP2 and UDP1 == UDP2 it seems to me it would
hit the same socket again. We would be prone to overwrite vxlan id 1
with vxlan id 2 in the key thus the key would be malicious and traffic
could be injected into other tenant networks, if the encapsulated
packets within VXLAN1 could be generated by a malicious user?

I was actually not concerned about the "recursion" but merely about
updating the values to the innermost values.

>>> Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>> Signed-off-by: Tom Herbert <tom@quantonium.net>
>>> ---
>>>  net/core/flow_dissector.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 45 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>> index 5110180a3e96..1bca748de27d 100644
>>> --- a/net/core/flow_dissector.c
>>> +++ b/net/core/flow_dissector.c
>>> @@ -396,6 +396,35 @@ __skb_flow_dissect_ipv6(const struct sk_buff *skb,
>>>       key_ip->ttl = iph->hop_limit;
>>>  }
>>>
>>> +/* Maximum number of nested encapsulations that can be processed in
>>> + * __skb_flow_dissect
>>> + */
>>> +#define MAX_FLOW_DISSECT_ENCAPS      5
>>
>> I think you can exactly parse one encapsulation layer safely. This is
>> because you can only keep state on one encapsulation layer protocol
>> right now.
>>
>> For example this scenario:
>>
>> I would like to circumvent tc flower rules that filter base
>> simply construct a packet looking like:
>>
>> Ethernet|Vlan|IP|GRE|Ethernet|Vlan|
>>
>> because we don't recurse in the flow keys either, the second vlan header
>> would overwrite the information of the first one.
>>
> Flow dissector returns the inner most information it sees. If only
> information from the outermost headers is needed then the caller
> should set FLOW_DISSECTOR_F_STOP_AT_ENCAP in the flags.

Right now, flower does not do so and seems to be prone to be fooled like
that.

Additionally, flow_dissector does not update all keys to its inner most
values also, e.g. I don't see the inner packets updating
FLOW_DISSECTOR_KEY_ETH_ADDRS (e.g. with ETH_P_TEB).

To me it would make more sense to be able to specificy
FLOW_DISSECTOR_F_AFTER_FIRST_ENCAP, so that we get the first
encapsulation information with the key, do policy decision, pull headers
and recurse here.

>>> +
>>> +static bool skb_flow_dissect_encap_allowed(int *num_encaps, unsigned int *flags)
>>> +{
>>> +     ++*num_encaps;
>>> +
>>> +     if (*num_encaps >= MAX_FLOW_DISSECT_ENCAPS) {
>>> +             if (*num_encaps == MAX_FLOW_DISSECT_ENCAPS) {
>>> +                     /* Allow one more pass but ignore disregard
>>> +                      * further encapsulations
>>> +                      */
>>> +                     *flags |= FLOW_DISSECTOR_F_STOP_AT_ENCAP;
>>> +             } else {
>>> +                     /* Max encaps reached */
>>> +                     return  false;
>>> +             }
>>> +     }
>>> +
>>> +     return true;
>>> +}
>>> +
>>> +/* Maximum number of extension headers can be processed in __skb_flow_dissect
>>> + * per IPv6 packet
>>> + */
>>> +#define MAX_FLOW_DISSECT_EH  5
>>
>> I would at least allow each extension header once, DEST_OPS twice, thus
>> let's say 12? It is safe in this version because it does not consume
>> stack space anyway and doesn't update internal state.
>
> This per each IPv6 header. This patch would allow up to five
> encapsulations of IPv6/IPv6 so maximum number of EHs we would consider
> is 6*5=30.

I would still increase the limit, but also no hard feelings. The reason
I came up with 12 is that anyway each header (besides DST_OPS) is only
allowed to show up once per header (11 + 1).

I am fine that this is a per-IPv6 header limit.

>>
>>> +
>>>  /**
>>>   * __skb_flow_dissect - extract the flow_keys struct and return it
>>>   * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
>>> @@ -426,6 +455,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>>       struct flow_dissector_key_tags *key_tags;
>>>       struct flow_dissector_key_vlan *key_vlan;
>>>       enum flow_dissect_ret fdret;
>>> +     int num_eh, num_encaps = 0;
>>>       bool skip_vlan = false;
>>>       u8 ip_proto = 0;
>>>       bool ret;
>>> @@ -714,7 +744,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>>       case FLOW_DISSECT_RET_OUT_GOOD:
>>>               goto out_good;
>>>       case FLOW_DISSECT_RET_PROTO_AGAIN:
>>> -             goto proto_again;
>>> +             if (skb_flow_dissect_encap_allowed(&num_encaps, &flags))
>>> +                     goto proto_again;
>>
>> I think you should get the check to the proto_again label. In case you
>> loop to often you can `goto out_good'.
>>
> That would add an extra conditional in the common case of no
> encapsulation. Having it here means we only care about the
> encapsulation limit when there is encapsulation.

In my opinion it would make the limits easier to grasp, but no hard
feelings about that.

Thanks,
Hannes

^ permalink raw reply

* [PATCH iproute2 0/2] fix "ip link show dev ..." for NICs with many VFs
From: Michal Kubecek @ 2017-09-01 16:39 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, linux-kernel

Two of our customers recently encountered problems with processing of large
messages produced by kernel in response to "ip link show" for NICs with
many (120-128) virtual functions. While some of them have been already
addressed in recent versions of iproute2, some still persist.

Patch 1 adds check to handle the case when a message fits into the
buffer in rtnl_talk() but not into the buffer in iplink_get().

Patch 2 increases the buffer size in iplink_get() to suffice even for
NICs with 128 VFs. 

Note: after applying patch 2, patch 1 seems useless as both buffers have
the same size so that the check cannot actually trigger. However, as we
cannot guarantee they will always stay the same, I believe the check
should still be added.

Michal Kubecek (2):
  iplink: check for message truncation in iplink_get()
  iplink: double the buffer size also in iplink_get()

 ip/iplink.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.14.1

^ permalink raw reply

* [PATCH iproute2 1/2] iplink: check for message truncation in iplink_get()
From: Michal Kubecek @ 2017-09-01 16:39 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, linux-kernel
In-Reply-To: <cover.1504283683.git.mkubecek@suse.cz>

If message length exceeds maxlen argument of rtnl_talk(), it is truncated
to maxlen but unlike in the case of truncation to the length of local
buffer in rtnl_talk(), the caller doesn't get any indication of a problem.

In particular, iplink_get() passes the truncated message on and parsing it
results in various warnings and sometimes even a segfault (observed with
"ip link show dev ..." for a NIC with 125 VFs).

Handle message truncation in iplink_get() the same way as truncation in
rtnl_talk() would be handled: return an error.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 ip/iplink.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ip/iplink.c b/ip/iplink.c
index 5aff2fde38da..790e3a138bb0 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1040,6 +1040,11 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 
 	if (rtnl_talk(&rth, &req.n, &answer.n, sizeof(answer)) < 0)
 		return -2;
+	if (answer.n.nlmsg_len > sizeof(answer.buf)) {
+		fprintf(stderr, "Message truncated from %u to %lu\n",
+			answer.n.nlmsg_len, sizeof(answer.buf));
+		return -2;
+	}
 
 	if (brief)
 		print_linkinfo_brief(NULL, &answer.n, stdout, NULL);
-- 
2.14.1

^ permalink raw reply related

* [PATCH iproute2 2/2] iplink: double the buffer size also in iplink_get()
From: Michal Kubecek @ 2017-09-01 16:39 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev, linux-kernel
In-Reply-To: <cover.1504283683.git.mkubecek@suse.cz>

Commit 72b365e8e0fd ("libnetlink: Double the dump buffer size") increased
the buffer size for "ip link show" command to 32 KB to handle NICs with
large number of VFs. With "dev" filter, a different code path is taken and
iplink_get() still uses only 16 KB buffer.

The size of 32768 is not very future-proof as NICs supporting 120-128 VFs
are already in use so that single RTM_NEWLINK message in the dump can
exceed 30000 bytes. But it's what rtnl_talk() and rtnl_dump_filter_l() use
so let's be consistent. Once this proves insufficient, all three sizes
should be increased.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 ip/iplink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index 790e3a138bb0..72c347932068 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1024,7 +1024,7 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 	};
 	struct {
 		struct nlmsghdr n;
-		char buf[16384];
+		char buf[32768];
 	} answer;
 
 	if (name) {
-- 
2.14.1

^ permalink raw reply related

* Re: [RFC net-next 0/8] net: dsa: Multi-queue awareness
From: Florian Fainelli @ 2017-09-01 16:46 UTC (permalink / raw)
  To: Andrew Lunn, jiri, jhs; +Cc: netdev, davem, xiyou.wangcong, vivien.didelot
In-Reply-To: <20170901132921.GV22289@lunn.ch>

On 09/01/2017 06:29 AM, Andrew Lunn wrote:
>> I suppose that you could somehow use TC to influence how the traffic
>> from host to CPU works, but without a "CPU" port representor the
>> question is how do we get that done? If we used "eth0" we need to
>> callback into the switch driver for programming..
> 
> We need to compare how the different switches work with respect to
> QoS. Marvell switches do a lot of the classification on the ingress
> port where it determines what queue the frame should be placed in on
> the egress port. The egress port then schedules its queues.
> 
> This does not map to TC too well.
> 
>> Regarding the last patch in this series, what I would ideally to replace
>> it with is something along the lines of:
>>
>> tc bind dev sw0p0 queue 0 dev eth0 queue 16
> 
> Why do you need this? sw0p0 has 8 queues? So i assume you use TC on
> sw0p0 to place frames into these queues? The queue then gets passed
> transparently down through the conduit interface and then used by the
> tagger. I don't see why you need eth0 here? We try our best to avoid
> eth0 wherever possible, it causes confusion. So i would prefer not to
> have to use eth0 with TC commands.

Well, if you read through patch 8 maybe this is explained. The dynamic
queue selection is working fine through the use of the DSA network
device's queue being passed to the Broadcom tag. If there was just I
would agree with you, but here is the catch below.

We also have this unique (AFAICT) hardware feature called Advanced
Congestion Buffering (ACB) where the CPU Ethernet MAC can receive
congestion information from the switch queues directly, but out of band
from specifically added HW logic and signals. This is not using pause
frames.

This is useful for instance when your CPU is linking at 1Gbits/sec (or
more) internally with the switch, but you have connected external hosts
that are only 10/100 capable. When you push 1Gbits/sec of traffic
towards such hosts, you would get severe packet loss, unless you have
pause frames enabled. The problem with Pause frames within the SF2
switch design is that they are not per-flow though, so you can't resolve
from the pause frames which egress queue to backpressure. With ACB
though, you get end-to-end backpressure between say, Port 8 and Port 0.
In order for this to work though, you need the CPU MAC to be inspecting
or rather receiving congestion notification from the switch port and
queues directly.

This is why I need to define a mapping between switch (port P,queue Q)
and CPU MAC queues (Q'). In the first generation HW, we have up to 4
ports exposed, each with 8 queues, and we have 32 CPU queues, 1:1
mapping is possible. On 2nd generation hardware, same number of ports
and queues per port, but only 16 queues, so a 2:1 mapping is possible only.

If I do not establish the mapping, several problems can occur but the
most severe is that congestion notification logic gets its congestion
information from the wrong port and queue, so it can be permanently
backpressuring the CPU queue based on e.g: a disabled port. This results
in the transmit queue being disabled, and so you get the netdev watchdog
to kick in and scream.

As you can see from the last patch, I used a notifier to receive
information when DSA slave network devices are added, from which I
extract their port number and set up an internal mapping to the CPU
queues, but this is creating a layering violation since I have now the
CPU driver extracting DSA specific net_device information.

The idea behind exposing a command like the proposed "tc bind" is to let
users define the mapping as they see fit. 1:1 or 2:1 mapping is fine as
a default, but may not be satisfactory for some use cases.

Hope this helps understand the bigger picture ;)
-- 
Florian

^ permalink raw reply

* Re: [PATCH net-next 2/2] flow_dissector: Add limits for encapsulation and EH
From: Tom Herbert @ 2017-09-01 16:49 UTC (permalink / raw)
  To: Hannes Frederic Sowa
  Cc: Tom Herbert, David S . Miller, Linux Kernel Network Developers,
	alex.popov
In-Reply-To: <87tw0mjs11.fsf@stressinduktion.org>

On Fri, Sep 1, 2017 at 9:35 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hello Tom,
>
> Tom Herbert <tom@quantonium.net> writes:
>
>> On Fri, Sep 1, 2017 at 6:32 AM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>> Tom Herbert <tom@quantonium.net> writes:
>>>
>>>> In flow dissector there are no limits to the number of nested
>>>> encapsulations that might be dissected which makes for a nice DOS
>>>> attack. This patch limits for dissecting nested encapsulations
>>>> as well as for dissecting over extension headers.
>>>
>>> I was actually more referring to your patch, because the flow dissector
>>> right now is not stack recursive. Your changes would make it doing
>>> recursion on the stack.
>>
>> I don't believe those patches had any recursion.
>
> I was wrong with stack recursion, you handle that using the
> FLOW_DISSECT_RET_PROTO_AGAIN return value thus leaving the stack frame
> again, sorry.
>
> But otherwise the walk would be unlimited (based on the packet size) in
> your first patchset, correct? See this malicious example:
>
> | IP1 | UDP1 | VXLAN1 | Ethernet | IP2 | UDP2 | VXLAN2 | ...
>
Without the limits patch I subsequently proposed, yes. However, this
is true for all the other encapsulations anyway; there's is nothing
unique about UDP encapsulations in this regard (hence with the limit
patch should generally apply to all encapsulations).

> where IP1 == IP2, UDP1 == UDP2 and VXLAN1 != VXLAN2?
>
> Notice that because IP1 == IP2 and UDP1 == UDP2 it seems to me it would
> hit the same socket again. We would be prone to overwrite vxlan id 1
> with vxlan id 2 in the key thus the key would be malicious and traffic
> could be injected into other tenant networks, if the encapsulated
> packets within VXLAN1 could be generated by a malicious user?
>
This is why flow dissection is not an authoritative parsing of the
packet. It can be wrong or misleading because it doesn't have all the
context, doesn't necessarily parse the whole chain, and only returns
one set of information (for instance only one pair of IP addresses
when there may be more in a packet). It's just a best effort mechanism
that is great for computing a hash for instance. If someone is
steering a packet to a VM based on the output of flow dissector that
is a bug; the only correct way to do this is to go through the normal
receive protocol processing path.

> I was actually not concerned about the "recursion" but merely about
> updating the values to the innermost values.
>
See my previous comment about use STOP_AT_ENCAP.

Tom

>>>> Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>>> Signed-off-by: Tom Herbert <tom@quantonium.net>
>>>> ---
>>>>  net/core/flow_dissector.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---
>>>>  1 file changed, 45 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>>> index 5110180a3e96..1bca748de27d 100644
>>>> --- a/net/core/flow_dissector.c
>>>> +++ b/net/core/flow_dissector.c
>>>> @@ -396,6 +396,35 @@ __skb_flow_dissect_ipv6(const struct sk_buff *skb,
>>>>       key_ip->ttl = iph->hop_limit;
>>>>  }
>>>>
>>>> +/* Maximum number of nested encapsulations that can be processed in
>>>> + * __skb_flow_dissect
>>>> + */
>>>> +#define MAX_FLOW_DISSECT_ENCAPS      5
>>>
>>> I think you can exactly parse one encapsulation layer safely. This is
>>> because you can only keep state on one encapsulation layer protocol
>>> right now.
>>>
>>> For example this scenario:
>>>
>>> I would like to circumvent tc flower rules that filter base
>>> simply construct a packet looking like:
>>>
>>> Ethernet|Vlan|IP|GRE|Ethernet|Vlan|
>>>
>>> because we don't recurse in the flow keys either, the second vlan header
>>> would overwrite the information of the first one.
>>>
>> Flow dissector returns the inner most information it sees. If only
>> information from the outermost headers is needed then the caller
>> should set FLOW_DISSECTOR_F_STOP_AT_ENCAP in the flags.
>
> Right now, flower does not do so and seems to be prone to be fooled like
> that.
>
> Additionally, flow_dissector does not update all keys to its inner most
> values also, e.g. I don't see the inner packets updating
> FLOW_DISSECTOR_KEY_ETH_ADDRS (e.g. with ETH_P_TEB).
>
> To me it would make more sense to be able to specificy
> FLOW_DISSECTOR_F_AFTER_FIRST_ENCAP, so that we get the first
> encapsulation information with the key, do policy decision, pull headers
> and recurse here.
>
>>>> +
>>>> +static bool skb_flow_dissect_encap_allowed(int *num_encaps, unsigned int *flags)
>>>> +{
>>>> +     ++*num_encaps;
>>>> +
>>>> +     if (*num_encaps >= MAX_FLOW_DISSECT_ENCAPS) {
>>>> +             if (*num_encaps == MAX_FLOW_DISSECT_ENCAPS) {
>>>> +                     /* Allow one more pass but ignore disregard
>>>> +                      * further encapsulations
>>>> +                      */
>>>> +                     *flags |= FLOW_DISSECTOR_F_STOP_AT_ENCAP;
>>>> +             } else {
>>>> +                     /* Max encaps reached */
>>>> +                     return  false;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     return true;
>>>> +}
>>>> +
>>>> +/* Maximum number of extension headers can be processed in __skb_flow_dissect
>>>> + * per IPv6 packet
>>>> + */
>>>> +#define MAX_FLOW_DISSECT_EH  5
>>>
>>> I would at least allow each extension header once, DEST_OPS twice, thus
>>> let's say 12? It is safe in this version because it does not consume
>>> stack space anyway and doesn't update internal state.
>>
>> This per each IPv6 header. This patch would allow up to five
>> encapsulations of IPv6/IPv6 so maximum number of EHs we would consider
>> is 6*5=30.
>
> I would still increase the limit, but also no hard feelings. The reason
> I came up with 12 is that anyway each header (besides DST_OPS) is only
> allowed to show up once per header (11 + 1).
>
> I am fine that this is a per-IPv6 header limit.
>
>>>
>>>> +
>>>>  /**
>>>>   * __skb_flow_dissect - extract the flow_keys struct and return it
>>>>   * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
>>>> @@ -426,6 +455,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>>>       struct flow_dissector_key_tags *key_tags;
>>>>       struct flow_dissector_key_vlan *key_vlan;
>>>>       enum flow_dissect_ret fdret;
>>>> +     int num_eh, num_encaps = 0;
>>>>       bool skip_vlan = false;
>>>>       u8 ip_proto = 0;
>>>>       bool ret;
>>>> @@ -714,7 +744,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>>>       case FLOW_DISSECT_RET_OUT_GOOD:
>>>>               goto out_good;
>>>>       case FLOW_DISSECT_RET_PROTO_AGAIN:
>>>> -             goto proto_again;
>>>> +             if (skb_flow_dissect_encap_allowed(&num_encaps, &flags))
>>>> +                     goto proto_again;
>>>
>>> I think you should get the check to the proto_again label. In case you
>>> loop to often you can `goto out_good'.
>>>
>> That would add an extra conditional in the common case of no
>> encapsulation. Having it here means we only care about the
>> encapsulation limit when there is encapsulation.
>
> In my opinion it would make the limits easier to grasp, but no hard
> feelings about that.
>
> Thanks,
> Hannes

^ permalink raw reply

* [iproute PATCH 0/6] strlcpy() and strlcat() for iproute2
From: Phil Sutter @ 2017-09-01 16:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev

The following series adds my own implementations of strlcpy() and
strlcat() in patch 1 and changes the code to make use of them in the
following patches but the last two: Patch 5 just eliminates a line of
useless code I found while searching for potential users of the
introduced functions, patch 6 sanitizes a call to strncpy() in
misc/lnstat_util.c without using strlcpy() since lnstat is not being
linked against libutil.

I implemented both functions solely based on information in libbsd's man
pages, so they are safe to be released under the GPL.

Phil Sutter (6):
  utils: Implement strlcpy() and strlcat()
  Convert the obvious cases to strlcpy()
  Convert harmful calls to strncpy() to strlcpy()
  ipxfrm: Replace STRBUF_CAT macro with strlcat()
  tc_util: No need to terminate an snprintf'ed buffer
  lnstat_util: Make sure buffer is NUL-terminated

 genl/ctrl.c           |  2 +-
 include/utils.h       |  3 +++
 ip/ipnetns.c          |  3 +--
 ip/iproute_lwtunnel.c |  3 +--
 ip/ipvrf.c            |  5 ++---
 ip/ipxfrm.c           | 21 +++++----------------
 ip/xfrm_state.c       |  2 +-
 lib/bpf.c             |  3 +--
 lib/fs.c              |  3 +--
 lib/inet_proto.c      |  3 +--
 lib/utils.c           | 19 +++++++++++++++++++
 misc/lnstat_util.c    |  3 ++-
 misc/ss.c             |  3 +--
 tc/em_ipset.c         |  3 +--
 tc/tc_util.c          |  1 -
 15 files changed, 40 insertions(+), 37 deletions(-)

-- 
2.13.1

^ permalink raw reply

* [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: Phil Sutter @ 2017-09-01 16:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170901165256.21459-1-phil@nwl.cc>

By making use of strncpy(), both implementations are really simple so
there is no need to add libbsd as additional dependency.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/utils.h |  3 +++
 lib/utils.c     | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index f665d9001806f..9c2f9fc257fba 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -252,4 +252,7 @@ int make_path(const char *path, mode_t mode);
 char *find_cgroup2_mount(void);
 int get_command_name(const char *pid, char *comm, size_t len);
 
+size_t strlcpy(char *dst, const char *src, size_t size);
+size_t strlcat(char *dst, const char *src, size_t size);
+
 #endif /* __UTILS_H__ */
diff --git a/lib/utils.c b/lib/utils.c
index 002063075fd61..c95780e725252 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1238,3 +1238,22 @@ int get_real_family(int rtm_type, int rtm_family)
 
 	return rtm_family;
 }
+
+size_t strlcpy(char *dst, const char *src, size_t size)
+{
+	if (size) {
+		strncpy(dst, src, size - 1);
+		dst[size - 1] = '\0';
+	}
+	return strlen(src);
+}
+
+size_t strlcat(char *dst, const char *src, size_t size)
+{
+	size_t dlen = strlen(dst);
+
+	if (dlen > size)
+		return dlen + strlen(src);
+
+	return dlen + strlcpy(dst + dlen, src, size - dlen);
+}
-- 
2.13.1

^ permalink raw reply related

* [iproute PATCH 4/6] ipxfrm: Replace STRBUF_CAT macro with strlcat()
From: Phil Sutter @ 2017-09-01 16:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170901165256.21459-1-phil@nwl.cc>

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipxfrm.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c
index d5eb22e25476a..12c2f721571b6 100644
--- a/ip/ipxfrm.c
+++ b/ip/ipxfrm.c
@@ -40,17 +40,6 @@
 #include "ip_common.h"
 
 #define STRBUF_SIZE	(128)
-#define STRBUF_CAT(buf, str) \
-	do { \
-		int rest = sizeof(buf) - 1 - strlen(buf); \
-		if (rest > 0) { \
-			int len = strlen(str); \
-			if (len > rest) \
-				len = rest; \
-			strncat(buf, str, len); \
-			buf[sizeof(buf) - 1] = '\0'; \
-		} \
-	} while (0);
 
 struct xfrm_filter filter;
 
@@ -902,8 +891,8 @@ void xfrm_state_info_print(struct xfrm_usersa_info *xsinfo,
 			   prefix, title);
 
 	if (prefix)
-		STRBUF_CAT(buf, prefix);
-	STRBUF_CAT(buf, "\t");
+		strlcat(buf, prefix, sizeof(buf));
+	strlcat(buf, "\t", sizeof(buf));
 
 	fputs(buf, fp);
 	fprintf(fp, "replay-window %u ", xsinfo->replay_window);
@@ -944,7 +933,7 @@ void xfrm_state_info_print(struct xfrm_usersa_info *xsinfo,
 		char sbuf[STRBUF_SIZE];
 
 		memcpy(sbuf, buf, sizeof(sbuf));
-		STRBUF_CAT(sbuf, "sel ");
+		strlcat(sbuf, "sel ", sizeof(sbuf));
 
 		xfrm_selector_print(&xsinfo->sel, xsinfo->family, fp, sbuf);
 	}
@@ -992,8 +981,8 @@ void xfrm_policy_info_print(struct xfrm_userpolicy_info *xpinfo,
 	}
 
 	if (prefix)
-		STRBUF_CAT(buf, prefix);
-	STRBUF_CAT(buf, "\t");
+		strlcat(buf, prefix, sizeof(buf));
+	strlcat(buf, "\t", sizeof(buf));
 
 	fputs(buf, fp);
 	if (xpinfo->dir >= XFRM_POLICY_MAX) {
-- 
2.13.1

^ permalink raw reply related

* [iproute PATCH 2/6] Convert the obvious cases to strlcpy()
From: Phil Sutter @ 2017-09-01 16:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170901165256.21459-1-phil@nwl.cc>

This converts the typical idiom of manually terminating the buffer after
a call to strncpy().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipnetns.c          | 3 +--
 ip/iproute_lwtunnel.c | 3 +--
 ip/ipvrf.c            | 3 +--
 lib/bpf.c             | 3 +--
 lib/fs.c              | 3 +--
 lib/inet_proto.c      | 3 +--
 misc/ss.c             | 3 +--
 tc/em_ipset.c         | 3 +--
 8 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 9ee1fe6a51f9c..afb4978a5be7d 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -518,8 +518,7 @@ int netns_identify_pid(const char *pidstr, char *name, int len)
 
 		if ((st.st_dev == netst.st_dev) &&
 		    (st.st_ino == netst.st_ino)) {
-			strncpy(name, entry->d_name, len - 1);
-			name[len - 1] = '\0';
+			strlcpy(name, entry->d_name, len);
 		}
 	}
 	closedir(dir);
diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
index 1a3dc4d4c0ed9..4c2d3b07e3b5c 100644
--- a/ip/iproute_lwtunnel.c
+++ b/ip/iproute_lwtunnel.c
@@ -325,8 +325,7 @@ static int parse_encap_seg6(struct rtattr *rta, size_t len, int *argcp,
 				invarg("\"segs\" provided before \"mode\"\n",
 				       *argv);
 
-			strncpy(segbuf, *argv, 1024);
-			segbuf[1023] = 0;
+			strlcpy(segbuf, *argv, 1024);
 		} else if (strcmp(*argv, "hmac") == 0) {
 			NEXT_ARG();
 			if (hmac_ok++)
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index e6fad32abd956..b74c501e0970c 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -336,8 +336,7 @@ static int vrf_path(char *vpath, size_t len)
 		if (vrf)
 			*vrf = '\0';
 
-		strncpy(vpath, start, len - 1);
-		vpath[len - 1] = '\0';
+		strlcpy(vpath, start, len);
 
 		/* if vrf path is just / then return nothing */
 		if (!strcmp(vpath, "/"))
diff --git a/lib/bpf.c b/lib/bpf.c
index 0bd0a95eafe6c..c180934acc7dc 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -512,8 +512,7 @@ static const char *bpf_find_mntpt_single(unsigned long magic, char *mnt,
 
 	ret = bpf_valid_mntpt(mntpt, magic);
 	if (!ret) {
-		strncpy(mnt, mntpt, len - 1);
-		mnt[len - 1] = 0;
+		strlcpy(mnt, mntpt, len);
 		return mnt;
 	}
 
diff --git a/lib/fs.c b/lib/fs.c
index ebe05cd44e11b..86efd4ed2ed80 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -172,8 +172,7 @@ int get_command_name(const char *pid, char *comm, size_t len)
 		if (nl)
 			*nl = '\0';
 
-		strncpy(comm, name, len - 1);
-		comm[len - 1] = '\0';
+		strlcpy(comm, name, len);
 		break;
 	}
 
diff --git a/lib/inet_proto.c b/lib/inet_proto.c
index 53c029039b6d5..bdfd52fdafe5a 100644
--- a/lib/inet_proto.c
+++ b/lib/inet_proto.c
@@ -38,8 +38,7 @@ const char *inet_proto_n2a(int proto, char *buf, int len)
 			free(ncache);
 		icache = proto;
 		ncache = strdup(pe->p_name);
-		strncpy(buf, pe->p_name, len - 1);
-		buf[len - 1] = '\0';
+		strlcpy(buf, pe->p_name, len);
 		return buf;
 	}
 	snprintf(buf, len, "ipproto-%d", proto);
diff --git a/misc/ss.c b/misc/ss.c
index 2c9e80e696595..dd8dfaa4e70db 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -425,8 +425,7 @@ static void user_ent_hash_build(void)
 
 	user_ent_hash_build_init = 1;
 
-	strncpy(name, root, sizeof(name)-1);
-	name[sizeof(name)-1] = 0;
+	strlcpy(name, root, sizeof(name));
 
 	if (strlen(name) == 0 || name[strlen(name)-1] != '/')
 		strcat(name, "/");
diff --git a/tc/em_ipset.c b/tc/em_ipset.c
index b59756515d239..48b287f5ba3b2 100644
--- a/tc/em_ipset.c
+++ b/tc/em_ipset.c
@@ -145,8 +145,7 @@ get_set_byname(const char *setname, struct xt_set_info *info)
 	int res;
 
 	req.op = IP_SET_OP_GET_BYNAME;
-	strncpy(req.set.name, setname, IPSET_MAXNAMELEN);
-	req.set.name[IPSET_MAXNAMELEN - 1] = '\0';
+	strlcpy(req.set.name, setname, IPSET_MAXNAMELEN);
 	res = do_getsockopt(&req);
 	if (res != 0)
 		return -1;
-- 
2.13.1

^ permalink raw reply related

* [iproute PATCH 6/6] lnstat_util: Make sure buffer is NUL-terminated
From: Phil Sutter @ 2017-09-01 16:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170901165256.21459-1-phil@nwl.cc>

Can't use strlcpy() here since lnstat is not linked against libutil.

While being at it, fix coding style in that chunk as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/lnstat_util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/misc/lnstat_util.c b/misc/lnstat_util.c
index ec19238c24b94..c2dc42ec1ff12 100644
--- a/misc/lnstat_util.c
+++ b/misc/lnstat_util.c
@@ -150,7 +150,8 @@ static int lnstat_scan_compat_rtstat_fields(struct lnstat_file *lf)
 {
 	char buf[FGETS_BUF_SIZE];
 
-	strncpy(buf, RTSTAT_COMPAT_LINE, sizeof(buf)-1);
+	strncpy(buf, RTSTAT_COMPAT_LINE, sizeof(buf) - 1);
+	buf[sizeof(buf) - 1] = '\0';
 
 	return __lnstat_scan_fields(lf, buf);
 }
-- 
2.13.1

^ permalink raw reply related

* [iproute PATCH 5/6] tc_util: No need to terminate an snprintf'ed buffer
From: Phil Sutter @ 2017-09-01 16:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170901165256.21459-1-phil@nwl.cc>

snprintf() won't leave the buffer unterminated, so manually terminating
is not necessary here.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/tc_util.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tc/tc_util.c b/tc/tc_util.c
index 371046839ba9f..50d355046bdad 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -434,7 +434,6 @@ static const char *action_n2a(int action)
 		return "trap";
 	default:
 		snprintf(buf, 64, "%d", action);
-		buf[63] = '\0';
 		return buf;
 	}
 }
-- 
2.13.1

^ permalink raw reply related

* Re: [RFC net-next 0/5] TSN: Add qdisc-based config interfaces for traffic shapers
From: Richard Cochran @ 2017-09-01 16:53 UTC (permalink / raw)
  To: Jesus Sanchez-Palencia
  Cc: Vinicius Costa Gomes, netdev, jhs, xiyou.wangcong, jiri,
	intel-wired-lan, andre.guedes, ivan.briano, boon.leong.ong
In-Reply-To: <1519366e-5418-4dda-db00-5bf50a1c67c4@intel.com>

On Fri, Sep 01, 2017 at 09:12:17AM -0700, Jesus Sanchez-Palencia wrote:
> Is it correct to assume you are referring to an interface for Launchtime here?

Yes.

Thanks,
Richard

^ permalink raw reply

* [iproute PATCH 3/6] Convert harmful calls to strncpy() to strlcpy()
From: Phil Sutter @ 2017-09-01 16:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170901165256.21459-1-phil@nwl.cc>

This patch converts spots where manual buffer termination was missing to
strlcpy() since that does what is needed.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 genl/ctrl.c     | 2 +-
 ip/ipvrf.c      | 2 +-
 ip/xfrm_state.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/genl/ctrl.c b/genl/ctrl.c
index 6abd52582d0d3..448988eb90e2b 100644
--- a/genl/ctrl.c
+++ b/genl/ctrl.c
@@ -313,7 +313,7 @@ static int ctrl_list(int cmd, int argc, char **argv)
 
 		if (matches(*argv, "name") == 0) {
 			NEXT_ARG();
-			strncpy(d, *argv, sizeof (d) - 1);
+			strlcpy(d, *argv, sizeof(d));
 			addattr_l(nlh, 128, CTRL_ATTR_FAMILY_NAME,
 				  d, strlen(d) + 1);
 		} else if (matches(*argv, "id") == 0) {
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index b74c501e0970c..f9277e1e66bdf 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -74,7 +74,7 @@ static int vrf_identify(pid_t pid, char *name, size_t len)
 			if (end)
 				*end = '\0';
 
-			strncpy(name, vrf, len - 1);
+			strlcpy(name, vrf, len);
 			break;
 		}
 	}
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index e11c93bf1c3b5..4483fb8f71d2c 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -125,7 +125,7 @@ static int xfrm_algo_parse(struct xfrm_algo *alg, enum xfrm_attr_type_t type,
 	fprintf(stderr, "warning: ALGO-NAME/ALGO-KEYMAT values will be sent to the kernel promiscuously! (verifying them isn't implemented yet)\n");
 #endif
 
-	strncpy(alg->alg_name, name, sizeof(alg->alg_name));
+	strlcpy(alg->alg_name, name, sizeof(alg->alg_name));
 
 	if (slen > 2 && strncmp(key, "0x", 2) == 0) {
 		/* split two chars "0x" from the top */
-- 
2.13.1

^ permalink raw reply related

* [PATCH net-next 0/4] bpf: add two helpers to read perf event enabled/running time
From: Yonghong Song @ 2017-09-01 16:53 UTC (permalink / raw)
  To: peterz, rostedt, ast, daniel, netdev; +Cc: kernel-team

Hardware pmu counters are limited resources. When there are more
pmu based perf events opened than available counters, kernel will
multiplex these events so each event gets certain percentage
(but not 100%) of the pmu time. In case that multiplexing happens,
the number of samples or counter value will not reflect the
case compared to no multiplexing. This makes comparison between
different runs difficult.
                                
Typically, the number of samples or counter value should be
normalized before comparing to other experiments. The typical
normalization is done like:
  normalized_num_samples = num_samples * time_enabled / time_running
  normalized_counter_value = counter_value * time_enabled / time_running
where time_enabled is the time enabled for event and time_running is
the time running for event since last normalization.

This patch set implements two helper functions.
The helper bpf_perf_read_counter_time reads counter/time_enabled/time_running
for perf event array map. The helper bpf_perf_prog_read_time read
time_enabled/time_running for bpf prog with type BPF_PROG_TYPE_PERF_EVENT.

Yonghong Song (4):
  bpf: add helper bpf_perf_read_counter_time for perf event array map
  bpf: add a test case for helper bpf_perf_read_counter_time
  bpf: add helper bpf_perf_prog_read_time
  bpf: add a test case for helper bpf_perf_prog_read_time

 include/linux/perf_event.h                |  3 ++
 include/uapi/linux/bpf.h                  | 29 +++++++++++-
 kernel/bpf/verifier.c                     |  4 +-
 kernel/events/core.c                      |  3 +-
 kernel/trace/bpf_trace.c                  | 73 +++++++++++++++++++++++++++++--
 samples/bpf/trace_event_kern.c            |  5 +++
 samples/bpf/tracex6_kern.c                | 26 +++++++++++
 samples/bpf/tracex6_user.c                | 13 +++++-
 tools/testing/selftests/bpf/bpf_helpers.h |  7 +++
 9 files changed, 155 insertions(+), 8 deletions(-)

-- 
2.9.5

^ permalink raw reply

* [PATCH net-next 1/4] bpf: add helper bpf_perf_read_counter_time for perf event array map
From: Yonghong Song @ 2017-09-01 16:53 UTC (permalink / raw)
  To: peterz, rostedt, ast, daniel, netdev; +Cc: kernel-team
In-Reply-To: <20170901165357.465121-1-yhs@fb.com>

Hardware pmu counters are limited resources. When there are more
pmu based perf events opened than available counters, kernel will
multiplex these events so each event gets certain percentage
(but not 100%) of the pmu time. In case that multiplexing happens,
the number of samples or counter value will not reflect the
case compared to no multiplexing. This makes comparison between
different runs difficult.

Typically, the number of samples or counter value should be
normalized before comparing to other experiments. The typical
normalization is done like:
  normalized_num_samples = num_samples * time_enabled / time_running
  normalized_counter_value = counter_value * time_enabled / time_running
where time_enabled is the time enabled for event and time_running is
the time running for event since last normalization.

This patch adds helper bpf_perf_read_counter_time for kprobed based perf
event array map, to read perf counter and enabled/running time.
The enabled/running time is accumulated since the perf event open.
To achieve scaling factor between two bpf invocations, users
can can use cpu_id as the key (which is typical for perf array usage model)
to remember the previous value and do the calculation inside the
bpf program.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/perf_event.h |  2 ++
 include/uapi/linux/bpf.h   | 21 +++++++++++++++++++-
 kernel/bpf/verifier.c      |  4 +++-
 kernel/events/core.c       |  2 +-
 kernel/trace/bpf_trace.c   | 49 ++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index b14095b..7fd5e94 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -901,6 +901,8 @@ extern void perf_pmu_migrate_context(struct pmu *pmu,
 int perf_event_read_local(struct perf_event *event, u64 *value);
 extern u64 perf_event_read_value(struct perf_event *event,
 				 u64 *enabled, u64 *running);
+extern void calc_timer_values(struct perf_event *event, u64 *now,
+         u64 *enabled, u64 *running);
 
 
 struct perf_sample_data {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ba848b7..9c23bef 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -582,6 +582,14 @@ union bpf_attr {
  *	@map: pointer to sockmap to update
  *	@key: key to insert/update sock in map
  *	@flags: same flags as map update elem
+ *
+ * int bpf_perf_read_counter_time(map, flags, counter_time_buf, buf_size)
+ *     read perf event counter value and perf event enabled/running time
+ *     @map: pointer to perf_event_array map
+ *     @flags: index of event in the map or bitmask flags
+ *     @counter_time_buf: buf to fill
+ *     @buf_size: size of the counter_time_buf
+ *     Return: 0 on success or negative error code
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -638,6 +646,7 @@ union bpf_attr {
 	FN(redirect_map),		\
 	FN(sk_redirect_map),		\
 	FN(sock_map_update),		\
+	FN(perf_read_counter_time),		\
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
@@ -681,7 +690,8 @@ enum bpf_func_id {
 #define BPF_F_ZERO_CSUM_TX		(1ULL << 1)
 #define BPF_F_DONT_FRAGMENT		(1ULL << 2)
 
-/* BPF_FUNC_perf_event_output and BPF_FUNC_perf_event_read flags. */
+/* BPF_FUNC_perf_event_output, BPF_FUNC_perf_event_read and
+ * BPF_FUNC_perf_read_counter_time flags. */
 #define BPF_F_INDEX_MASK		0xffffffffULL
 #define BPF_F_CURRENT_CPU		BPF_F_INDEX_MASK
 /* BPF_FUNC_perf_event_output for sk_buff input context. */
@@ -864,4 +874,13 @@ enum {
 #define TCP_BPF_IW		1001	/* Set TCP initial congestion window */
 #define TCP_BPF_SNDCWND_CLAMP	1002	/* Set sndcwnd_clamp */
 
+struct bpf_perf_time {
+	__u64 enabled;
+	__u64 running;
+};
+struct bpf_perf_counter_time {
+	__u64 counter;
+	struct bpf_perf_time time;
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d690c7d..c4d29e3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1494,7 +1494,8 @@ static int check_map_func_compatibility(struct bpf_map *map, int func_id)
 		break;
 	case BPF_MAP_TYPE_PERF_EVENT_ARRAY:
 		if (func_id != BPF_FUNC_perf_event_read &&
-		    func_id != BPF_FUNC_perf_event_output)
+		    func_id != BPF_FUNC_perf_event_output &&
+		    func_id != BPF_FUNC_perf_read_counter_time)
 			goto error;
 		break;
 	case BPF_MAP_TYPE_STACK_TRACE:
@@ -1537,6 +1538,7 @@ static int check_map_func_compatibility(struct bpf_map *map, int func_id)
 		break;
 	case BPF_FUNC_perf_event_read:
 	case BPF_FUNC_perf_event_output:
+	case BPF_FUNC_perf_read_counter_time:
 		if (map->map_type != BPF_MAP_TYPE_PERF_EVENT_ARRAY)
 			goto error;
 		break;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8c01572..ef5c7fb 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4883,7 +4883,7 @@ static int perf_event_index(struct perf_event *event)
 	return event->pmu->event_idx(event);
 }
 
-static void calc_timer_values(struct perf_event *event,
+void calc_timer_values(struct perf_event *event,
 				u64 *now,
 				u64 *enabled,
 				u64 *running)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index dc498b6..b807b1a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -255,13 +255,13 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
 	return &bpf_trace_printk_proto;
 }
 
-BPF_CALL_2(bpf_perf_event_read, struct bpf_map *, map, u64, flags)
-{
+static __always_inline int
+get_map_perf_counter(struct bpf_map *map, u64 flags,
+		u64 *value, struct perf_event **pe) {
 	struct bpf_array *array = container_of(map, struct bpf_array, map);
 	unsigned int cpu = smp_processor_id();
 	u64 index = flags & BPF_F_INDEX_MASK;
 	struct bpf_event_entry *ee;
-	u64 value = 0;
 	int err;
 
 	if (unlikely(flags & ~(BPF_F_INDEX_MASK)))
@@ -275,7 +275,19 @@ BPF_CALL_2(bpf_perf_event_read, struct bpf_map *, map, u64, flags)
 	if (!ee)
 		return -ENOENT;
 
-	err = perf_event_read_local(ee->event, &value);
+	err = perf_event_read_local(ee->event, value);
+	if (!err && pe)
+		*pe = ee->event;
+	return err;
+}
+
+
+BPF_CALL_2(bpf_perf_event_read, struct bpf_map *, map, u64, flags)
+{
+	u64 value = 0;
+	int err;
+
+	err = get_map_perf_counter(map, flags, &value, NULL);
 	/*
 	 * this api is ugly since we miss [-22..-2] range of valid
 	 * counter values, but that's uapi
@@ -285,6 +297,23 @@ BPF_CALL_2(bpf_perf_event_read, struct bpf_map *, map, u64, flags)
 	return value;
 }
 
+BPF_CALL_4(bpf_perf_read_counter_time, struct bpf_map *, map, u64, flags,
+	struct bpf_perf_counter_time *, buf, u32, size)
+{
+	struct perf_event *pe;
+	u64 now;
+	int err;
+
+	if (unlikely(size != sizeof(struct bpf_perf_counter_time)))
+		return -EINVAL;
+	err = get_map_perf_counter(map, flags, &buf->counter, &pe);
+	if (err)
+		return err;
+
+	calc_timer_values(pe, &now, &buf->time.enabled, &buf->time.running);
+	return 0;
+}
+
 static const struct bpf_func_proto bpf_perf_event_read_proto = {
 	.func		= bpf_perf_event_read,
 	.gpl_only	= true,
@@ -293,6 +322,16 @@ static const struct bpf_func_proto bpf_perf_event_read_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+static const struct bpf_func_proto bpf_perf_read_counter_time_proto = {
+	.func		= bpf_perf_read_counter_time,
+	.gpl_only	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_CONST_MAP_PTR,
+	.arg2_type	= ARG_ANYTHING,
+	.arg3_type	= ARG_PTR_TO_UNINIT_MEM,
+	.arg4_type	= ARG_CONST_SIZE,
+};
+
 static DEFINE_PER_CPU(struct perf_sample_data, bpf_sd);
 
 static __always_inline u64
@@ -499,6 +538,8 @@ static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func
 		return &bpf_perf_event_output_proto;
 	case BPF_FUNC_get_stackid:
 		return &bpf_get_stackid_proto;
+	case BPF_FUNC_perf_read_counter_time:
+		return &bpf_perf_read_counter_time_proto;
 	default:
 		return tracing_func_proto(func_id);
 	}
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next 4/4] bpf: add a test case for helper bpf_perf_prog_read_time
From: Yonghong Song @ 2017-09-01 16:53 UTC (permalink / raw)
  To: peterz, rostedt, ast, daniel, netdev; +Cc: kernel-team
In-Reply-To: <20170901165357.465121-1-yhs@fb.com>

The bpf sample program trace_event is enhanced to use the new
helper to print out enabled/running time.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 samples/bpf/trace_event_kern.c            | 5 +++++
 tools/testing/selftests/bpf/bpf_helpers.h | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/samples/bpf/trace_event_kern.c b/samples/bpf/trace_event_kern.c
index 41b6115..f372660 100644
--- a/samples/bpf/trace_event_kern.c
+++ b/samples/bpf/trace_event_kern.c
@@ -37,8 +37,10 @@ struct bpf_map_def SEC("maps") stackmap = {
 SEC("perf_event")
 int bpf_prog1(struct bpf_perf_event_data *ctx)
 {
+	char time_fmt[] = "Time Enabled: %lld, Time Running: %lld";
 	char fmt[] = "CPU-%d period %lld ip %llx";
 	u32 cpu = bpf_get_smp_processor_id();
+	struct bpf_perf_time time_buf;
 	struct key_t key;
 	u64 *val, one = 1;
 
@@ -54,6 +56,9 @@ int bpf_prog1(struct bpf_perf_event_data *ctx)
 		return 0;
 	}
 
+	bpf_perf_prog_read_time(ctx, (void *)&time_buf, sizeof(struct bpf_perf_time));
+	bpf_trace_printk(time_fmt, sizeof(time_fmt), time_buf.enabled, time_buf.running);
+
 	val = bpf_map_lookup_elem(&counts, &key);
 	if (val)
 		(*val)++;
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index fe41852..ddad690 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -74,6 +74,9 @@ static int (*bpf_perf_read_counter_time)(void *map, unsigned long long flags,
 				       void *counter_time_buf,
 				       unsigned int buf_size) =
 	(void *) BPF_FUNC_perf_read_counter_time;
+static int (*bpf_perf_prog_read_time)(void *ctx, void *time_buf,
+				      unsigned int size) =
+	(void *) BPF_FUNC_perf_prog_read_time;
 
 
 /* llvm builtin functions that eBPF C program may use to
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next 3/4] bpf: add helper bpf_perf_prog_read_time
From: Yonghong Song @ 2017-09-01 16:53 UTC (permalink / raw)
  To: peterz, rostedt, ast, daniel, netdev; +Cc: kernel-team
In-Reply-To: <20170901165357.465121-1-yhs@fb.com>

This patch adds helper bpf_perf_prog_read_time for perf event based bpf
programs, to read event enabled/running time.
The enabled/running time is accumulated since the perf event open.

The typical use case for perf event based bpf program is to attach itself
to a single event. In such cases, if it is desirable to get scaling factor
between two bpf invocations, users can can save the time values in a map,
and use the value from the map and the current value to calculate
the scaling factor.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/perf_event.h |  1 +
 include/uapi/linux/bpf.h   |  8 ++++++++
 kernel/events/core.c       |  1 +
 kernel/trace/bpf_trace.c   | 24 ++++++++++++++++++++++++
 4 files changed, 34 insertions(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 7fd5e94..92955fc 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -821,6 +821,7 @@ struct perf_output_handle {
 struct bpf_perf_event_data_kern {
 	struct pt_regs *regs;
 	struct perf_sample_data *data;
+	struct perf_event *event;
 };
 
 #ifdef CONFIG_CGROUP_PERF
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9c23bef..1ae55c8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -590,6 +590,13 @@ union bpf_attr {
  *     @counter_time_buf: buf to fill
  *     @buf_size: size of the counter_time_buf
  *     Return: 0 on success or negative error code
+ *
+ * int bpf_perf_prog_read_time(ctx, time_buf, buf_size)
+ *     Read perf event enabled and running time
+ *     @ctx: pointer to ctx
+ *     @time_buf: buf to fill
+ *     @buf_size: size of the time_buf
+ *     Return : 0 on success or negative error code
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -647,6 +654,7 @@ union bpf_attr {
 	FN(sk_redirect_map),		\
 	FN(sock_map_update),		\
 	FN(perf_read_counter_time),		\
+	FN(perf_prog_read_time),		\
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ef5c7fb..1f16f1f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8019,6 +8019,7 @@ static void bpf_overflow_handler(struct perf_event *event,
 	struct bpf_perf_event_data_kern ctx = {
 		.data = data,
 		.regs = regs,
+		.event = event,
 	};
 	int ret = 0;
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b807b1a..e97620a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -608,6 +608,19 @@ BPF_CALL_3(bpf_get_stackid_tp, void *, tp_buff, struct bpf_map *, map,
 			       flags, 0, 0);
 }
 
+BPF_CALL_3(bpf_perf_prog_read_time_tp, void *, ctx, struct bpf_perf_time *,
+	time_buf, u32, size)
+{
+	struct bpf_perf_event_data_kern *kctx = (struct bpf_perf_event_data_kern *)ctx;
+	u64 now;
+
+	if (size != sizeof(struct bpf_perf_time))
+		return -EINVAL;
+
+	calc_timer_values(kctx->event, &now, &time_buf->enabled, &time_buf->running);
+	return 0;
+}
+
 static const struct bpf_func_proto bpf_get_stackid_proto_tp = {
 	.func		= bpf_get_stackid_tp,
 	.gpl_only	= true,
@@ -617,6 +630,15 @@ static const struct bpf_func_proto bpf_get_stackid_proto_tp = {
 	.arg3_type	= ARG_ANYTHING,
 };
 
+static const struct bpf_func_proto bpf_perf_prog_read_time_proto_tp = {
+         .func           = bpf_perf_prog_read_time_tp,
+         .gpl_only       = true,
+         .ret_type       = RET_INTEGER,
+         .arg1_type      = ARG_PTR_TO_CTX,
+         .arg2_type      = ARG_PTR_TO_UNINIT_MEM,
+         .arg3_type      = ARG_CONST_SIZE,
+};
+
 static const struct bpf_func_proto *tp_prog_func_proto(enum bpf_func_id func_id)
 {
 	switch (func_id) {
@@ -624,6 +646,8 @@ static const struct bpf_func_proto *tp_prog_func_proto(enum bpf_func_id func_id)
 		return &bpf_perf_event_output_proto_tp;
 	case BPF_FUNC_get_stackid:
 		return &bpf_get_stackid_proto_tp;
+	case BPF_FUNC_perf_prog_read_time:
+		return &bpf_perf_prog_read_time_proto_tp;
 	default:
 		return tracing_func_proto(func_id);
 	}
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next 2/4] bpf: add a test case for helper bpf_perf_read_counter_time
From: Yonghong Song @ 2017-09-01 16:53 UTC (permalink / raw)
  To: peterz, rostedt, ast, daniel, netdev; +Cc: kernel-team
In-Reply-To: <20170901165357.465121-1-yhs@fb.com>

The bpf sample program tracex6 is enhanced to use the new
helper to read enabled/running time.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 samples/bpf/tracex6_kern.c                | 26 ++++++++++++++++++++++++++
 samples/bpf/tracex6_user.c                | 13 ++++++++++++-
 tools/testing/selftests/bpf/bpf_helpers.h |  4 ++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/samples/bpf/tracex6_kern.c b/samples/bpf/tracex6_kern.c
index e7d1803..46acfef 100644
--- a/samples/bpf/tracex6_kern.c
+++ b/samples/bpf/tracex6_kern.c
@@ -15,6 +15,12 @@ struct bpf_map_def SEC("maps") values = {
 	.value_size = sizeof(u64),
 	.max_entries = 64,
 };
+struct bpf_map_def SEC("maps") values2 = {
+	.type = BPF_MAP_TYPE_HASH,
+	.key_size = sizeof(int),
+	.value_size = sizeof(struct bpf_perf_counter_time),
+	.max_entries = 64,
+};
 
 SEC("kprobe/htab_map_get_next_key")
 int bpf_prog1(struct pt_regs *ctx)
@@ -37,5 +43,25 @@ int bpf_prog1(struct pt_regs *ctx)
 	return 0;
 }
 
+SEC("kprobe/htab_map_lookup_elem")
+int bpf_prog2(struct pt_regs *ctx)
+{
+	u32 key = bpf_get_smp_processor_id();
+	struct bpf_perf_counter_time *val, buf;
+	int error;
+
+	error = bpf_perf_read_counter_time(&counters, key, &buf, sizeof(buf));
+	if (error)
+		return 0;
+
+	val = bpf_map_lookup_elem(&values2, &key);
+	if (val)
+		*val = buf;
+	else
+		bpf_map_update_elem(&values2, &key, &buf, BPF_NOEXIST);
+
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
 u32 _version SEC("version") = LINUX_VERSION_CODE;
diff --git a/samples/bpf/tracex6_user.c b/samples/bpf/tracex6_user.c
index a05a99a..2a0c5d8 100644
--- a/samples/bpf/tracex6_user.c
+++ b/samples/bpf/tracex6_user.c
@@ -22,6 +22,7 @@
 
 static void check_on_cpu(int cpu, struct perf_event_attr *attr)
 {
+	struct bpf_perf_counter_time value2;
 	int pmu_fd, error = 0;
 	cpu_set_t set;
 	__u64 value;
@@ -46,8 +47,18 @@ static void check_on_cpu(int cpu, struct perf_event_attr *attr)
 		fprintf(stderr, "Value missing for CPU %d\n", cpu);
 		error = 1;
 		goto on_exit;
+	} else {
+		fprintf(stderr, "CPU %d: %llu\n", cpu, value);
+	}
+	/* The above bpf_map_lookup_elem should trigger the second kprobe */
+	if (bpf_map_lookup_elem(map_fd[2], &cpu, &value2)) {
+		fprintf(stderr, "Value2 missing for CPU %d\n", cpu);
+		error = 1;
+		goto on_exit;
+	} else {
+		fprintf(stderr, "CPU %d: counter: %llu, enabled: %llu, running: %llu\n", cpu,
+			value2.counter, value2.time.enabled, value2.time.running);
 	}
-	fprintf(stderr, "CPU %d: %llu\n", cpu, value);
 
 on_exit:
 	assert(bpf_map_delete_elem(map_fd[0], &cpu) == 0 || error);
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 36fb916..fe41852 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -70,6 +70,10 @@ static int (*bpf_sk_redirect_map)(void *map, int key, int flags) =
 static int (*bpf_sock_map_update)(void *map, void *key, void *value,
 				  unsigned long long flags) =
 	(void *) BPF_FUNC_sock_map_update;
+static int (*bpf_perf_read_counter_time)(void *map, unsigned long long flags,
+				       void *counter_time_buf,
+				       unsigned int buf_size) =
+	(void *) BPF_FUNC_perf_read_counter_time;
 
 
 /* llvm builtin functions that eBPF C program may use to
-- 
2.9.5

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox