* Re: [PATCH] can: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2019-02-26 15:24 UTC (permalink / raw)
To: Marc Kleine-Budde, Wolfgang Grandegger, David S. Miller,
Nicolas Ferre, Alexandre Belloni, Ludovic Desroches
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel
In-Reply-To: <31d206cd-65f2-66be-ed79-583210a88d57@pengutronix.de>
On 2/26/19 2:02 AM, Marc Kleine-Budde wrote:
> On 1/29/19 7:06 PM, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>>
>> This patch fixes the following warnings:
>>
>> drivers/net/can/peak_canfd/peak_pciefd_main.c:668:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/spi/mcp251x.c:875:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/usb/peak_usb/pcan_usb.c:422:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/at91_can.c:895:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/at91_can.c:953:15: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> drivers/net/can/usb/peak_usb/pcan_usb.c: In function ‘pcan_usb_decode_error’:
>> drivers/net/can/usb/peak_usb/pcan_usb.c:422:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> if (n & PCAN_USB_ERROR_BUS_LIGHT) {
>> ^
>> drivers/net/can/usb/peak_usb/pcan_usb.c:428:2: note: here
>> case CAN_STATE_ERROR_WARNING:
>> ^~~~
>>
>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>
>> This patch is part of the ongoing efforts to enabling
>> -Wimplicit-fallthrough.
>>
>> Notice that in some cases spelling mistakes were fixed.
>> In other cases, the /* fall through */ comment is placed
>> at the bottom of the case statement, which is what GCC
>> is expecting to find.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Added to linux-can-next.
>
Thanks, Marc.
--
Gustavo
^ permalink raw reply
* Re: [PATCH bpf-next 1/4] bpf: enable program stats
From: Daniel Borkmann @ 2019-02-26 15:29 UTC (permalink / raw)
To: Alexei Starovoitov, Alexei Starovoitov
Cc: Roman Gushchin, Alexei Starovoitov, davem@davemloft.net,
netdev@vger.kernel.org, bpf@vger.kernel.org, Kernel Team
In-Reply-To: <24d2c526-3f9c-3101-ef60-d92777ebdcb7@fb.com>
On 02/26/2019 05:27 AM, Alexei Starovoitov wrote:
> On 2/25/19 2:36 AM, Daniel Borkmann wrote:
>>
>> Not through the stack, but was more thinking something like low-overhead
>> kprobes-style extension for a BPF prog where such sequence would be added
>> 'inline' at beginning / exit of BPF prog invocation with normal ctx access
>> and helpers as the native program (e.g. time-stamping plus read/write into
>> mark as one example which kprobes couldn't do). But might go much beyond
>> context of this stats collection.
>
> see below.
>
>> That's actually an interesting thought, given the original prog->bpf_func
>> is a known address at that time, this could be templated where an inner
>> dummy bpf_func call to the normal BPF prog gets later replaced with the
>> actual prog->bpf_func address to have no indirect call. This would also
>> allow to keep the actual prog->bpf_func entry call-sites small and simple.
>
> My first thought was that we indeed can have a template of stats
> collecting wrapper in .text,
> then at 'switch stats on' event vmalloc exec region, copy wrapper code
> into it and manually patch 'call foo' x86 insn with direct jump.
> That is still quite involved from coding side.
> It's similar to what kprobe/ftrace is doing, but probably
> an overkill for stats due to potential security
> concerns with new executable code.
> But it won't work due to tail_calls.
You mean for the scenario to measure the _individual_ progs that
are part of the given tail call processing such that each have
their individual inc in count and time-spent attribution. If so,
yeah, agree. Or for the case you have right now where everything
gets attributed to the "entry" program that triggers the tail
call processing? Latter would be similar as we have now in this
patch, imho.
> I've looked into single wrapper approach with indirect call
> (because it's much easer than patching x86) and realized it
> won't work for the same reason.
> Inline kprobe+kretprobe insertion won't work either.
> All because we have tail_calls.
> we cannot have executable epilogue in progs.
> tail_call will jmp into next prog and second part of stats collection
> won't run. Essentially no epilogue allowed unless we disable tail_call.
> Or we somehow reimplement tail_calls so that prog returns,
> an epilogue is executed and then jumps into the next prog.
> tail_call turned out be the worst corner case feature to deal with.
Agree, it's a pain as in most cases. Was more thinking that
'special' epilogue would inc the count, start first ktime/cycle
snapshot and then do a direct function call to the normally
JITed BPF prog (where it can do tail calls, etc, so nothing
changes there), and upon return from this call take the second
measurement, and update the per CPU buffer, similar as we have
now just with static keys approach. So, was thinking similar as
we JIT BPF-to-BPF calls on an individual prog basis.
> With static_key approach the main prog accounts the time
> of all progs called via tail_call.
> We can argue whether it's ideal semantics, but looks like
> it's the only thing we can do.
Yeah true, it's more about measuring how long the prog 'chain'
in the whole hook is taking to complete, which may be interesting
in itself, just perhaps a bit confusing when looking at individual
progs' share.
> I don't see a way how tail_called progs can count their own time.
Me neither, if so I guess it would be of similar fashion as you
have right now where it's accounted to the first prog.
> Stats would somehow need to be computed right before jumping
> into next prog. Which means heavy changes in all JITs
> plus extra performance cost at runtime, since such if (stats_on)
> check in tail_call handling would have to be done at runtime,
> since JITed code is read-only and regenerating progs due to stats
> is non-starter.
> Or tail-called next prog would need to update stats for previous
> prog, but there is no pointer to 'aux->stats' there.
> So imo that is dead end.
> static_key approach is the simplest by far.
It's simplest, yep. I guess if we get to the point of removing
indirect call for BPF_PROG_RUN itself, we might need to revisit
then and move it into bpf_func at that point. Kind of implementation
detail given we only expose count and time spent data to uapi,
though the direct switch at runtime may be a bit problematic in
future. Anyway, lets see how it goes when we get there.
Thanks,
Daniel
^ permalink raw reply
* [PATCH net-next] net: sched: fix typo in walker_check_empty()
From: Vlad Buslov @ 2019-02-26 15:34 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
Function walker_check_empty() incorrectly verifies that tp pointer is not
NULL, instead of actual filter pointer. Fix conditional to check the right
pointer. Adjust filter pointer naming accordingly to other cls API
functions.
Fixes: 6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/cls_api.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index cffb7f710de7..578051f8f448 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -238,10 +238,10 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
tcf_proto_destroy(tp, rtnl_held, extack);
}
-static int walker_check_empty(struct tcf_proto *tp, void *d,
+static int walker_check_empty(struct tcf_proto *tp, void *fh,
struct tcf_walker *arg)
{
- if (tp) {
+ if (fh) {
arg->nonempty = true;
return -1;
}
--
2.13.6
^ permalink raw reply related
* [PATCH net-next] tc-testing: gitignore, ignore local tdc config file
From: Vlad Buslov @ 2019-02-26 15:37 UTC (permalink / raw)
To: netdev; +Cc: jhs, davem, shuah, lucasb, Vlad Buslov
Comment in tdc_config.py recommends putting customizations in
tdc_config_local.py file that wasn't included in gitignore. Add the local
config file to gitignore.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
tools/testing/selftests/tc-testing/.gitignore | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/tc-testing/.gitignore b/tools/testing/selftests/tc-testing/.gitignore
index c5cc160948b3..c26d72e0166f 100644
--- a/tools/testing/selftests/tc-testing/.gitignore
+++ b/tools/testing/selftests/tc-testing/.gitignore
@@ -3,3 +3,4 @@ __pycache__/
plugins/
*.xml
*.tap
+tdc_config_local.py
--
2.13.6
^ permalink raw reply related
* [PATCH net] net: aquantia: regression on cpus with high cores: set mode with 8 queues
From: Igor Russkikh @ 2019-02-26 15:39 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev@vger.kernel.org, Igor Russkikh, Dmitry Bogdanov
From: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
Recently the maximum number of queues was increased up to 8, but
NIC was not fully configured for 8 queues. In setups with more than 4 CPU
cores parts of TX traffic gets lost if the kernel routes it to queues 4th-8th.
This patch sets a tx hw traffic mode with 8 queues.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=202651
Fixes: 71a963cfc50b ("net: aquantia: increase max number of hw queues")
Reported-by: Nicholas Johnson <nicholas.johnson@outlook.com.au>
Signed-off-by: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 3 +++
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c | 9 +++++++++
.../ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h | 4 ++++
.../aquantia/atlantic/hw_atl/hw_atl_llh_internal.h | 13 +++++++++++++
4 files changed, 29 insertions(+)
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index b58ca7cb8e9d..fbba300c1d01 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -275,6 +275,9 @@ static int hw_atl_b0_hw_offload_set(struct aq_hw_s *self,
static int hw_atl_b0_hw_init_tx_path(struct aq_hw_s *self)
{
+ /* Tx TC/Queue number config */
+ hw_atl_rpb_tps_tx_tc_mode_set(self, 1U);
+
hw_atl_thm_lso_tcp_flag_of_first_pkt_set(self, 0x0FF6U);
hw_atl_thm_lso_tcp_flag_of_middle_pkt_set(self, 0x0FF6U);
hw_atl_thm_lso_tcp_flag_of_last_pkt_set(self, 0x0F7FU);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
index 939f77e2e117..8ac7a67b15c1 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
@@ -1274,6 +1274,15 @@ void hw_atl_tpb_tx_buff_en_set(struct aq_hw_s *aq_hw, u32 tx_buff_en)
HW_ATL_TPB_TX_BUF_EN_SHIFT, tx_buff_en);
}
+void hw_atl_rpb_tps_tx_tc_mode_set(struct aq_hw_s *aq_hw,
+ u32 tx_traf_class_mode)
+{
+ aq_hw_write_reg_bit(aq_hw, HW_ATL_TPB_TX_TC_MODE_ADDR,
+ HW_ATL_TPB_TX_TC_MODE_MSK,
+ HW_ATL_TPB_TX_TC_MODE_SHIFT,
+ tx_traf_class_mode);
+}
+
void hw_atl_tpb_tx_buff_hi_threshold_per_tc_set(struct aq_hw_s *aq_hw,
u32 tx_buff_hi_threshold_per_tc,
u32 buffer)
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
index 03c570d115fe..f529540bfd7e 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
@@ -605,6 +605,10 @@ void hw_atl_thm_lso_tcp_flag_of_middle_pkt_set(struct aq_hw_s *aq_hw,
/* tpb */
+/* set TX Traffic Class Mode */
+void hw_atl_rpb_tps_tx_tc_mode_set(struct aq_hw_s *aq_hw,
+ u32 tx_traf_class_mode);
+
/* set tx buffer enable */
void hw_atl_tpb_tx_buff_en_set(struct aq_hw_s *aq_hw, u32 tx_buff_en);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
index 8470d92db812..e91ffce005f1 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
@@ -1948,6 +1948,19 @@
/* default value of bitfield tx_buf_en */
#define HW_ATL_TPB_TX_BUF_EN_DEFAULT 0x0
+/* register address for bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_ADDR 0x00007900
+/* bitmask for bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_MSK 0x00000100
+/* inverted bitmask for bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_MSKN 0xFFFFFEFF
+/* lower bit position of bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_SHIFT 8
+/* width of bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_WIDTH 1
+/* default value of bitfield tx_tc_mode */
+#define HW_ATL_TPB_TX_TC_MODE_DEFAULT 0x0
+
/* tx tx{b}_hi_thresh[c:0] bitfield definitions
* preprocessor definitions for the bitfield "tx{b}_hi_thresh[c:0]".
* parameter: buffer {b} | stride size 0x10 | range [0, 7]
--
2.17.1
^ permalink raw reply related
* Re: [RFC] nasty corner case in unix_dgram_sendmsg()
From: Rainer Weikusat @ 2019-02-26 15:31 UTC (permalink / raw)
To: Al Viro; +Cc: Jason Baron, netdev
In-Reply-To: <20190226063804.GI2217@ZenIV.linux.org.uk>
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Tue, Feb 26, 2019 at 06:28:17AM +0000, Al Viro wrote:
[...]
>> * if after relocking we see that unix_peer(sk) now
>> is equal to other, we arrange for wakeup forwarding from other's
>> peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN.
>> Huh?
This returns 1 if sending isn't possible at the moment, ie, if the
process which tries to send has to wait.
^ permalink raw reply
* Re: [PATCH net-next v3 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Stephen Hemminger @ 2019-02-26 15:50 UTC (permalink / raw)
To: Leslie Monis
Cc: davem, netdev, Mohit P . Tahiliani, Dave Taht, Jamal Hadi Salim
In-Reply-To: <20190226082046.GA2266@Inspiron-3521>
On Tue, 26 Feb 2019 13:50:46 +0530
Leslie Monis <lesliemonis@gmail.com> wrote:
> On Mon, Feb 25, 2019 at 04:38:11PM -0800, Stephen Hemminger wrote:
> > On Tue, 26 Feb 2019 00:39:54 +0530
> > Leslie Monis <lesliemonis@gmail.com> wrote:
> >
> > > The current implementation of the PIE queuing discipline is according to the
> > > IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and the paper
> > > [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
> > > However, a lot of necessary modifications and enhancements have been proposed
> > > in RFC 8033, which have not yet been incorporated in the source code of Linux.
> > > This patch series helps in achieving the same.
> > >
> > > Performance tests carried out using Flent [https://flent.org/]
> > >
> > > Changes from v2 to v3:
> > > - Used div_u64() instead of direct division after explicit type casting as
> > > recommended by David
> > >
> > > Changes from v1 to v2:
> > > - Excluded the patch setting PIE dynamically active/inactive as the test
> > > results were unsatisfactory
> > > - Fixed a scaling issue when adding more auto-tuning cases which caused
> > > local variables to underflow
> > > - Changed the long if/else chain to a loop as suggested by Stephen
> > > - Changed the position of the accu_prob variable in the pie_vars
> > > structure as recommended by Stephen
> > >
> > > Mohit P. Tahiliani (7):
> > > net: sched: pie: change value of QUEUE_THRESHOLD
> > > net: sched: pie: change default value of pie_params->target
> > > net: sched: pie: change default value of pie_params->tupdate
> > > net: sched: pie: change initial value of pie_vars->burst_time
> > > net: sched: pie: add more cases to auto-tune alpha and beta
> > > net: sched: pie: add derandomization mechanism
> > > net: sched: pie: update references
> > >
> > > include/uapi/linux/pkt_sched.h | 2 +-
> > > net/sched/sch_pie.c | 107 ++++++++++++++++++++-------------
> > > 2 files changed, 66 insertions(+), 43 deletions(-)
> >
> > Are you concerned at all that changes to default values might change
> > expected behavior of existing users?
>
> Hi Stephen,
>
> As Dave mentioned, the changes which we have made do not really change the
> behaviour of the aqm drastically. Our performance tests show that these changes
> improve performance without any side-effects. So existing users (if there are
> any) should not be negatively affected in any way.
Thanks for answering. Looks good
^ permalink raw reply
* Re: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Eric Dumazet @ 2019-02-26 15:52 UTC (permalink / raw)
To: Sheng Lan, Stephen Hemminger
Cc: davem, netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji,
liuzhiqiang26, yuehaibing
In-Reply-To: <05fa74b9-6e3b-7bd3-fa1c-c02e37a521f8@huawei.com>
On 02/26/2019 05:02 AM, Sheng Lan wrote:
>
>
>
>> On Mon, 25 Feb 2019 22:49:39 +0800
>> Sheng Lan <lansheng@huawei.com> wrote:
>>
>>> From: Sheng Lan <lansheng@huawei.com>
>>> Subject: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
>>>
>>> It can be reproduced by following steps:
>>> 1. virtio_net NIC is configured with gso/tso on
>>> 2. configure nginx as http server with an index file bigger than 1M bytes
>>> 3. use tc netem to produce duplicate packets and delay:
>>> tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
>>> 4. continually curl the nginx http server to get index file on client
>>> 5. BUG_ON is seen quickly
>>>
>>> [10258690.371129] kernel BUG at net/core/skbuff.c:4028!
>>> [10258690.371748] invalid opcode: 0000 [#1] SMP PTI
>>> [10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
>>> [10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
>>> [10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
>>> [10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
>>> [10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
>>> [10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
>>> [10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
>>> [10258690.372094] Call Trace:
>>> [10258690.372094] <IRQ>
>>> [10258690.372094] skb_to_sgvec+0x11/0x40
>>> [10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
>>> [10258690.372094] dev_hard_start_xmit+0x9b/0x200
>>> [10258690.372094] sch_direct_xmit+0xff/0x260
>>> [10258690.372094] __qdisc_run+0x15e/0x4e0
>>> [10258690.372094] net_tx_action+0x137/0x210
>>> [10258690.372094] __do_softirq+0xd6/0x2a9
>>> [10258690.372094] irq_exit+0xde/0xf0
>>> [10258690.372094] smp_apic_timer_interrupt+0x74/0x140
>>> [10258690.372094] apic_timer_interrupt+0xf/0x20
>>> [10258690.372094] </IRQ>
>>>
>>> In __skb_to_sgvec, the skb->len is not equal to the sum of the skb's
>>> linear data size and nonlinear data size, thus BUG_ON triggered. The
>>> bad skb's nonlinear data size is less than skb->data_len, because the
>>> skb is cloned and a part of related cloned skb's nonlinear data is
>>> split off.
>>>
>>> Duplicate packet is cloned by skb_clone in netem_enqueue and may be delayed
>>> some time in qdisc. Due to the delay time, the original skb will be pushed
>>> again later in __tcp_push_pending_frames when tcp receives new packets.
>>> In tcp_write_xmit, when the tcp_mss_split_point returns a smaller limit,
>>> the original skb will be fragmented and the skb's nonlinear data will be
>>> split off. The length of the skb cloned by netem will not be updated.
>>> When we use virtio_net NIC, the duplicated cloned skb will be filled into
>>> a scatter-gather list in __skb_to_sgvec and trigger the BUG_ON.
>>>
>>> Here I replace the skb_clone with skb_copy in netem_enqueue to ensure
>>> the duplicated skb's nonlinear data is independent.
>>>
>>> Signed-off-by: Sheng Lan <lansheng@huawei.com>
>>> Reported-by: Qin Ji <jiqin.ji@huawei.com>
>>>
>>> Fixes: 0afb51e7 ("netem: reinsert for duplication")
>>
>> This sounds like a bug in the other layers (either TCP or Virtio net)
>> not handling a cloned skb properly.
>>
>
> I have traced the route of skb by printk, let me take an example to describe the problem to make it clearly:
> Mss value equals to 1448. Limit value is the split size when tcp do tso_fragment, is depending on the size of the sending congestion window and mss value.
>
> TCP layer transmit the index file to client, the original skb1 size is large:
> ...
> tcp_write_xmit (skb1->data_len == 62264, limit == 2*mss == 2896)
> tso_fragment (it needs to be fragmented by limit value)
> skb_split (after split, skb1->data_len == 2896, skb_shinfo(skb1)->frags[0] == 2896, skb_shinfo(skb1)->nr_frags == 1)
> ...
> netem_enqueue (netem construct a duplicate packet of skb1 by skb_clone)
> skb2 = skb_clone(skb1) (skb1->data_len == skb2->data_len == 2896, skb1 and skb2 share the nonlinear data frags[0] == 2896)
> waiting 30ms (skb1 and skb2 will be delayed in qdisc queue due to the netem delay configuration)
>
>
> TCP layer receives new packets and trys to retransmit the skb1:
> tcp_rcv_established
> __tcp_push_pending_frames
> tcp_write_xmit (skb1->data_len == 2896, cwnd size decreased or packets in flight increased, cause the limit decreased to 1*mss == 1448)
tcp_write_xmit() only deals with packet in the write queue,
they never were sent. They can not be any clone of them by definition, since
skbs in the TCP write queue are private to TCP stack,
Once a packet is sent, the master skb is moved to the rtx rb-tree,
while the clone is going through lower stacks.
When/if a retransmit is due, we always make sure there is no clone on it,
look at the various calls to skb_unclone()
> tso_fragment (limit value is less than skb1->data_len, skb1 will be fragmented again)
> skb_split (the second time split, skb1 is cloned now and share nonlinear data with skb2.
> skb1->data_len == 1448, frags[0] == 1448, a part of shared nonlinear data has been split off)
>
> Now we can see:
> skb1->data_len == 1448
> skb2->data_len == 2896 (2896 is wrong value)
> frags[0] == 1448
>
>
> The route of skb2:
> netem_enqueue (netem construct a duplicate packet, skb2 = skb_clone(skb1), put skb2 into queue)
> waiting 30ms (delayed packet)
> ...
> netem_dequeue (skb2->data_len == 2896, frags[0] == 1448)
> sch_direct_xmit
> dev_hard_start_xmit
> xmit_one
> start_xmit [virtio_net driver]
> skb_to_sgvec (BUG_ON here )
>
>
> The key is that skb be split by skb_split in tso_fragment again after netem clone it and share nonlinear data with another skb.
> Processing of TCP seems OK, which push and fragment delayed packets in write queue. And virtio_net is the trigger of the BUG_ON.
> So I replaced skb_clone with skb_copy in netem_enqueue, and the method worked. Currently, I have no better idea to fix it,
> would you give me some inspiring advice ? If I am wrong, please correct me.
>
> Thanks
>
^ permalink raw reply
* Re: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Stephen Hemminger @ 2019-02-26 15:59 UTC (permalink / raw)
To: Sheng Lan
Cc: davem, netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji,
liuzhiqiang26, yuehaibing
In-Reply-To: <05fa74b9-6e3b-7bd3-fa1c-c02e37a521f8@huawei.com>
On Tue, 26 Feb 2019 21:02:10 +0800
Sheng Lan <lansheng@huawei.com> wrote:
> > On Mon, 25 Feb 2019 22:49:39 +0800
> > Sheng Lan <lansheng@huawei.com> wrote:
> >
> >> From: Sheng Lan <lansheng@huawei.com>
> >> Subject: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
> >>
> >> It can be reproduced by following steps:
> >> 1. virtio_net NIC is configured with gso/tso on
> >> 2. configure nginx as http server with an index file bigger than 1M bytes
> >> 3. use tc netem to produce duplicate packets and delay:
> >> tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
> >> 4. continually curl the nginx http server to get index file on client
> >> 5. BUG_ON is seen quickly
> >>
> >> [10258690.371129] kernel BUG at net/core/skbuff.c:4028!
> >> [10258690.371748] invalid opcode: 0000 [#1] SMP PTI
> >> [10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
> >> [10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
> >> [10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
> >> [10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
> >> [10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
> >> [10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
> >> [10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
> >> [10258690.372094] Call Trace:
> >> [10258690.372094] <IRQ>
> >> [10258690.372094] skb_to_sgvec+0x11/0x40
> >> [10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
> >> [10258690.372094] dev_hard_start_xmit+0x9b/0x200
> >> [10258690.372094] sch_direct_xmit+0xff/0x260
> >> [10258690.372094] __qdisc_run+0x15e/0x4e0
> >> [10258690.372094] net_tx_action+0x137/0x210
> >> [10258690.372094] __do_softirq+0xd6/0x2a9
> >> [10258690.372094] irq_exit+0xde/0xf0
> >> [10258690.372094] smp_apic_timer_interrupt+0x74/0x140
> >> [10258690.372094] apic_timer_interrupt+0xf/0x20
> >> [10258690.372094] </IRQ>
> >>
> >> In __skb_to_sgvec, the skb->len is not equal to the sum of the skb's
> >> linear data size and nonlinear data size, thus BUG_ON triggered. The
> >> bad skb's nonlinear data size is less than skb->data_len, because the
> >> skb is cloned and a part of related cloned skb's nonlinear data is
> >> split off.
> >>
> >> Duplicate packet is cloned by skb_clone in netem_enqueue and may be delayed
> >> some time in qdisc. Due to the delay time, the original skb will be pushed
> >> again later in __tcp_push_pending_frames when tcp receives new packets.
> >> In tcp_write_xmit, when the tcp_mss_split_point returns a smaller limit,
> >> the original skb will be fragmented and the skb's nonlinear data will be
> >> split off. The length of the skb cloned by netem will not be updated.
> >> When we use virtio_net NIC, the duplicated cloned skb will be filled into
> >> a scatter-gather list in __skb_to_sgvec and trigger the BUG_ON.
> >>
> >> Here I replace the skb_clone with skb_copy in netem_enqueue to ensure
> >> the duplicated skb's nonlinear data is independent.
> >>
> >> Signed-off-by: Sheng Lan <lansheng@huawei.com>
> >> Reported-by: Qin Ji <jiqin.ji@huawei.com>
> >>
> >> Fixes: 0afb51e7 ("netem: reinsert for duplication")
> >
> > This sounds like a bug in the other layers (either TCP or Virtio net)
> > not handling a cloned skb properly.
> >
>
> I have traced the route of skb by printk, let me take an example to describe the problem to make it clearly:
> Mss value equals to 1448. Limit value is the split size when tcp do tso_fragment, is depending on the size of the sending congestion window and mss value.
>
> TCP layer transmit the index file to client, the original skb1 size is large:
> ...
> tcp_write_xmit (skb1->data_len == 62264, limit == 2*mss == 2896)
> tso_fragment (it needs to be fragmented by limit value)
> skb_split (after split, skb1->data_len == 2896, skb_shinfo(skb1)->frags[0] == 2896, skb_shinfo(skb1)->nr_frags == 1)
> ...
> netem_enqueue (netem construct a duplicate packet of skb1 by skb_clone)
> skb2 = skb_clone(skb1) (skb1->data_len == skb2->data_len == 2896, skb1 and skb2 share the nonlinear data frags[0] == 2896)
> waiting 30ms (skb1 and skb2 will be delayed in qdisc queue due to the netem delay configuration)
>
>
> TCP layer receives new packets and trys to retransmit the skb1:
> tcp_rcv_established
> __tcp_push_pending_frames
> tcp_write_xmit (skb1->data_len == 2896, cwnd size decreased or packets in flight increased, cause the limit decreased to 1*mss == 1448)
> tso_fragment (limit value is less than skb1->data_len, skb1 will be fragmented again)
Maybe the fix is to stop TSO fragment from overwriting by doing something like:
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 730bc44dbad9..5fe91d0224f6 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1856,7 +1856,7 @@ static int tso_fragment(struct sock *sk, enum tcp_queue tcp_queue,
u8 flags;
/* All of a TSO frame must be composed of paged data. */
- if (skb->len != skb->data_len)
+ if (skb->len != skb->data_len || skb_cloned(skb))
return tcp_fragment(sk, tcp_queue, skb, len, mss_now, gfp);
buff = sk_stream_alloc_skb(sk, 0, gfp, true);
^ permalink raw reply related
* Re: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Eric Dumazet @ 2019-02-26 16:08 UTC (permalink / raw)
To: Stephen Hemminger, Sheng Lan
Cc: davem, netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji,
liuzhiqiang26, yuehaibing
In-Reply-To: <20190226075906.103fa072@shemminger-XPS-13-9360>
On 02/26/2019 07:59 AM, Stephen Hemminger wrote:
>
>
> Maybe the fix is to stop TSO fragment from overwriting by doing something like:
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 730bc44dbad9..5fe91d0224f6 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1856,7 +1856,7 @@ static int tso_fragment(struct sock *sk, enum tcp_queue tcp_queue,
> u8 flags;
>
> /* All of a TSO frame must be composed of paged data. */
> - if (skb->len != skb->data_len)
> + if (skb->len != skb->data_len || skb_cloned(skb))
> return tcp_fragment(sk, tcp_queue, skb, len, mss_now, gfp);
>
> buff = sk_stream_alloc_skb(sk, 0, gfp, true);
>
tso_fragment() is only called with packets that were not yet transmit.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: don't release block->lock when dumping chains
From: Vlad Buslov @ 2019-02-26 16:09 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpXQXMxWDTcFvEEXra_1uKujhbeb630_kPfBqO1R8uwYtA@mail.gmail.com>
On Tue 26 Feb 2019 at 00:15, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 25, 2019 at 7:45 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Function tc_dump_chain() obtains and releases block->lock on each iteration
>> of its inner loop that dumps all chains on block. Outputting chain template
>> info is fast operation so locking/unlocking mutex multiple times is an
>> overhead when lock is highly contested. Modify tc_dump_chain() to only
>> obtain block->lock once and dump all chains without releasing it.
>>
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> Thanks for the followup!
>
> Isn't it similar for __tcf_get_next_proto() in tcf_chain_dump()?
> And for tc_dump_tfilter()?
Not really. These two dump all tp filters and not just a template, which
is O(n) on number of filters and can be slow because it calls hw offload
API for each of them. Our typical use-case involves periodic filter dump
(to update stats) while multiple concurrent user-space threads are
updating filters, so it is important for them to be able to execute in
parallel.
^ permalink raw reply
* Re: linux-next: Tree for Feb 26 (net/sched/sch_pie.c)
From: Randy Dunlap @ 2019-02-26 16:21 UTC (permalink / raw)
To: Stephen Rothwell, Linux Next Mailing List
Cc: Linux Kernel Mailing List, netdev@vger.kernel.org,
Vijay Subramanian, Mythili Prabhu
In-Reply-To: <20190226190859.624e21f0@canb.auug.org.au>
On 2/26/19 12:08 AM, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20190225:
>
on i386:
ld: net/sched/sch_pie.o: in function `pie_timer':
sch_pie.c:(.text+0x604): undefined reference to `__udivdi3'
--
~Randy
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Ard Biesheuvel @ 2019-02-26 16:21 UTC (permalink / raw)
To: Marc Zyngier
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, Devicetree List, <netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel
In-Reply-To: <5310b73b-4821-6dff-b9c0-34c59fb7fd72@arm.com>
On Mon, 25 Feb 2019 at 15:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Ard,
>
> On 25/02/2019 12:45, Ard Biesheuvel wrote:
> > On Sun, 24 Feb 2019 at 15:08, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>
> >> For quite some time, I wondered why the PCI mwifiex device built in my
> >> Chromebook was unable to use the good old legacy interrupts. But as MSIs
> >> were working fine, I never really bothered investigating. I finally had a
> >> look, and the result isn't very pretty.
> >>
> >> On this machine (rk3399-based kevin), the wake-up interrupt is described as
> >> such:
> >>
> >> &pci_rootport {
> >> mvl_wifi: wifi@0,0 {
> >> compatible = "pci1b4b,2b42";
> >> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
> >> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
> >> interrupt-parent = <&gpio0>;
> >> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&wlan_host_wake_l>;
> >> wakeup-source;
> >> };
> >> };
> >>
> >> Note how the interrupt is part of the properties directly attached to the
> >> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
> >> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
> >> altogether (Yay for the broken design!). This is in total violation of the
> >> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
> >> specifiers describe the PCI device interrupts, and must obey the
> >> INT-{A,B,C,D} mapping. Oops!
> >>
> >
> > The mapping of legacy PCIe INTx interrupts onto wired system
> > interrupts is a property of the PCIe host controller, not of a
> > particular PCIe device. So I would argue that the code is broken here
> > as well: it should never attempt to interpret 'interrupt' properties
> > at the PCI device level as having any bearing on how legacy interrupts
> > are routed.
>
> OpenFirmware says that this node contains the interrupt number of the
> device (4.1.1. Open Firmware-defined Properties for Child Nodes). The
> trick is that this property is generated *from* the device, and not set
> in stone.
>
> DT, on the other hand, takes whatever is described there and uses it as
> the gospel to configure the OS, no matter how the PCI device is actually
> configured. If the two don't match (like in this case), things break.
> This is the "DT describes the HW" mantra, for (sometimes) better or
> (more generally) worse.
>
> What the DT code does is to interpret the whole interrupt specifier,
> *including the interrupt-parent*. And that feels wrong. It should always
> be in the context of the host controller. But on the other side, the DT
> code is not in the business of validating the DT either...
>
> It outlines one thing: If you have to interpret per-device PCI
> properties from DT, you're in for serious trouble. I should get some
> better HW.
>
Yeah, it obviously makes no sense at all for the interrupt parent of a
PCI device to deviate from the host bridge's interrupt parent, and
it's quite unfortunate that we can't simply ban it now that the cat is
out of the bag already.
Arguably, the wake up widget is not part of the PCI device, but I have
no opinion as to whether it is better modeling it as a sub device as
you are proposing or as an entirely separate device referenced via a
phandle.
^ permalink raw reply
* Re: [PATCH net-next] net: remove unused struct inet_frag_queue.fragments field
From: Stefan Schmidt @ 2019-02-26 16:22 UTC (permalink / raw)
To: Peter Oskolkov, David Miller, netdev; +Cc: Peter Oskolkov, linux-wpan
In-Reply-To: <20190226014346.77719-1-posk@google.com>
Hello Peter.
On 26.02.19 02:43, Peter Oskolkov wrote:
> Now that all users of struct inet_frag_queue have been converted
> to use 'rb_fragments', remove the unused 'fragments' field.
>
> Build with `make allyesconfig` succeeded. ip_defrag selftest passed.
>
> Signed-off-by: Peter Oskolkov <posk@google.com>
> ---
> include/net/inet_frag.h | 4 +--
> net/ieee802154/6lowpan/reassembly.c | 1 -
> net/ipv4/inet_fragment.c | 44 ++++++++-----------------
> net/ipv4/ip_fragment.c | 2 --
> net/ipv6/netfilter/nf_conntrack_reasm.c | 1 -
> net/ipv6/reassembly.c | 1 -
> 6 files changed, 14 insertions(+), 39 deletions(-)
>
> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
> index b02bf737d019..378904ee9129 100644
> --- a/include/net/inet_frag.h
> +++ b/include/net/inet_frag.h
> @@ -56,7 +56,6 @@ struct frag_v6_compare_key {
> * @timer: queue expiration timer
> * @lock: spinlock protecting this frag
> * @refcnt: reference count of the queue
> - * @fragments: received fragments head
> * @rb_fragments: received fragments rb-tree root
> * @fragments_tail: received fragments tail
> * @last_run_head: the head of the last "run". see ip_fragment.c
> @@ -77,8 +76,7 @@ struct inet_frag_queue {
> struct timer_list timer;
> spinlock_t lock;
> refcount_t refcnt;
> - struct sk_buff *fragments; /* used in 6lopwpan IPv6. */
> - struct rb_root rb_fragments; /* Used in IPv4/IPv6. */
> + struct rb_root rb_fragments;
> struct sk_buff *fragments_tail;
> struct sk_buff *last_run_head;
> ktime_t stamp;
> diff --git a/net/ieee802154/6lowpan/reassembly.c b/net/ieee802154/6lowpan/reassembly.c
> index bd61633d2c32..4196bcd4105a 100644
> --- a/net/ieee802154/6lowpan/reassembly.c
> +++ b/net/ieee802154/6lowpan/reassembly.c
> @@ -179,7 +179,6 @@ static int lowpan_frag_reasm(struct lowpan_frag_queue *fq, struct sk_buff *skb,
>
> skb->dev = ldev;
> skb->tstamp = fq->q.stamp;
> - fq->q.fragments = NULL;
> fq->q.rb_fragments = RB_ROOT;
> fq->q.fragments_tail = NULL;
> fq->q.last_run_head = NULL;
> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index 9f69411251d0..737808e27f8b 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -203,7 +203,6 @@ EXPORT_SYMBOL(inet_frag_rbtree_purge);
>
> void inet_frag_destroy(struct inet_frag_queue *q)
> {
> - struct sk_buff *fp;
> struct netns_frags *nf;
> unsigned int sum, sum_truesize = 0;
> struct inet_frags *f;
> @@ -212,20 +211,9 @@ void inet_frag_destroy(struct inet_frag_queue *q)
> WARN_ON(del_timer(&q->timer) != 0);
>
> /* Release all fragment data. */
> - fp = q->fragments;
> nf = q->net;
> f = nf->f;
> - if (fp) {
> - do {
> - struct sk_buff *xp = fp->next;
> -
> - sum_truesize += fp->truesize;
> - kfree_skb(fp);
> - fp = xp;
> - } while (fp);
> - } else {
> - sum_truesize = inet_frag_rbtree_purge(&q->rb_fragments);
> - }
> + sum_truesize = inet_frag_rbtree_purge(&q->rb_fragments);
> sum = sum_truesize + f->qsize;
>
> call_rcu(&q->rcu, inet_frag_destroy_rcu);
> @@ -489,26 +477,20 @@ EXPORT_SYMBOL(inet_frag_reasm_finish);
>
> struct sk_buff *inet_frag_pull_head(struct inet_frag_queue *q)
> {
> - struct sk_buff *head;
> + struct sk_buff *head, *skb;
>
> - if (q->fragments) {
> - head = q->fragments;
> - q->fragments = head->next;
> - } else {
> - struct sk_buff *skb;
> + head = skb_rb_first(&q->rb_fragments);
> + if (!head)
> + return NULL;
> + skb = FRAG_CB(head)->next_frag;
> + if (skb)
> + rb_replace_node(&head->rbnode, &skb->rbnode,
> + &q->rb_fragments);
> + else
> + rb_erase(&head->rbnode, &q->rb_fragments);
> + memset(&head->rbnode, 0, sizeof(head->rbnode));
> + barrier();
>
> - head = skb_rb_first(&q->rb_fragments);
> - if (!head)
> - return NULL;
> - skb = FRAG_CB(head)->next_frag;
> - if (skb)
> - rb_replace_node(&head->rbnode, &skb->rbnode,
> - &q->rb_fragments);
> - else
> - rb_erase(&head->rbnode, &q->rb_fragments);
> - memset(&head->rbnode, 0, sizeof(head->rbnode));
> - barrier();
> - }
> if (head == q->fragments_tail)
> q->fragments_tail = NULL;
>
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index 486ecb0aeb87..cf2b0a6a3337 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -261,7 +261,6 @@ static int ip_frag_reinit(struct ipq *qp)
> qp->q.flags = 0;
> qp->q.len = 0;
> qp->q.meat = 0;
> - qp->q.fragments = NULL;
> qp->q.rb_fragments = RB_ROOT;
> qp->q.fragments_tail = NULL;
> qp->q.last_run_head = NULL;
> @@ -451,7 +450,6 @@ static int ip_frag_reasm(struct ipq *qp, struct sk_buff *skb,
> ip_send_check(iph);
>
> __IP_INC_STATS(net, IPSTATS_MIB_REASMOKS);
> - qp->q.fragments = NULL;
> qp->q.rb_fragments = RB_ROOT;
> qp->q.fragments_tail = NULL;
> qp->q.last_run_head = NULL;
> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
> index cb1b4772dac0..3de0e9b0a482 100644
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@ -365,7 +365,6 @@ static int nf_ct_frag6_reasm(struct frag_queue *fq, struct sk_buff *skb,
> skb_network_header_len(skb),
> skb->csum);
>
> - fq->q.fragments = NULL;
> fq->q.rb_fragments = RB_ROOT;
> fq->q.fragments_tail = NULL;
> fq->q.last_run_head = NULL;
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 24264d0a4b85..1a832f5e190b 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -304,7 +304,6 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb,
> rcu_read_lock();
> __IP6_INC_STATS(net, __in6_dev_get(dev), IPSTATS_MIB_REASMOKS);
> rcu_read_unlock();
> - fq->q.fragments = NULL;
> fq->q.rb_fragments = RB_ROOT;
> fq->q.fragments_tail = NULL;
> fq->q.last_run_head = NULL;
>
For the ieee802154 part:
Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
regards
Stefan Schmidt
^ permalink raw reply
* Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()
From: Daniel Borkmann @ 2019-02-26 16:24 UTC (permalink / raw)
To: zerons, ast; +Cc: netdev, linux-kernel
In-Reply-To: <288b6db2-b14a-4f85-b89c-d873f0f6fcd8@gmail.com>
On 02/26/2019 03:58 PM, zerons wrote:
> On 2/26/19 22:44, Daniel Borkmann wrote:
>> On 02/26/2019 03:15 PM, zerons wrote:
>>> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
>>
>> Thanks for the fix! What do you mean by "upstream commit" above in this context?
>
> This patch is based on that commit, I thought I should mention this.
> Sorry for the confusion.
Ah, no. Usually that line is placed into stable commits that are
cherry-picked to point to the original commit sha in the mainline
tree. In any case, I didn't find c91951f15978, hence my question.
Anyway, for normal patch submissions in future, please don't add
it into the log, we'll remove it this time.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net-next] net: remove unused struct inet_frag_queue.fragments field
From: David Miller @ 2019-02-26 16:27 UTC (permalink / raw)
To: posk; +Cc: netdev, posk, linux-wpan
In-Reply-To: <20190226014346.77719-1-posk@google.com>
From: Peter Oskolkov <posk@google.com>
Date: Mon, 25 Feb 2019 17:43:46 -0800
> Now that all users of struct inet_frag_queue have been converted
> to use 'rb_fragments', remove the unused 'fragments' field.
>
> Build with `make allyesconfig` succeeded. ip_defrag selftest passed.
>
> Signed-off-by: Peter Oskolkov <posk@google.com>
Applied, thanks Peter.
^ permalink raw reply
* Re: AF_XDP design flaws
From: John Fastabend @ 2019-02-26 16:41 UTC (permalink / raw)
To: Maxim Mikityanskiy, netdev@vger.kernel.org, Björn Töpel,
Magnus Karlsson, David S. Miller
Cc: Tariq Toukan, Saeed Mahameed, Eran Ben Elisha
In-Reply-To: <AM6PR05MB5879DF6B2BD7DC426869875ED17B0@AM6PR05MB5879.eurprd05.prod.outlook.com>
On 2/26/19 6:49 AM, Maxim Mikityanskiy wrote:
> Hi everyone,
>
> I would like to discuss some design flaws of AF_XDP socket (XSK) implementation
> in kernel. At the moment I don't see a way to work around them without changing
> the API, so I would like to make sure that I'm not missing anything and to
> suggest and discuss some possible improvements that can be made.
>
> The issues I describe below are caused by the fact that the driver depends on
> the application doing some things, and if the application is
> slow/buggy/malicious, the driver is forced to busy poll because of the lack of a
> notification mechanism from the application side. I will refer to the i40e
> driver implementation a lot, as it is the first implementation of AF_XDP, but
> the issues are general and affect any driver. I already considered trying to fix
> it on driver level, but it doesn't seem possible, so it looks like the behavior
> and implementation of AF_XDP in the kernel has to be changed.
>
> RX side busy polling
> ====================
>
> On the RX side, the driver expects the application to put some descriptors in
> the Fill Ring. There is no way for the application to notify the driver that
> there are more Fill Ring descriptors to take, so the driver is forced to busy
> poll the Fill Ring if it gets empty. E.g., the i40e driver does it in NAPI poll:
>
> int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> {
> ...
> failure = failure ||
> !i40e_alloc_rx_buffers_fast_zc(rx_ring,
> cleaned_count);
> ...
> return failure ? budget : (int)total_rx_packets;
> }
>
> Basically, it means that if there are no descriptors in the Fill Ring, NAPI will
> never stop, draining CPU.
>
> Possible cases when it happens
> ------------------------------
>
> 1. The application is slow, it received some frames in the RX Ring, and it is
> still handling the data, so it has no free frames to put to the Fill Ring.
>
> 2. The application is malicious, it opens an XSK and puts no frames to the Fill
> Ring. It can be used as a local DoS attack.
>
> 3. The application is buggy and stops filling the Fill Ring for whatever reason
> (deadlock, waiting for another blocking operation, other bugs).
>
> Although loading an XDP program requires root access, the DoS attack can be
> targeted to setups that already use XDP, i.e. an XDP program is already loaded.
> Even under root, userspace applications should not be able to disrupt system
> stability by just calling normal APIs without an intention to destroy the
> system, and here it happens in case 1.
I believe this is by design. If packets per second is your top priority
(at the expense of power, cpu, etc.) this is the behavior you might
want. To resolve your points if you don't trust the application it
should be isolated to a queue pair and cores so the impact is known and
managed.
That said having a flag to back-off seems like a good idea. But should
be doable as a feature without breaking current API.
>
> Possible way to solve the issue
> -------------------------------
>
> When the driver can't take new Fill Ring frames, it shouldn't busy poll.
> Instead, it signals the failure to the application (e.g., with POLLERR), and
> after that it's up to the application to restart polling (e.g., by calling
> sendto()) after refilling the Fill Ring. The issue with this approach is that it
> changes the API, so we either have to deal with it or to introduce some API
> version field.
See above. I like the idea here though.
>
> TX side getting stuck
> =====================
>
> On the TX side, there is the Completion Ring that the application has to clean.
> If it doesn't, the i40e driver stops taking descriptors from the TX Ring. If the
> application finally completes something, the driver can go on transmitting.
> However, it would require busy polling the Completion Ring (just like with the
> Fill Ring on the RX side). i40e doesn't do it, instead, it relies on the
> application to kick the TX by calling sendto(). The issue is that poll() doesn't
> return POLLOUT in this case, because the TX Ring is full, so the application
> will never call sendto(), and the ring is stuck forever (or at least until
> something triggers NAPI).
>
> Possible way to solve the issue
> -------------------------------
>
> When the driver can't reserve a descriptor in the Completion Ring, it should
> signal the failure to the application (e.g., with POLLERR). The application
> shouldn't call sendto() every time it sees that the number of not completed
> frames is greater than zero (like xdpsock sample does). Instead, the application
> should kick the TX only when it wants to flush the ring, and, in addition, after
> resolving the cause for POLLERR, i.e. after handling Completion Ring entries.
> The API will also have to change with this approach.
>
+1 seems to me this can be done without breaking existing API though.
> Triggering NAPI on a different CPU core
> =======================================
>
> .ndo_xsk_async_xmit runs on a random CPU core, so, to preserve CPU affinity,
> i40e triggers an interrupt to schedule NAPI, instead of calling napi_schedule
> directly. Scheduling NAPI on the correct CPU is what would every driver do, I
> guess, but currently it has to be implemented differently in every driver, and
> it relies on hardware features (the ability to trigger an IRQ).
Ideally the application would be pinned to a core and the traffic
steered to that core using ethtool/tc. Yes it requires a bit of mgmt on
the platform but I think this is needed for best pps numbers.
>
> I suggest introducing a kernel API that would allow triggering NAPI on a given
> CPU. A brief look shows that something like smp_call_function_single_async can
> be used. Advantages:
Assuming you want to avoid pinning cores/traffic for some reason. Could
this be done with some combination of cpumap + af_xdp? I haven't thought
too much about it though. Maybe Bjorn has some ideas.
>
> 1. It lifts the hardware requirement to be able to raise an interrupt on demand.
>
> 2. It would allow to move common code to the kernel (.ndo_xsk_async_xmit).
>
> 3. It is also useful in the situation where CPU affinity changes while being in
> NAPI poll. Currently, i40e and mlx5e try to stop NAPI polling by returning
> a value less than budget if CPU affinity changes. However, there are cases
> (e.g., NAPIF_STATE_MISSED) when NAPI will be rescheduled on a wrong CPU. It's a
> race between the interrupt, which will move NAPI to the correct CPU, and
> __napi_schedule from a wrong CPU. Having an API to schedule NAPI on a given CPU
> will benefit both mlx5e and i40e, because when this situation happens, it kills
> the performance.
How would we know what core to trigger NAPI on?
>
> I would be happy to hear your thoughts about these issues.
At least the first two observations seem incrementally solvable to me
and would be nice improvements. I assume the last case can be resolved
by pinning cores/traffic but for the general case perhaps it is useful.
>
> Thanks,
> Max
>
^ permalink raw reply
* Re: [PATCH net-next v4 0/6] devlink: make ethtool compat reliable
From: David Miller @ 2019-02-26 16:50 UTC (permalink / raw)
To: jakub.kicinski; +Cc: jiri, mkubecek, andrew, f.fainelli, netdev, oss-drivers
In-Reply-To: <20190226033407.32625-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Mon, 25 Feb 2019 19:34:01 -0800
> This is a follow up to the series which added device flash
> updates via devlink. I went with the approach of adding a
> new NDO in the end. It seems to end up looking cleaner.
>
> First patch removes the option to build devlink as a module.
> Users can still decide to not build it, but the module option
> ends up not being worth the maintenance cost.
>
> Next two patches add a NDO which can be used to ask the driver
> to return a devlink instance associated with a given netdev,
> instead of iterating over devlink ports. Drivers which implement
> this NDO must take into account the potential impact on the
> visibility of the devlink instance.
>
> With the new NDO in place we can remove NFP ethtool flash update
> code.
>
> Fifth patch makes sure we hold a reference to dev while
> callbacks are active.
>
> Last but not least the NULL-check of devlink->ops is moved
> to instance allocation time.
>
> Last but not least missing checks for devlink->ops are added.
> There is currently no driver registering devlink without ops,
> so can just fix this in -next.
>
> v2 (Michal): add netdev_to_devlink() in patch 3.
> v3 (Florian):
> - add missing checks for devlink->ops;
> - move locking/holding into devlink_compat_ functions.
> v4 (Jiri):
> - hold devlink_mutex around callbacks (patch 2);
> - require non-NULL ops (patch 6).
Series applied, thanks Jakub.
^ permalink raw reply
* [PATCH net 1/3] ipv4: Return error for RTA_VIA attribute
From: David Ahern @ 2019-02-26 17:00 UTC (permalink / raw)
To: davem; +Cc: netdev, David Ahern
In-Reply-To: <20190226170004.4535-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
IPv4 currently does not support nexthops outside of the AF_INET family.
Specifically, it does not handle RTA_VIA attribute. If it is passed
in a route add request, the actual route added only uses the device
which is clearly not what the user intended:
$ ip ro add 172.16.1.0/24 via inet6 2001:db8:1::1 dev eth0
$ ip ro ls
...
172.16.1.0/24 dev eth0
Catch this and fail the route add:
$ ip ro add 172.16.1.0/24 via inet6 2001:db8:1::1 dev eth0
Error: IPv4 does not support RTA_VIA attribute.
Fixes: 03c0566542f4c ("mpls: Netlink commands to add, remove, and dump routes")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv4/fib_frontend.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index fe4f6a624238..ed14ec245584 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -710,6 +710,10 @@ static int rtm_to_fib_config(struct net *net, struct sk_buff *skb,
case RTA_GATEWAY:
cfg->fc_gw = nla_get_be32(attr);
break;
+ case RTA_VIA:
+ NL_SET_ERR_MSG(extack, "IPv4 does not support RTA_VIA attribute");
+ err = -EINVAL;
+ goto errout;
case RTA_PRIORITY:
cfg->fc_priority = nla_get_u32(attr);
break;
--
2.11.0
^ permalink raw reply related
* [PATCH net 2/3] ipv6: Return error for RTA_VIA attribute
From: David Ahern @ 2019-02-26 17:00 UTC (permalink / raw)
To: davem; +Cc: netdev, David Ahern
In-Reply-To: <20190226170004.4535-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
IPv6 currently does not support nexthops outside of the AF_INET6 family.
Specifically, it does not handle RTA_VIA attribute. If it is passed
in a route add request, the actual route added only uses the device
which is clearly not what the user intended:
$ ip -6 ro add 2001:db8:2::/64 via inet 172.16.1.1 dev eth0
$ ip ro ls
...
2001:db8:2::/64 dev eth0 metric 1024 pref medium
Catch this and fail the route add:
$ ip -6 ro add 2001:db8:2::/64 via inet 172.16.1.1 dev eth0
Error: IPv6 does not support RTA_VIA attribute.
Fixes: 03c0566542f4c ("mpls: Netlink commands to add, remove, and dump routes")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv6/route.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ce15dc4ccbfa..b7a620023a52 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4182,6 +4182,10 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
cfg->fc_gateway = nla_get_in6_addr(tb[RTA_GATEWAY]);
cfg->fc_flags |= RTF_GATEWAY;
}
+ if (tb[RTA_VIA]) {
+ NL_SET_ERR_MSG(extack, "IPv6 does not support RTA_VIA attribute");
+ goto errout;
+ }
if (tb[RTA_DST]) {
int plen = (rtm->rtm_dst_len + 7) >> 3;
--
2.11.0
^ permalink raw reply related
* [PATCH net 3/3] mpls: Return error for RTA_GATEWAY attribute
From: David Ahern @ 2019-02-26 17:00 UTC (permalink / raw)
To: davem; +Cc: netdev, David Ahern
In-Reply-To: <20190226170004.4535-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
MPLS does not support nexthops with an MPLS address family.
Specifically, it does not handle RTA_GATEWAY attribute. Make it
clear by returning an error.
Fixes: 03c0566542f4c ("mpls: Netlink commands to add, remove, and dump routes")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/mpls/af_mpls.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7d55d4c04088..fa763e2e50ec 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1838,6 +1838,9 @@ static int rtm_to_route_config(struct sk_buff *skb,
goto errout;
break;
}
+ case RTA_GATEWAY:
+ NL_SET_ERR_MSG(extack, "MPLS does not support RTA_GATEWAY attribute");
+ goto errout;
case RTA_VIA:
{
if (nla_get_via(nla, &cfg->rc_via_alen,
--
2.11.0
^ permalink raw reply related
* [PATCH net 0/3] net: Fail route add with unsupported nexthop attribute
From: David Ahern @ 2019-02-26 17:00 UTC (permalink / raw)
To: davem; +Cc: netdev, David Ahern
From: David Ahern <dsahern@gmail.com>
RTA_VIA was added for MPLS as a way of specifying a gateway from a
different address family. IPv4 and IPv6 do not currently support RTA_VIA
so using it leads to routes that are not what the user intended. Catch
and fail - returning a proper error message.
MPLS on the other hand does not support RTA_GATEWAY since it does not
make sense to have a nexthop from the MPLS address family. Similarly,
catch and fail - returning a proper error message.
David Ahern (3):
ipv4: Return error for RTA_VIA attribute
ipv6: Return error for RTA_VIA attribute
mpls: Return error for RTA_GATEWAY attribute
net/ipv4/fib_frontend.c | 4 ++++
net/ipv6/route.c | 4 ++++
net/mpls/af_mpls.c | 3 +++
3 files changed, 11 insertions(+)
--
2.11.0
^ permalink raw reply
* Re: [PATCH net-next 0/2] vxlan: create and changelink extack support
From: David Miller @ 2019-02-26 17:01 UTC (permalink / raw)
To: roopa; +Cc: netdev, dsa, sd, johannes
In-Reply-To: <1551160982-32685-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Mon, 25 Feb 2019 22:03:00 -0800
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> This series adds extack support to changelink paths.
> In the process re-factors flag sets to a separate helper.
> Also adds some changelink testcases to rtnetlink.sh
>
> (This series was initially part of another series that
> tried to support changelink for more attributes.
> But after some feedback from sabrina, i have dropped the
> 'support changelink for more attributes' part because some
> of them cannot be supported today or may require additional
> use-case handling code. These can be done separately
> as and when we see the need for it.)
Series applied, thanks Roopa.
^ permalink raw reply
* Re: [PATCH net-next] mlxsw: spectrum: remove set but not used variable 'autoneg_status'
From: David Miller @ 2019-02-26 17:07 UTC (permalink / raw)
To: yuehaibing; +Cc: jiri, idosch, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20190225020328.42510-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Mon, 25 Feb 2019 02:03:28 +0000
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/ethernet/mellanox/mlxsw/spectrum.c: In function 'mlxsw_sp_port_get_link_ksettings':
> drivers/net/ethernet/mellanox/mlxsw/spectrum.c:3062:5: warning:
> variable 'autoneg_status' set but not used [-Wunused-but-set-variable]
>
> It's not used since commit 475b33cb66c9 ("mlxsw: spectrum: Remove unsupported
> eth_proto_lp_advertise field in PTYS")
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied.
^ permalink raw reply
* Re: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed
From: David Miller @ 2019-02-26 17:09 UTC (permalink / raw)
To: Nicolas.Ferre
Cc: f.fainelli, pthombar, netdev, andrew, hkallweit1, linux-kernel,
rafalc, piotrs, jank, Claudiu.Beznea
In-Reply-To: <a0f7be10-ff9b-c11c-fde9-30876940d719@microchip.com>
From: <Nicolas.Ferre@microchip.com>
Date: Tue, 26 Feb 2019 09:23:04 +0000
> This patch series seem pretty intrusive, so I would like that you wait
> for my explicit ACK before applying even next versions of it.
Ok.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox