* [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
* 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
* 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] 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
* [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: 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
* 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: [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: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi
From: Willem de Bruijn @ 2017-09-01 16:15 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, Koichiro Den, virtualization,
Network Development
In-Reply-To: <5ef7fcf3-d4f0-be16-6ddb-724d954cfc68@redhat.com>
On Thu, Aug 31, 2017 at 11:25 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年08月31日 22:30, Willem de Bruijn wrote:
>>>
>>> Incomplete results at this stage, but I do see this correlation between
>>> flows. It occurs even while not running out of zerocopy descriptors,
>>> which I cannot yet explain.
>>>
>>> Running two threads in a guest, each with a udp socket, each
>>> sending up to 100 datagrams, or until EAGAIN, every msec.
>>>
>>> Sender A sends 1B datagrams.
>>> Sender B sends VHOST_GOODCOPY_LEN, which is enough
>>> to trigger zcopy_used in vhost net.
>>>
>>> A local receive process on the host receives both flows. To avoid
>>> a deep copy when looping the packet onto the receive path,
>>> changed skb_orphan_frags_rx to always return false (gross hack).
>>>
>>> The flow with the larger packets is redirected through netem on ifb0:
>>>
>>> modprobe ifb
>>> ip link set dev ifb0 up
>>> tc qdisc add dev ifb0 root netem limit $LIMIT rate 1MBit
>>>
>>> tc qdisc add dev tap0 ingress
>>> tc filter add dev tap0 parent ffff: protocol ip \
>>> u32 match ip dport 8000 0xffff \
>>> action mirred egress redirect dev ifb0
>>>
>>> For 10 second run, packet count with various ifb0 queue lengths $LIMIT:
>>>
>>> no filter
>>> rx.A: ~840,000
>>> rx.B: ~840,000
>>>
>>> limit 1
>>> rx.A: ~500,000
>>> rx.B: ~3100
>>> ifb0: 3273 sent, 371141 dropped
>>>
>>> limit 100
>>> rx.A: ~9000
>>> rx.B: ~4200
>>> ifb0: 4630 sent, 1491 dropped
>>>
>>> limit 1000
>>> rx.A: ~6800
>>> rx.B: ~4200
>>> ifb0: 4651 sent, 0 dropped
>>>
>>> Sender B is always correctly rate limited to 1 MBps or less. With a
>>> short queue, it ends up dropping a lot and sending even less.
>>>
>>> When a queue builds up for sender B, sender A throughput is strongly
>>> correlated with queue length. With queue length 1, it can send almost
>>> at unthrottled speed. But even at limit 100 its throughput is on the
>>> same order as sender B.
>>>
>>> What is surprising to me is that this happens even though the number
>>> of ubuf_info in use at limit 100 is around 100 at all times. In other
>>> words,
>>> it does not exhaust the pool.
>>>
>>> When forcing zcopy_used to be false for all packets, this effect of
>>> sender A throughput being correlated with sender B does not happen.
>>>
>>> no filter
>>> rx.A: ~850,000
>>> rx.B: ~850,000
>>>
>>> limit 100
>>> rx.A: ~850,000
>>> rx.B: ~4200
>>> ifb0: 4518 sent, 876182 dropped
>>>
>>> Also relevant is that with zerocopy, the sender processes back off
>>> and report the same count as the receiver. Without zerocopy,
>>> both senders send at full speed, even if only 4200 packets from flow
>>> B arrive at the receiver.
>>>
>>> This is with the default virtio_net driver, so without napi-tx.
>>>
>>> It appears that the zerocopy notifications are pausing the guest.
>>> Will look at that now.
>>
>> It was indeed as simple as that. With 256 descriptors, queuing even
>> a hundred or so packets causes the guest to stall the device as soon
>> as the qdisc is installed.
>>
>> Adding this check
>>
>> + in_use = nvq->upend_idx - nvq->done_idx;
>> + if (nvq->upend_idx < nvq->done_idx)
>> + in_use += UIO_MAXIOV;
>> +
>> + if (in_use > (vq->num >> 2))
>> + zcopy_used = false;
>>
>> Has the desired behavior of reverting zerocopy requests to copying.
>>
>> Without this change, the result is, as previously reported, throughput
>> dropping to hundreds of packets per second on both flows.
>>
>> With the change, pps as observed for a few seconds at handle_tx is
>>
>> zerocopy=165 copy=168435
>> zerocopy=0 copy=168500
>> zerocopy=65 copy=168535
>>
>> Both flows continue to send at more or less normal rate, with only
>> sender B observing massive drops at the netem.
>>
>> With the queue removed the rate reverts to
>>
>> zerocopy=58878 copy=110239
>> zerocopy=58833 copy=110207
>>
>> 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?
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: [PATCH net-next 1/2] flow_dissector: Cleanup control flow
From: Tom Herbert @ 2017-09-01 16:12 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: David S . Miller, Linux Kernel Network Developers, alex.popov
In-Reply-To: <87k21izjdc.fsf@stressinduktion.org>
On Fri, Sep 1, 2017 at 5:35 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Tom Herbert <tom@quantonium.net> writes:
>
>> __skb_flow_dissect is riddled with gotos that make discerning the flow,
>> debugging, and extending the capability difficult. This patch
>> reorganizes things so that we only perform goto's after the two main
>> switch statements (no gotos within the cases now). It also eliminates
>> several goto labels so that there are only two labels that can be target
>> for goto.
>
> The problem with the
>
> fdret = ... ;
> break;
>
> is that we now have to count curly braces to look what break does
> actually break and where fdret is being processed. With the number of
> context lines you send for the patch this is hard to review.
>
> I tend to like the gotos a bit more for now.
This is a step towards a more modular design for flow dissector. The
goto's force a monolithic design and make it hard to implement new
functionality like trying to enforce limits on encapsulation which
requires a single point for logic. The ip, ipv6, and mpls labels were
really unnecessary to begin with, the proto again logic works fine for
those. This is also a segway to breaking up the very large
__skb_flow_dissect function into more manageable components (IP
protocol dissection should be its own function for instance). Follow
on patches will allow protocol specific implementations of flow
dissection located with the rest of the protocol implementation, so
hopefully we can end the practice of adding support for every
networking protocol in one single core function (analogous to how we
parse protocols in GRO).
Tom
^ permalink raw reply
* Re: [PATCH net-next v2] doc: document MSG_ZEROCOPY
From: Alexei Starovoitov @ 2017-09-01 16:12 UTC (permalink / raw)
To: Willem de Bruijn, netdev; +Cc: davem, brouer, Willem de Bruijn
In-Reply-To: <20170901160141.54616-1-willemdebruijn.kernel@gmail.com>
On 9/1/17 9:01 AM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Documentation for this feature was missing from the patchset.
> Copied a lot from the netdev 2.1 paper, addressing some small
> interface changes since then.
>
> Changes
> v1 -> v2
> - change email discussion URL format
> - clarify that u32 counter is per-syscall, unsigned and
> wraps after UINT_MAX calls
> - describe errno on send failure specific to MSG_ZEROCOPY
> - a few very minor rewordings
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* [PATCH net-next v2] doc: document MSG_ZEROCOPY
From: Willem de Bruijn @ 2017-09-01 16:01 UTC (permalink / raw)
To: netdev; +Cc: davem, ast, brouer, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Documentation for this feature was missing from the patchset.
Copied a lot from the netdev 2.1 paper, addressing some small
interface changes since then.
Changes
v1 -> v2
- change email discussion URL format
- clarify that u32 counter is per-syscall, unsigned and
wraps after UINT_MAX calls
- describe errno on send failure specific to MSG_ZEROCOPY
- a few very minor rewordings
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
Documentation/networking/msg_zerocopy.rst | 258 ++++++++++++++++++++++++++++++
1 file changed, 258 insertions(+)
create mode 100644 Documentation/networking/msg_zerocopy.rst
diff --git a/Documentation/networking/msg_zerocopy.rst b/Documentation/networking/msg_zerocopy.rst
new file mode 100644
index 000000000000..1d18e79fba86
--- /dev/null
+++ b/Documentation/networking/msg_zerocopy.rst
@@ -0,0 +1,258 @@
+
+============
+MSG_ZEROCOPY
+============
+
+Intro
+=====
+
+The MSG_ZEROCOPY flag enables copy avoidance for socket send calls.
+The feature is currently implemented for TCP sockets.
+
+
+Opportunity and Caveats
+-----------------------
+
+Copying large buffers between user process and kernel can be
+expensive. Linux supports various interfaces that eschew copying,
+such as sendpage and splice. The MSG_ZEROCOPY flag extends the
+underlying copy avoidance mechanism to common socket send calls.
+
+Copy avoidance is not a free lunch. As implemented, with page pinning,
+it replaces per byte copy cost with page accounting and completion
+notification overhead. As a result, MSG_ZEROCOPY is generally only
+effective at writes over around 10 KB.
+
+Page pinning also changes system call semantics. It temporarily shares
+the buffer between process and network stack. Unlike with copying, the
+process cannot immediately overwrite the buffer after system call
+return without possibly modifying the data in flight. Kernel integrity
+is not affected, but a buggy program can possibly corrupt its own data
+stream.
+
+The kernel returns a notification when it is safe to modify data.
+Converting an existing application to MSG_ZEROCOPY is not always as
+trivial as just passing the flag, then.
+
+
+More Info
+---------
+
+Much of this document was derived from a longer paper presented at
+netdev 2.1. For more in-depth information see that paper and talk,
+the excellent reporting over at LWN.net or read the original code.
+
+ paper, slides, video
+ https://netdevconf.org/2.1/session.html?debruijn
+
+ LWN article
+ https://lwn.net/Articles/726917/
+
+ patchset
+ [PATCH net-next v4 0/9] socket sendmsg MSG_ZEROCOPY
+ http://lkml.kernel.org/r/20170803202945.70750-1-willemdebruijn.kernel@gmail.com
+
+
+Interface
+=========
+
+Passing the MSG_ZEROCOPY flag is the most obvious step to enable copy
+avoidance, but not the only one.
+
+Socket Setup
+------------
+
+The kernel is permissive when applications pass undefined flags to the
+send system call. By default it simply ignores these. To avoid enabling
+copy avoidance mode for legacy processes that accidentally already pass
+this flag, a process must first signal intent by setting a socket option:
+
+::
+
+ if (setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &one, sizeof(one)))
+ error(1, errno, "setsockopt zerocopy");
+
+
+Transmission
+------------
+
+The change to send (or sendto, sendmsg, sendmmsg) itself is trivial.
+Pass the new flag.
+
+::
+
+ ret = send(fd, buf, sizeof(buf), MSG_ZEROCOPY);
+
+A zerocopy failure will return -1 with errno ENOBUFS. This happens if
+the socket option was not set, the socket exceeds its optmem limit or
+the user exceeds its ulimit on locked pages.
+
+
+Mixing copy avoidance and copying
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Many workloads have a mixture of large and small buffers. Because copy
+avoidance is more expensive than copying for small packets, the
+feature is implemented as a flag. It is safe to mix calls with the flag
+with those without.
+
+
+Notifications
+-------------
+
+The kernel has to notify the process when it is safe to reuse a
+previously passed buffer. It queues completion notifications on the
+socket error queue, akin to the transmit timestamping interface.
+
+The notification itself is a simple scalar value. Each socket
+maintains an internal unsigned 32-bit counter. Each send call with
+MSG_ZEROCOPY that successfully sends data increments the counter. The
+counter is not incremented on failure or if called with length zero.
+The counter counts system call invocations, not bytes. It wraps after
+UINT_MAX calls.
+
+
+Notification Reception
+~~~~~~~~~~~~~~~~~~~~~~
+
+The below snippet demonstrates the API. In the simplest case, each
+send syscall is followed by a poll and recvmsg on the error queue.
+
+Reading from the error queue is always a non-blocking operation. The
+poll call is there to block until an error is outstanding. It will set
+POLLERR in its output flags. That flag does not have to be set in the
+events field. Errors are signaled unconditionally.
+
+::
+
+ pfd.fd = fd;
+ pfd.events = 0;
+ if (poll(&pfd, 1, -1) != 1 || pfd.revents & POLLERR == 0)
+ error(1, errno, "poll");
+
+ ret = recvmsg(fd, &msg, MSG_ERRQUEUE);
+ if (ret == -1)
+ error(1, errno, "recvmsg");
+
+ read_notification(msg);
+
+The example is for demonstration purpose only. In practice, it is more
+efficient to not wait for notifications, but read without blocking
+every couple of send calls.
+
+Notifications can be processed out of order with other operations on
+the socket. A socket that has an error queued would normally block
+other operations until the error is read. Zerocopy notifications have
+a zero error code, however, to not block send and recv calls.
+
+
+Notification Batching
+~~~~~~~~~~~~~~~~~~~~~
+
+Multiple outstanding packets can be read at once using the recvmmsg
+call. This is often not needed. In each message the kernel returns not
+a single value, but a range. It coalesces consecutive notifications
+while one is outstanding for reception on the error queue.
+
+When a new notification is about to be queued, it checks whether the
+new value extends the range of the notification at the tail of the
+queue. If so, it drops the new notification packet and instead increases
+the range upper value of the outstanding notification.
+
+For protocols that acknowledge data in-order, like TCP, each
+notification can be squashed into the previous one, so that no more
+than one notification is outstanding at any one point.
+
+Ordered delivery is the common case, but not guaranteed. Notifications
+may arrive out of order on retransmission and socket teardown.
+
+
+Notification Parsing
+~~~~~~~~~~~~~~~~~~~~
+
+The below snippet demonstrates how to parse the control message: the
+read_notification() call in the previous snippet. A notification
+is encoded in the standard error format, sock_extended_err.
+
+The level and type fields in the control data are protocol family
+specific, IP_RECVERR or IPV6_RECVERR.
+
+Error origin is the new type SO_EE_ORIGIN_ZEROCOPY. ee_errno is zero,
+as explained before, to avoid blocking read and write system calls on
+the socket.
+
+The 32-bit notification range is encoded as [ee_info, ee_data]. This
+range is inclusive. Other fields in the struct must be treated as
+undefined, bar for ee_code, as discussed below.
+
+::
+
+ struct sock_extended_err *serr;
+ struct cmsghdr *cm;
+
+ cm = CMSG_FIRSTHDR(msg);
+ if (cm->cmsg_level != SOL_IP &&
+ cm->cmsg_type != IP_RECVERR)
+ error(1, 0, "cmsg");
+
+ serr = (void *) CMSG_DATA(cm);
+ if (serr->ee_errno != 0 ||
+ serr->ee_origin != SO_EE_ORIGIN_ZEROCOPY)
+ error(1, 0, "serr");
+
+ printf("completed: %u..%u\n", serr->ee_info, serr->ee_data);
+
+
+Deferred copies
+~~~~~~~~~~~~~~~
+
+Passing flag MSG_ZEROCOPY is a hint to the kernel to apply copy
+avoidance, and a contract that the kernel will queue a completion
+notification. It is not a guarantee that the copy is elided.
+
+Copy avoidance is not always feasible. Devices that do not support
+scatter-gather I/O cannot send packets made up of kernel generated
+protocol headers plus zerocopy user data. A packet may need to be
+converted to a private copy of data deep in the stack, say to compute
+a checksum.
+
+In all these cases, the kernel returns a completion notification when
+it releases its hold on the shared pages. That notification may arrive
+before the (copied) data is fully transmitted. A zerocopy completion
+notification is not a transmit completion notification, therefore.
+
+Deferred copies can be more expensive than a copy immediately in the
+system call, if the data is no longer warm in the cache. The process
+also incurs notification processing cost for no benefit. For this
+reason, the kernel signals if data was completed with a copy, by
+setting flag SO_EE_CODE_ZEROCOPY_COPIED in field ee_code on return.
+A process may use this signal to stop passing flag MSG_ZEROCOPY on
+subsequent requests on the same socket.
+
+
+Implementation
+==============
+
+Loopback
+--------
+
+Data sent to local sockets can be queued indefinitely if the receive
+process does not read its socket. Unbound notification latency is not
+acceptable. For this reason all packets generated with MSG_ZEROCOPY
+that are looped to a local socket will incur a deferred copy. This
+includes looping onto packet sockets (e.g., tcpdump) and tun devices.
+
+
+Testing
+=======
+
+More realistic example code can be found in the kernel source under
+tools/testing/selftests/net/msg_zerocopy.c.
+
+Be cognizant of the loopback constraint. The test can be run between
+a pair of hosts. But if run between a local pair of processes, for
+instance when run with msg_zerocopy.sh between a veth pair across
+namespaces, the test will not show any improvement. For testing, the
+loopback restriction can be temporarily relaxed by making
+skb_orphan_frags_rx identical to skb_orphan_frags.
+
--
2.14.1.581.gf28d330327-goog
^ permalink raw reply related
* [PATCH net 2/2] l2tp: pass tunnel pointer to ->session_create()
From: Guillaume Nault @ 2017-09-01 15:58 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
In-Reply-To: <cover.1504277892.git.g.nault@alphalink.fr>
Using l2tp_tunnel_find() in pppol2tp_session_create() and
l2tp_eth_create() is racy, because no reference is held on the
returned session. These functions are only used to implement the
->session_create callback which is run by l2tp_nl_cmd_session_create().
Therefore searching for the parent tunnel isn't necessary because
l2tp_nl_cmd_session_create() already has a pointer to it and holds a
reference.
This patch modifies ->session_create()'s prototype to directly pass the
the parent tunnel as parameter, thus avoiding searching for it in
pppol2tp_session_create() and l2tp_eth_create().
Since we have to touch the ->session_create() call in
l2tp_nl_cmd_session_create(), let's also remove the useless conditional:
we know that ->session_create isn't NULL at this point because it's
already been checked earlier in this same function.
Finally, one might be tempted to think that the removed
l2tp_tunnel_find() calls were harmless because they would return the
same tunnel as the one held by l2tp_nl_cmd_session_create() anyway.
But that tunnel might be removed and a new one created with same tunnel
Id before the l2tp_tunnel_find() call. In this case l2tp_tunnel_find()
would return the new tunnel which wouldn't be protected by the
reference held by l2tp_nl_cmd_session_create().
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
net/l2tp/l2tp_core.h | 4 +++-
net/l2tp/l2tp_eth.c | 11 +++--------
net/l2tp/l2tp_netlink.c | 8 ++++----
net/l2tp/l2tp_ppp.c | 19 +++++++------------
4 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 4593d48df953..a305e0c5925a 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -201,7 +201,9 @@ struct l2tp_tunnel {
};
struct l2tp_nl_cmd_ops {
- int (*session_create)(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg);
+ int (*session_create)(struct net *net, struct l2tp_tunnel *tunnel,
+ u32 session_id, u32 peer_session_id,
+ struct l2tp_session_cfg *cfg);
int (*session_delete)(struct l2tp_session *session);
};
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 4de2ec94b08c..87da9ef61860 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -262,24 +262,19 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
dev->needed_headroom += session->hdr_len;
}
-static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
+static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
+ u32 session_id, u32 peer_session_id,
+ struct l2tp_session_cfg *cfg)
{
unsigned char name_assign_type;
struct net_device *dev;
char name[IFNAMSIZ];
- struct l2tp_tunnel *tunnel;
struct l2tp_session *session;
struct l2tp_eth *priv;
struct l2tp_eth_sess *spriv;
int rc;
struct l2tp_eth_net *pn;
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (!tunnel) {
- rc = -ENODEV;
- goto out;
- }
-
if (cfg->ifname) {
strlcpy(name, cfg->ifname, IFNAMSIZ);
name_assign_type = NET_NAME_USER;
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 57427d430f10..7135f4645d3a 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -643,10 +643,10 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
break;
}
- ret = -EPROTONOSUPPORT;
- if (l2tp_nl_cmd_ops[cfg.pw_type]->session_create)
- ret = (*l2tp_nl_cmd_ops[cfg.pw_type]->session_create)(net, tunnel_id,
- session_id, peer_session_id, &cfg);
+ ret = l2tp_nl_cmd_ops[cfg.pw_type]->session_create(net, tunnel,
+ session_id,
+ peer_session_id,
+ &cfg);
if (ret >= 0) {
session = l2tp_session_get(net, tunnel, session_id, false);
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index f0edb7209079..50e3ee9a9d61 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -788,25 +788,20 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
#ifdef CONFIG_L2TP_V3
-/* Called when creating sessions via the netlink interface.
- */
-static int pppol2tp_session_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
+/* Called when creating sessions via the netlink interface. */
+static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
+ u32 session_id, u32 peer_session_id,
+ struct l2tp_session_cfg *cfg)
{
int error;
- struct l2tp_tunnel *tunnel;
struct l2tp_session *session;
struct pppol2tp_session *ps;
- tunnel = l2tp_tunnel_find(net, tunnel_id);
-
- /* Error if we can't find the tunnel */
- error = -ENOENT;
- if (tunnel == NULL)
- goto out;
-
/* Error if tunnel socket is not prepped */
- if (tunnel->sock == NULL)
+ if (!tunnel->sock) {
+ error = -ENOENT;
goto out;
+ }
/* Default MTU values. */
if (cfg->mtu == 0)
--
2.14.1
^ permalink raw reply related
* [PATCH net 0/2] l2tp: session creation fixes
From: Guillaume Nault @ 2017-09-01 15:58 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
The session creation process has a few issues wrt. concurrent tunnel
deletion.
Patch #1 avoids creating sessions in tunnels that are getting removed.
This prevents races where sessions could try to take tunnel resources
that were already released.
Patch #2 removes some racy l2tp_tunnel_find() calls in session creation
callbacks. Together with path #1 it ensures that sessions can only
access tunnel resources that are guaranteed to remain valid during the
session creation process.
There are other problems with how sessions are created: pseudo-wire
specific data are set after the session is added to the tunnel. So
the session can be used, or deleted, before it has been completely
initialised. Separating session allocation from session registration
would be necessary, but we'd still have circular dependencies
preventing race-free registration. I'll consider this issue in future
series.
Guillaume Nault (2):
l2tp: prevent creation of sessions on terminated tunnels
l2tp: pass tunnel pointer to ->session_create()
net/l2tp/l2tp_core.c | 41 ++++++++++++++++++++++++++++-------------
net/l2tp/l2tp_core.h | 8 +++++++-
net/l2tp/l2tp_eth.c | 11 +++--------
net/l2tp/l2tp_netlink.c | 8 ++++----
net/l2tp/l2tp_ppp.c | 19 +++++++------------
5 files changed, 49 insertions(+), 38 deletions(-)
--
2.14.1
^ permalink raw reply
* [PATCH net 1/2] l2tp: prevent creation of sessions on terminated tunnels
From: Guillaume Nault @ 2017-09-01 15:58 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
In-Reply-To: <cover.1504277892.git.g.nault@alphalink.fr>
l2tp_tunnel_destruct() sets tunnel->sock to NULL, then removes the
tunnel from the pernet list and finally closes all its sessions.
Therefore, it's possible to add a session to a tunnel that is still
reachable, but for which tunnel->sock has already been reset. This can
make l2tp_session_create() dereference a NULL pointer when calling
sock_hold(tunnel->sock).
This patch adds the .acpt_newsess field to struct l2tp_tunnel, which is
used by l2tp_tunnel_closeall() to prevent addition of new sessions to
tunnels. Resetting tunnel->sock is done after l2tp_tunnel_closeall()
returned, so that l2tp_session_add_to_tunnel() can safely take a
reference on it when .acpt_newsess is true.
The .acpt_newsess field is modified in l2tp_tunnel_closeall(), rather
than in l2tp_tunnel_destruct(), so that it benefits all tunnel removal
mechanisms. E.g. on UDP tunnels, a session could be added to a tunnel
after l2tp_udp_encap_destroy() proceeded. This would prevent the tunnel
from being removed because of the references held by this new session
on the tunnel and its socket. Even though the session could be removed
manually later on, this defeats the purpose of
commit 9980d001cec8 ("l2tp: add udp encap socket destroy handler").
Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
net/l2tp/l2tp_core.c | 41 ++++++++++++++++++++++++++++-------------
net/l2tp/l2tp_core.h | 4 ++++
2 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 90165a6874bc..ee485df73ccd 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -329,13 +329,21 @@ static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
struct hlist_head *g_head;
struct hlist_head *head;
struct l2tp_net *pn;
+ int err;
head = l2tp_session_id_hash(tunnel, session->session_id);
write_lock_bh(&tunnel->hlist_lock);
+ if (!tunnel->acpt_newsess) {
+ err = -ENODEV;
+ goto err_tlock;
+ }
+
hlist_for_each_entry(session_walk, head, hlist)
- if (session_walk->session_id == session->session_id)
- goto exist;
+ if (session_walk->session_id == session->session_id) {
+ err = -EEXIST;
+ goto err_tlock;
+ }
if (tunnel->version == L2TP_HDR_VER_3) {
pn = l2tp_pernet(tunnel->l2tp_net);
@@ -343,12 +351,21 @@ static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
session->session_id);
spin_lock_bh(&pn->l2tp_session_hlist_lock);
+
hlist_for_each_entry(session_walk, g_head, global_hlist)
- if (session_walk->session_id == session->session_id)
- goto exist_glob;
+ if (session_walk->session_id == session->session_id) {
+ err = -EEXIST;
+ goto err_tlock_pnlock;
+ }
+ l2tp_tunnel_inc_refcount(tunnel);
+ sock_hold(tunnel->sock);
hlist_add_head_rcu(&session->global_hlist, g_head);
+
spin_unlock_bh(&pn->l2tp_session_hlist_lock);
+ } else {
+ l2tp_tunnel_inc_refcount(tunnel);
+ sock_hold(tunnel->sock);
}
hlist_add_head(&session->hlist, head);
@@ -356,12 +373,12 @@ static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
return 0;
-exist_glob:
+err_tlock_pnlock:
spin_unlock_bh(&pn->l2tp_session_hlist_lock);
-exist:
+err_tlock:
write_unlock_bh(&tunnel->hlist_lock);
- return -EEXIST;
+ return err;
}
/* Lookup a tunnel by id
@@ -1251,7 +1268,6 @@ static void l2tp_tunnel_destruct(struct sock *sk)
/* Remove hooks into tunnel socket */
sk->sk_destruct = tunnel->old_sk_destruct;
sk->sk_user_data = NULL;
- tunnel->sock = NULL;
/* Remove the tunnel struct from the tunnel list */
pn = l2tp_pernet(tunnel->l2tp_net);
@@ -1261,6 +1277,8 @@ static void l2tp_tunnel_destruct(struct sock *sk)
atomic_dec(&l2tp_tunnel_count);
l2tp_tunnel_closeall(tunnel);
+
+ tunnel->sock = NULL;
l2tp_tunnel_dec_refcount(tunnel);
/* Call the original destructor */
@@ -1285,6 +1303,7 @@ void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
tunnel->name);
write_lock_bh(&tunnel->hlist_lock);
+ tunnel->acpt_newsess = false;
for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
again:
hlist_for_each_safe(walk, tmp, &tunnel->session_hlist[hash]) {
@@ -1581,6 +1600,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
tunnel->magic = L2TP_TUNNEL_MAGIC;
sprintf(&tunnel->name[0], "tunl %u", tunnel_id);
rwlock_init(&tunnel->hlist_lock);
+ tunnel->acpt_newsess = true;
/* The net we belong to */
tunnel->l2tp_net = net;
@@ -1829,11 +1849,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
return ERR_PTR(err);
}
- l2tp_tunnel_inc_refcount(tunnel);
-
- /* Ensure tunnel socket isn't deleted */
- sock_hold(tunnel->sock);
-
/* Ignore management session in session count value */
if (session->session_id != 0)
atomic_inc(&l2tp_session_count);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 9101297f27ad..4593d48df953 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -162,6 +162,10 @@ struct l2tp_tunnel {
int magic; /* Should be L2TP_TUNNEL_MAGIC */
struct rcu_head rcu;
rwlock_t hlist_lock; /* protect session_hlist */
+ bool acpt_newsess; /* Indicates whether this
+ * tunnel accepts new sessions.
+ * Protected by hlist_lock.
+ */
struct hlist_head session_hlist[L2TP_HASH_SIZE];
/* hashed list of sessions,
* hashed by id */
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net-next] bpf: Collapse offset checks in sock_filter_is_valid_access
From: Daniel Borkmann @ 2017-09-01 15:53 UTC (permalink / raw)
To: David Ahern, netdev
In-Reply-To: <1504279087-11379-1-git-send-email-dsahern@gmail.com>
On 09/01/2017 05:18 PM, 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>
Thanks for following up,
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: [PATCH net] vhost_net: correctly check tx avail during rx busy polling
From: Michael S. Tsirkin @ 2017-09-01 15:51 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1504256570-3488-1-git-send-email-jasowang@redhat.com>
On Fri, Sep 01, 2017 at 05:02:50PM +0800, Jason Wang wrote:
> We check tx avail through vhost_enable_notify() in the past which is
> wrong since it only checks whether or not guest has filled more
> available buffer since last avail idx synchronization which was just
> done by vhost_vq_avail_empty() before. What we really want is checking
> pending buffers in the avail ring.
These are rx buffers, right? I'm not even sure why do we need to poll
for them. Running out of rx buffers is a slow path.
> Fix this by calling
> vhost_vq_avail_empty() instead.
>
> This issue could be noticed by doing netperf TCP_RR benchmark as
> client from guest (but not host). With this fix, TCP_RR from guest to
> localhost restores from 1375.91 trans per sec to 55235.28 trans per
> sec on my laptop (Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz).
>
> Fixes: 030881372460 ("vhost_net: basic polling support")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> - The patch is needed for -stable
> ---
> drivers/vhost/net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 06d0448..1b68253 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -634,7 +634,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
In fact why does it poll the ring at all? I thought this function's
job is to poll the socket, isn't it?
>
> preempt_enable();
>
> - if (vhost_enable_notify(&net->dev, vq))
> + if (!vhost_vq_avail_empty(&net->dev, vq))
> vhost_poll_queue(&vq->poll);
> mutex_unlock(&vq->mutex);
Adding more contex:
mutex_lock(&vq->mutex);
vhost_disable_notify(&net->dev, vq);
preempt_disable();
endtime = busy_clock() + vq->busyloop_timeout;
while (vhost_can_busy_poll(&net->dev, endtime) &&
!sk_has_rx_data(sk) &&
vhost_vq_avail_empty(&net->dev, vq))
cpu_relax();
preempt_enable();
if (vhost_enable_notify(&net->dev, vq))
vhost_poll_queue(&vq->poll);
mutex_unlock(&vq->mutex);
len = peek_head_len(rvq, sk);
If you drop this we'll exit the function with notifications
disabled. Seems wrong to me.
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next] doc: document MSG_ZEROCOPY
From: Willem de Bruijn @ 2017-09-01 15:47 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Network Development, David Miller, Willem de Bruijn
In-Reply-To: <20170901031009.2p4vv6fncma2y2l7@ast-mbp>
On Thu, Aug 31, 2017 at 11:10 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Aug 31, 2017 at 11:04:41PM -0400, Willem de Bruijn wrote:
>> On Thu, Aug 31, 2017 at 10:10 PM, Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>> > On Thu, Aug 31, 2017 at 05:00:13PM -0400, Willem de Bruijn wrote:
>> >> From: Willem de Bruijn <willemb@google.com>
>> >>
>> >> Documentation for this feature was missing from the patchset.
>> >> Copied a lot from the netdev 2.1 paper, addressing some small
>> >> interface changes since then.
>> >>
>> >> Signed-off-by: Willem de Bruijn <willemb@google.com>
>> > ...
>> >> +Notification Batching
>> >> +~~~~~~~~~~~~~~~~~~~~~
>> >> +
>> >> +Multiple outstanding packets can be read at once using the recvmmsg
>> >> +call. This is often not needed. In each message the kernel returns not
>> >> +a single value, but a range. It coalesces consecutive notifications
>> >> +while one is outstanding for reception on the error queue.
>> >> +
>> >> +When a new notification is about to be queued, it checks whether the
>> >> +new value extends the range of the notification at the tail of the
>> >> +queue. If so, it drops the new notification packet and instead increases
>> >> +the range upper value of the outstanding notification.
>> >
>> > Would it make sense to mention that max notification range is 32-bit?
>> > So each 4Gbyte of xmit bytes there will be a notification.
>> > In modern 40Gbps NICs it's not a lot. Means that there will be
>> > at least one notification every second.
>> > Or I misread the code?
>>
>> You're right. The doc does mention that the counter and range
>> are 32-bit. I can state more explicitly that that bounds the working
>> set size to 4GB. Do you expect this to be problematic? Processing
>> a single notification per 4GB of data should not be a significant
>> cost in itself.
Actually, the counter is not a byte counter. It is incremented on each
system call that sends data with MSG_ZEROCOPY. So the 4GB limit
would only hold if a caller sends single byte requests at a time.
I will make this more clear in v2.
>
> I think 4GB is fine. Just there was an idea that in cases when
> notification of transmission can be known by other means the user space
> could have skipped reading errqeuee completely, but looks like it
> still needs to poll. That's fine.
>> > Thanks for the doc!
>>
>> Thanks for reviewing :)
>>
>> >
>> > Acked-by: Alexei Starovoitov <ast@kernel.org>
>> >
^ permalink raw reply
* Re: [PATCH net-next v5 2/2] tcp_diag: report TCP MD5 signing keys and addresses
From: Sabrina Dubroca @ 2017-09-01 15:47 UTC (permalink / raw)
To: Ivan Delalande; +Cc: David Miller, Eric Dumazet, netdev
In-Reply-To: <20170901002118.sprwhuybmk4spy34@ycc.fr>
2017-09-01, 02:21:18 +0200, Ivan Delalande wrote:
> On Fri, Sep 01, 2017 at 01:26:33AM +0200, Sabrina Dubroca wrote:
> > 2017-08-31, 09:59:39 -0700, Ivan Delalande wrote:
> > > diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
> > > index a748c74aa8b7..abbf0edcf6c2 100644
> > > --- a/net/ipv4/tcp_diag.c
> > > +++ b/net/ipv4/tcp_diag.c
> > [...]
> > > +static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
> > > + struct sk_buff *skb)
> > > +{
> > > +#ifdef CONFIG_TCP_MD5SIG
> > > + if (net_admin) {
> >
> > In tcp_diag_get_aux_size() you put a check for sk_fullsock. I don't
> > see anything preventing you from reaching this with a !fullsock?
>
> Currently handler->idiag_get_aux is only called from inet_sk_diag_fill
> which has a `BUG_ON(!sk_fullsock(sk));`, but I could add another
Oh, right, that's the part I was missing. Thanks.
> explicit check in that function if you think it's more consistent.
>
> Actually, I wasn't sure when adding this idiag_get_aux in v2 if it
> should be called from inet_twsk_diag_fill, inet_req_diag_fill and
> inet_csk_diag_fill, or just the last one. I chose that simpler approach
> for now to avoid duplicating these state checks in the idiag_get_aux
> defined by protocols and because we didn't need for INET_DIAG_MD5SIG,
> but it shouldn't be too hard to change. Do you think this could be
> useful for other protocols or attributes?
No opinion. It can be added later if necessary.
--
Sabrina
^ permalink raw reply
* Re: virtio_net: ethtool supported link modes
From: Michael S. Tsirkin @ 2017-09-01 15:43 UTC (permalink / raw)
To: Radu Rendec; +Cc: virtualization, netdev, linux-kernel, Jason Wang, virtio-dev
In-Reply-To: <1504199044.22080.11.camel@arista.com>
On Thu, Aug 31, 2017 at 06:04:04PM +0100, Radu Rendec wrote:
> Hello,
>
> 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?
>
> Thank you,
> Radu Rendec
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
If you think we need to add more hacks like this, a stronger
motivation than "to see consistency" would be needed.
--
MST
^ permalink raw reply
* Re: [PATCH net-next 2/2] flow_dissector: Add limits for encapsulation and EH
From: Tom Herbert @ 2017-09-01 15:38 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: David S . Miller, Linux Kernel Network Developers, alex.popov
In-Reply-To: <87fuc6zgra.fsf@stressinduktion.org>
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.
>> 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.
>> +
>> +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.
>
>> +
>> /**
>> * __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.
Tom
^ permalink raw reply
* (unknown),
From: stef.ryckmans @ 2017-09-01 15:30 UTC (permalink / raw)
To: netdev
[-- Attachment #1: 8247893720.doc --]
[-- Type: application/msword, Size: 40147 bytes --]
^ permalink raw reply
* ptp device strangeness
From: Tim Sander @ 2017-09-01 15:28 UTC (permalink / raw)
To: Richard Cochran, linux-kernel; +Cc: netdev
Hi
I am currently using ptp on a Altera/Intel SOC with a dp8640 PHY.
PTP functionality seems to be right. But i am doing timestamping
with gpio0 and sometimes i loose the sync of the stamping and
the events. So i would like to read out all messages. Reading O_NONBLOCK
does not work so i tried polling from usermode with the below code:
np = poll(&ev, 1, 0);
ev.fd=ptpDev;
ev.events = POLLIN;
if (np>0) {
if (ev.revents>0) {
std::cout<<"discarded ptp event"<<std::endl;
read(ptpDev, &event, sizeof(event));
}
But as confirmed in the debugger np=1 and read blocks forever.
I don't think that this is correct behavior?
For pinning down this misbehavior I would like to know it this is a local
problem of my hardware or if this is a general problem with the ptp chardev
interface?
I am currently on 4.11.12. As this is the latest preempt rt release.
Best regards
Tim
^ permalink raw reply
* Re: [net-next PATCH] ixgbe: add counter for times rx pages gets allocated, not recycled
From: Alexander Duyck @ 2017-09-01 15:24 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: Netdev, Jeff Kirsher
In-Reply-To: <150426329512.26774.12697329010993174947.stgit@firesoul>
On Fri, Sep 1, 2017 at 3:54 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> The ixgbe driver have page recycle scheme based around the RX-ring
> queue, where a RX page is shared between two packets. Based on the
> refcnt, the driver can determine if the RX-page is currently only used
> by a single packet, if so it can then directly refill/recycle the
> RX-slot by with the opposite "side" of the page.
>
> While this is a clever trick, it is hard to determine when this
> recycling is successful and when it fails. Adding a counter, which is
> available via ethtool --statistics as 'alloc_rx_page'. Which counts
> the number of times the recycle fails and the real page allocator is
> invoked. When interpreting the stats, do remember that every alloc
> will serve two packets.
>
> The counter is collected per rx_ring, but is summed and ethtool
> exported as 'alloc_rx_page'. It would be relevant to know what
> rx_ring that cannot keep up, but that can be exported later if
> someone experience a need for this.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 ++
> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 1 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++++
> 3 files changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index dd5578756ae0..008d0085e01f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -275,6 +275,7 @@ struct ixgbe_rx_queue_stats {
> u64 rsc_count;
> u64 rsc_flush;
> u64 non_eop_descs;
> + u64 alloc_rx_page;
> u64 alloc_rx_page_failed;
> u64 alloc_rx_buff_failed;
> u64 csum_err;
> @@ -655,6 +656,7 @@ struct ixgbe_adapter {
> u64 rsc_total_count;
> u64 rsc_total_flush;
> u64 non_eop_descs;
> + u32 alloc_rx_page;
> u32 alloc_rx_page_failed;
> u32 alloc_rx_buff_failed;
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 72c565712a5f..d96d9d6c3492 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -104,6 +104,7 @@ static const struct ixgbe_stats ixgbe_gstrings_stats[] = {
> {"tx_flow_control_xoff", IXGBE_STAT(stats.lxofftxc)},
> {"rx_flow_control_xoff", IXGBE_STAT(stats.lxoffrxc)},
> {"rx_csum_offload_errors", IXGBE_STAT(hw_csum_rx_error)},
> + {"alloc_rx_page", IXGBE_STAT(alloc_rx_page)},
> {"alloc_rx_page_failed", IXGBE_STAT(alloc_rx_page_failed)},
> {"alloc_rx_buff_failed", IXGBE_STAT(alloc_rx_buff_failed)},
> {"rx_no_dma_resources", IXGBE_STAT(hw_rx_no_dma_resources)},
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index d962368d08d0..7d2e4b08cdf4 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -1598,6 +1598,7 @@ static bool ixgbe_alloc_mapped_page(struct ixgbe_ring *rx_ring,
> rx_ring->rx_stats.alloc_rx_page_failed++;
> return false;
> }
> + rx_ring->rx_stats.alloc_rx_page++;
So this line should be moved down past the DMA page mapping as it can
fail on some architectures. My personal preference would be to have
this placed in the lines just after the "bi" members being updated,
and before the return.
> /* map page for use */
> dma = dma_map_page_attrs(rx_ring->dev, page, 0,
> @@ -6771,6 +6772,7 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
> u32 i, missed_rx = 0, mpc, bprc, lxon, lxoff, xon_off_tot;
> u64 non_eop_descs = 0, restart_queue = 0, tx_busy = 0;
> u64 alloc_rx_page_failed = 0, alloc_rx_buff_failed = 0;
> + u64 alloc_rx_page = 0;
> u64 bytes = 0, packets = 0, hw_csum_rx_error = 0;
>
> if (test_bit(__IXGBE_DOWN, &adapter->state) ||
> @@ -6791,6 +6793,7 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
> for (i = 0; i < adapter->num_rx_queues; i++) {
> struct ixgbe_ring *rx_ring = adapter->rx_ring[i];
> non_eop_descs += rx_ring->rx_stats.non_eop_descs;
> + alloc_rx_page += rx_ring->rx_stats.alloc_rx_page;
> alloc_rx_page_failed += rx_ring->rx_stats.alloc_rx_page_failed;
> alloc_rx_buff_failed += rx_ring->rx_stats.alloc_rx_buff_failed;
> hw_csum_rx_error += rx_ring->rx_stats.csum_err;
> @@ -6798,6 +6801,7 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
> packets += rx_ring->stats.packets;
> }
> adapter->non_eop_descs = non_eop_descs;
> + adapter->alloc_rx_page = alloc_rx_page;
> adapter->alloc_rx_page_failed = alloc_rx_page_failed;
> adapter->alloc_rx_buff_failed = alloc_rx_buff_failed;
> adapter->hw_csum_rx_error = hw_csum_rx_error;
>
^ permalink raw reply
* [PATCH net-next] bpf: Collapse offset checks in sock_filter_is_valid_access
From: David Ahern @ 2017-09-01 15:18 UTC (permalink / raw)
To: netdev, daniel; +Cc: David Ahern
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>
---
net/core/filter.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 9dad3e7e2e10..f9add024d92f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3468,9 +3468,7 @@ static bool sock_filter_is_valid_access(int off, int size,
if (type == BPF_WRITE) {
switch (off) {
case offsetof(struct bpf_sock, bound_dev_if):
- break;
case offsetof(struct bpf_sock, mark):
- break;
case offsetof(struct bpf_sock, priority):
break;
default:
--
2.1.4
^ permalink raw reply related
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