* [PATCH net 1/2] Revert "ipv6: fix double refcount of fib6_metrics"
From: Wei Wang @ 2018-09-18 20:44 UTC (permalink / raw)
To: David Miller, netdev
Cc: Eric Dumazet, David Ahern, Cong Wang, Sabrina Dubroca, Wei Wang
In-Reply-To: <20180918204500.106240-1-tracywwnj@gmail.com>
From: Wei Wang <weiwan@google.com>
This reverts commit e70a3aad44cc8b24986687ffc98c4a4f6ecf25ea.
This change causes use-after-free on dst->_metrics.
The crash trace looks like this:
[ 97.763269] BUG: KASAN: use-after-free in ip6_mtu+0x116/0x140
[ 97.769038] Read of size 4 at addr ffff881781d2cf84 by task svw_NetThreadEv/8801
[ 97.777954] CPU: 76 PID: 8801 Comm: svw_NetThreadEv Not tainted 4.15.0-smp-DEV #11
[ 97.777956] Hardware name: Default string Default string/Indus_QC_02, BIOS 5.46.4 03/29/2018
[ 97.777957] Call Trace:
[ 97.777971] [<ffffffff895709db>] dump_stack+0x4d/0x72
[ 97.777985] [<ffffffff881651df>] print_address_description+0x6f/0x260
[ 97.777997] [<ffffffff88165747>] kasan_report+0x257/0x370
[ 97.778001] [<ffffffff894488e6>] ? ip6_mtu+0x116/0x140
[ 97.778004] [<ffffffff881658b9>] __asan_report_load4_noabort+0x19/0x20
[ 97.778008] [<ffffffff894488e6>] ip6_mtu+0x116/0x140
[ 97.778013] [<ffffffff892bb91e>] tcp_current_mss+0x12e/0x280
[ 97.778016] [<ffffffff892bb7f0>] ? tcp_mtu_to_mss+0x2d0/0x2d0
[ 97.778022] [<ffffffff887b45b8>] ? depot_save_stack+0x138/0x4a0
[ 97.778037] [<ffffffff87c38985>] ? __mmdrop+0x145/0x1f0
[ 97.778040] [<ffffffff881643b1>] ? save_stack+0xb1/0xd0
[ 97.778046] [<ffffffff89264c82>] tcp_send_mss+0x22/0x220
[ 97.778059] [<ffffffff89273a49>] tcp_sendmsg_locked+0x4f9/0x39f0
[ 97.778062] [<ffffffff881642b4>] ? kasan_check_write+0x14/0x20
[ 97.778066] [<ffffffff89273550>] ? tcp_sendpage+0x60/0x60
[ 97.778070] [<ffffffff881cb359>] ? rw_copy_check_uvector+0x69/0x280
[ 97.778075] [<ffffffff8873c65f>] ? import_iovec+0x9f/0x430
[ 97.778078] [<ffffffff88164be7>] ? kasan_slab_free+0x87/0xc0
[ 97.778082] [<ffffffff8873c5c0>] ? memzero_page+0x140/0x140
[ 97.778085] [<ffffffff881642b4>] ? kasan_check_write+0x14/0x20
[ 97.778088] [<ffffffff89276f6c>] tcp_sendmsg+0x2c/0x50
[ 97.778092] [<ffffffff89276f6c>] ? tcp_sendmsg+0x2c/0x50
[ 97.778098] [<ffffffff89352d43>] inet_sendmsg+0x103/0x480
[ 97.778102] [<ffffffff89352c40>] ? inet_gso_segment+0x15b0/0x15b0
[ 97.778105] [<ffffffff890294da>] sock_sendmsg+0xba/0xf0
[ 97.778108] [<ffffffff8902ab6a>] ___sys_sendmsg+0x6ca/0x8e0
[ 97.778113] [<ffffffff87dccac1>] ? hrtimer_try_to_cancel+0x71/0x3b0
[ 97.778116] [<ffffffff8902a4a0>] ? copy_msghdr_from_user+0x3d0/0x3d0
[ 97.778119] [<ffffffff881646d1>] ? memset+0x31/0x40
[ 97.778123] [<ffffffff87a0cff5>] ? schedule_hrtimeout_range_clock+0x165/0x380
[ 97.778127] [<ffffffff87a0ce90>] ? hrtimer_nanosleep_restart+0x250/0x250
[ 97.778130] [<ffffffff87dcc700>] ? __hrtimer_init+0x180/0x180
[ 97.778133] [<ffffffff87dd1f82>] ? ktime_get_ts64+0x172/0x200
[ 97.778137] [<ffffffff8822b8ec>] ? __fget_light+0x8c/0x2f0
[ 97.778141] [<ffffffff8902d5c6>] __sys_sendmsg+0xe6/0x190
[ 97.778144] [<ffffffff8902d5c6>] ? __sys_sendmsg+0xe6/0x190
[ 97.778147] [<ffffffff8902d4e0>] ? SyS_shutdown+0x20/0x20
[ 97.778152] [<ffffffff87cd4370>] ? wake_up_q+0xe0/0xe0
[ 97.778155] [<ffffffff8902d670>] ? __sys_sendmsg+0x190/0x190
[ 97.778158] [<ffffffff8902d683>] SyS_sendmsg+0x13/0x20
[ 97.778162] [<ffffffff87a1600c>] do_syscall_64+0x2ac/0x430
[ 97.778166] [<ffffffff87c17515>] ? do_page_fault+0x35/0x3d0
[ 97.778171] [<ffffffff8960131f>] ? page_fault+0x2f/0x50
[ 97.778174] [<ffffffff89600071>] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[ 97.778177] RIP: 0033:0x7f83fa36000d
[ 97.778178] RSP: 002b:00007f83ef9229e0 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
[ 97.778180] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f83fa36000d
[ 97.778182] RDX: 0000000000004000 RSI: 00007f83ef922f00 RDI: 0000000000000036
[ 97.778183] RBP: 00007f83ef923040 R08: 00007f83ef9231f8 R09: 00007f83ef923168
[ 97.778184] R10: 0000000000000000 R11: 0000000000000293 R12: 00007f83f69c5b40
[ 97.778185] R13: 000000000000001c R14: 0000000000000001 R15: 0000000000004000
[ 97.779684] Allocated by task 5919:
[ 97.783185] save_stack+0x46/0xd0
[ 97.783187] kasan_kmalloc+0xad/0xe0
[ 97.783189] kmem_cache_alloc_trace+0xdf/0x580
[ 97.783190] ip6_convert_metrics.isra.79+0x7e/0x190
[ 97.783192] ip6_route_info_create+0x60a/0x2480
[ 97.783193] ip6_route_add+0x1d/0x80
[ 97.783195] inet6_rtm_newroute+0xdd/0xf0
[ 97.783198] rtnetlink_rcv_msg+0x641/0xb10
[ 97.783200] netlink_rcv_skb+0x27b/0x3e0
[ 97.783202] rtnetlink_rcv+0x15/0x20
[ 97.783203] netlink_unicast+0x4be/0x720
[ 97.783204] netlink_sendmsg+0x7bc/0xbf0
[ 97.783205] sock_sendmsg+0xba/0xf0
[ 97.783207] ___sys_sendmsg+0x6ca/0x8e0
[ 97.783208] __sys_sendmsg+0xe6/0x190
[ 97.783209] SyS_sendmsg+0x13/0x20
[ 97.783211] do_syscall_64+0x2ac/0x430
[ 97.783213] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[ 97.784709] Freed by task 0:
[ 97.785056] knetbase: Error: /proc/sys/net/core/txcs_enable does not exist
[ 97.794497] save_stack+0x46/0xd0
[ 97.794499] kasan_slab_free+0x71/0xc0
[ 97.794500] kfree+0x7c/0xf0
[ 97.794501] fib6_info_destroy_rcu+0x24f/0x310
[ 97.794504] rcu_process_callbacks+0x38b/0x1730
[ 97.794506] __do_softirq+0x1c8/0x5d0
Reported-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Wei Wang <weiwan@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv6/route.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 480a79f47c52..b5d3e6b294ab 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -976,6 +976,10 @@ static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from)
rt->rt6i_flags &= ~RTF_EXPIRES;
rcu_assign_pointer(rt->from, from);
dst_init_metrics(&rt->dst, from->fib6_metrics->metrics, true);
+ if (from->fib6_metrics != &dst_default_metrics) {
+ rt->dst._metrics |= DST_METRICS_REFCOUNTED;
+ refcount_inc(&from->fib6_metrics->refcnt);
+ }
}
/* Caller must already hold reference to @ort */
--
2.19.0.397.gdd90340f6a-goog
^ permalink raw reply related
* [PATCH net 2/2] ipv6: fix memory leak on dst->_metrics
From: Wei Wang @ 2018-09-18 20:45 UTC (permalink / raw)
To: David Miller, netdev
Cc: Eric Dumazet, David Ahern, Cong Wang, Sabrina Dubroca, Wei Wang
In-Reply-To: <20180918204500.106240-2-tracywwnj@gmail.com>
From: Wei Wang <weiwan@google.com>
When dst->_metrics and f6i->fib6_metrics share the same memory, both
take reference count on the dst_metrics structure. However, when dst is
destroyed, ip6_dst_destroy() only invokes dst_destroy_metrics_generic()
which does not take care of READONLY metrics and does not release refcnt.
This causes memory leak.
Similar to ipv4 logic, the fix is to properly release refcnt and free
the memory space pointed by dst->_metrics if refcnt becomes 0.
Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Wei Wang <weiwan@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv6/route.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b5d3e6b294ab..826b14de7dbb 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -364,11 +364,14 @@ EXPORT_SYMBOL(ip6_dst_alloc);
static void ip6_dst_destroy(struct dst_entry *dst)
{
+ struct dst_metrics *p = (struct dst_metrics *)DST_METRICS_PTR(dst);
struct rt6_info *rt = (struct rt6_info *)dst;
struct fib6_info *from;
struct inet6_dev *idev;
- dst_destroy_metrics_generic(dst);
+ if (p != &dst_default_metrics && refcount_dec_and_test(&p->refcnt))
+ kfree(p);
+
rt6_uncached_list_del(rt);
idev = rt->rt6i_idev;
--
2.19.0.397.gdd90340f6a-goog
^ permalink raw reply related
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 21:13 UTC (permalink / raw)
To: songliubraving
Cc: Alexei Starovoitov, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan, kernel-team
In-Reply-To: <0FD562CC-CDE9-43C8-9623-B42AC7A208C8@fb.com>
On Tue, Sep 18, 2018 at 1:37 PM Song Liu <songliubraving@fb.com> wrote:
>
> Looks like a patch like the following fixes the issue for ixgbe. But I
> cannot explain it yet.
>
> Does this ring a bell?
I dunno, it looks like the NIC is generating an interrupt while it should not,
and constantly sets NAPI_STATE_MISSED.
Or maybe we need to properly check napi_complete_done() return value.
diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
index d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedcdcb72dc1c401813
100644
--- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
+++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
@@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int budget)
ixgb_clean_rx_irq(adapter, &work_done, budget);
/* If budget not fully consumed, exit the polling mode */
- if (work_done < budget) {
- napi_complete_done(napi, work_done);
+ if (work_done < budget &&
+ napi_complete_done(napi, work_done)) {
if (!test_bit(__IXGB_DOWN, &adapter->flags))
ixgb_irq_enable(adapter);
}
^ permalink raw reply
* Re: [PATCH 0/2] NFC fixes for 4.19-final
From: David Miller @ 2018-09-19 2:54 UTC (permalink / raw)
To: gregkh
Cc: sameo, netdev, allen.pais, keescook, surenb, linux-wireless,
linux-kernel
In-Reply-To: <20180917135141.28328-1-gregkh@linuxfoundation.org>
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Mon, 17 Sep 2018 15:51:39 +0200
> For some reason, these two NFC patches fell through the cracks. It
> looks like sometimes NFC patches go through the wireless tree, and
> other times, through the networking tree directly.
>
> I don't care which way they go, but they should get merged through some
> way, or, I can take them in my device tree, but that feels a bit odd to
> me.
I'll take these directly, thanks Greg.
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 21:21 UTC (permalink / raw)
To: Eric Dumazet, songliubraving
Cc: Alexei Starovoitov, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan, kernel-team
In-Reply-To: <CANn89i+kg=tE9aRYoFb+9ws35CCJJQz9Yc9i=9w=vDj=Epvj=A@mail.gmail.com>
On 09/18/2018 02:13 PM, Eric Dumazet wrote:
> On Tue, Sep 18, 2018 at 1:37 PM Song Liu <songliubraving@fb.com> wrote:
>>
>
>> Looks like a patch like the following fixes the issue for ixgbe. But I
>> cannot explain it yet.
>>
>> Does this ring a bell?
>
> I dunno, it looks like the NIC is generating an interrupt while it should not,
> and constantly sets NAPI_STATE_MISSED.
>
> Or maybe we need to properly check napi_complete_done() return value.
>
> diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> index d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedcdcb72dc1c401813
> 100644
> --- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> +++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> @@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int budget)
> ixgb_clean_rx_irq(adapter, &work_done, budget);
>
> /* If budget not fully consumed, exit the polling mode */
> - if (work_done < budget) {
> - napi_complete_done(napi, work_done);
> + if (work_done < budget &&
> + napi_complete_done(napi, work_done)) {
> if (!test_bit(__IXGB_DOWN, &adapter->flags))
> ixgb_irq_enable(adapter);
> }
>
ixgbe patch would be :
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 604282f03d236e4358fc91e64d8ba00a9b36cb8c..80d00aecb6e3e3e950ce6309bfe3639953dd73d9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3196,12 +3196,12 @@ int ixgbe_poll(struct napi_struct *napi, int budget)
return budget;
/* all work done, exit the polling mode */
- napi_complete_done(napi, work_done);
- if (adapter->rx_itr_setting & 1)
- ixgbe_set_itr(q_vector);
- if (!test_bit(__IXGBE_DOWN, &adapter->state))
- ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector->v_idx));
-
+ if (napi_complete_done(napi, work_done)) {
+ if (adapter->rx_itr_setting & 1)
+ ixgbe_set_itr(q_vector);
+ if (!test_bit(__IXGBE_DOWN, &adapter->state))
+ ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector->v_idx));
+ }
return min(work_done, budget - 1);
}
^ permalink raw reply related
* Re: pegged softirq and NAPI race (?)
From: Song Liu @ 2018-09-18 21:21 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexei Starovoitov, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan@broadcom.com, Kernel Team
In-Reply-To: <CANn89i+kg=tE9aRYoFb+9ws35CCJJQz9Yc9i=9w=vDj=Epvj=A@mail.gmail.com>
> On Sep 18, 2018, at 2:13 PM, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 18, 2018 at 1:37 PM Song Liu <songliubraving@fb.com> wrote:
>>
>
>> Looks like a patch like the following fixes the issue for ixgbe. But I
>> cannot explain it yet.
>>
>> Does this ring a bell?
>
> I dunno, it looks like the NIC is generating an interrupt while it should not,
> and constantly sets NAPI_STATE_MISSED.
>
> Or maybe we need to properly check napi_complete_done() return value.
>
> diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> index d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedcdcb72dc1c401813
> 100644
> --- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> +++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> @@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int budget)
> ixgb_clean_rx_irq(adapter, &work_done, budget);
>
> /* If budget not fully consumed, exit the polling mode */
> - if (work_done < budget) {
> - napi_complete_done(napi, work_done);
> + if (work_done < budget &&
> + napi_complete_done(napi, work_done)) {
> if (!test_bit(__IXGB_DOWN, &adapter->flags))
> ixgb_irq_enable(adapter);
> }
Thanks Eric!
I was looking at exactly this part. :) And it seems working!
I will run a bigger test and update shortly.
Best,
Song
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 21:25 UTC (permalink / raw)
To: songliubraving
Cc: Alexei Starovoitov, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan, kernel-team
In-Reply-To: <117A22DB-A4CF-4FFB-AD08-A187E9F6EEB3@fb.com>
On Tue, Sep 18, 2018 at 2:22 PM Song Liu <songliubraving@fb.com> wrote:
>
> Thanks Eric!
>
> I was looking at exactly this part. :) And it seems working!
And this should also make busy polling a bit faster as well, avoiding
enabling/receiving interrupts in the busy loop.
>
> I will run a bigger test and update shortly.
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Florian Fainelli @ 2018-09-18 21:25 UTC (permalink / raw)
To: Eric Dumazet, songliubraving
Cc: Alexei Starovoitov, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan, kernel-team
In-Reply-To: <CANn89i+kg=tE9aRYoFb+9ws35CCJJQz9Yc9i=9w=vDj=Epvj=A@mail.gmail.com>
On 09/18/2018 02:13 PM, Eric Dumazet wrote:
> On Tue, Sep 18, 2018 at 1:37 PM Song Liu <songliubraving@fb.com> wrote:
>>
>
>> Looks like a patch like the following fixes the issue for ixgbe. But I
>> cannot explain it yet.
>>
>> Does this ring a bell?
>
> I dunno, it looks like the NIC is generating an interrupt while it should not,
> and constantly sets NAPI_STATE_MISSED.
>
> Or maybe we need to properly check napi_complete_done() return value.
>
> diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> index d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedcdcb72dc1c401813
> 100644
> --- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> +++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> @@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int budget)
> ixgb_clean_rx_irq(adapter, &work_done, budget);
>
> /* If budget not fully consumed, exit the polling mode */
> - if (work_done < budget) {
> - napi_complete_done(napi, work_done);
> + if (work_done < budget &&
> + napi_complete_done(napi, work_done)) {
> if (!test_bit(__IXGB_DOWN, &adapter->flags))
> ixgb_irq_enable(adapter);
> }
This would not be the only driver doing this unfortunately... should we
add a __must_check annotation to help catch those (mis)uses? Though that
could cause false positives for drivers using NAPI to clean their TX ring.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] net: hns3: fix return type of ndo_start_xmit function
From: David Miller @ 2018-09-19 3:01 UTC (permalink / raw)
To: yuehaibing; +Cc: yisen.zhuang, salil.mehta, linux-kernel, netdev
In-Reply-To: <20180918060943.25208-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Tue, 18 Sep 2018 14:09:43 +0800
> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
> which is a typedef for an enum type, also the implementation in this
> driver has returns 'netdev_tx_t' value, so just change the function
> return type to netdev_tx_t.
>
> Found by coccinelle.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] net: cavium: fix return type of ndo_start_xmit function
From: David Miller @ 2018-09-19 3:03 UTC (permalink / raw)
To: yuehaibing
Cc: derek.chickles, satananda.burla, felix.manlunas, raghu.vatsavayi,
alexander.sverdlin, david.daney, linux-kernel, netdev
In-Reply-To: <20180918061905.25000-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Tue, 18 Sep 2018 14:19:05 +0800
> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
> which is a typedef for an enum type, so make sure the implementation in
> this driver has returns 'netdev_tx_t' value, and change the function
> return type to netdev_tx_t.
>
> Found by coccinelle.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied.
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 21:28 UTC (permalink / raw)
To: Florian Fainelli
Cc: songliubraving, Alexei Starovoitov, netdev, Jeff Kirsher,
Alexander Duyck, michael.chan, kernel-team
In-Reply-To: <09e7e3f8-dee8-71ea-7e57-4d0c92dcf13b@gmail.com>
On Tue, Sep 18, 2018 at 2:25 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
> This would not be the only driver doing this unfortunately... should we
> add a __must_check annotation to help catch those (mis)uses? Though that
> could cause false positives for drivers using NAPI to clean their TX ring.
>
Well, before adding __must_check we would need to cook a (big) patch
series to change all occurrences.
Not clear why netpoll is the trigger ?
^ permalink raw reply
* Re: [PATCH net-next] net: ibm: fix return type of ndo_start_xmit function
From: David Miller @ 2018-09-19 3:03 UTC (permalink / raw)
To: yuehaibing
Cc: dougmill, benh, paulus, mpe, tlfalcon, jallen, ivan, chunkeey,
keescook, linux-kernel, netdev, linuxppc-dev
In-Reply-To: <20180918063547.25644-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Tue, 18 Sep 2018 14:35:47 +0800
> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
> which is a typedef for an enum type, so make sure the implementation in
> this driver has returns 'netdev_tx_t' value, and change the function
> return type to netdev_tx_t.
>
> Found by coccinelle.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied.
^ permalink raw reply
* Re: [PATCHv3] net: bnxt: Fix a uninitialized variable warning.
From: David Miller @ 2018-09-19 3:05 UTC (permalink / raw)
To: zhongjiang; +Cc: michael.chan, vasundhara-v.volam, netdev, linux-kernel
In-Reply-To: <1537254944-41068-1-git-send-email-zhongjiang@huawei.com>
From: zhong jiang <zhongjiang@huawei.com>
Date: Tue, 18 Sep 2018 15:15:44 +0800
> Fix the following compile warning:
>
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c:49:5: warning: ‘nvm_param.dir_type’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> if (nvm_param.dir_type == BNXT_NVM_PORT_CFG)
>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2 0/2] net: ethernet: neterion: use linux/io-64-nonatomic-lo-hi.h
From: David Miller @ 2018-09-19 3:06 UTC (permalink / raw)
To: clabbe; +Cc: jdmason, linux-kernel, netdev
In-Reply-To: <1537255994-39517-1-git-send-email-clabbe@baylibre.com>
From: Corentin Labbe <clabbe@baylibre.com>
Date: Tue, 18 Sep 2018 07:33:12 +0000
> This serie remove usage of custom writeq/readq in favor of ones
> defined in linux/io-64-nonatomic-lo-hi.h
>
> This serie is only compile tested.
Series applied.
^ permalink raw reply
* Re: [PATCH bpf-next] flow_dissector: fix build failure without CONFIG_NET
From: Randy Dunlap @ 2018-09-18 21:33 UTC (permalink / raw)
To: Willem de Bruijn, netdev; +Cc: ast, daniel, Willem de Bruijn
In-Reply-To: <20180918202018.204099-1-willemdebruijn.kernel@gmail.com>
On 9/18/18 1:20 PM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> If boolean CONFIG_BPF_SYSCALL is enabled, kernel/bpf/syscall.c will
> call flow_dissector functions from net/core/flow_dissector.c.
>
> This causes this build failure if CONFIG_NET is disabled:
>
> kernel/bpf/syscall.o: In function `__x64_sys_bpf':
> syscall.c:(.text+0x3278): undefined reference to
> `skb_flow_dissector_bpf_prog_attach'
> syscall.c:(.text+0x3310): undefined reference to
> `skb_flow_dissector_bpf_prog_detach'
> kernel/bpf/syscall.o:(.rodata+0x3f0): undefined reference to
> `flow_dissector_prog_ops'
> kernel/bpf/verifier.o:(.rodata+0x250): undefined reference to
> `flow_dissector_verifier_ops'
>
> Analogous to other optional BPF program types in syscall.c, add stubs
> if the relevant functions are not compiled and move the BPF_PROG_TYPE
> definition in the #ifdef CONFIG_NET block.
>
> Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Works for me. Thanks.
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
> ---
> include/linux/bpf_types.h | 2 +-
> include/linux/skbuff.h | 13 +++++++++++++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index 22083712dd18..c9bd6fb765b0 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -16,6 +16,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_SEG6LOCAL, lwt_seg6local)
> BPF_PROG_TYPE(BPF_PROG_TYPE_SOCK_OPS, sock_ops)
> BPF_PROG_TYPE(BPF_PROG_TYPE_SK_SKB, sk_skb)
> BPF_PROG_TYPE(BPF_PROG_TYPE_SK_MSG, sk_msg)
> +BPF_PROG_TYPE(BPF_PROG_TYPE_FLOW_DISSECTOR, flow_dissector)
> #endif
> #ifdef CONFIG_BPF_EVENTS
> BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe)
> @@ -32,7 +33,6 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LIRC_MODE2, lirc_mode2)
> #ifdef CONFIG_INET
> BPF_PROG_TYPE(BPF_PROG_TYPE_SK_REUSEPORT, sk_reuseport)
> #endif
> -BPF_PROG_TYPE(BPF_PROG_TYPE_FLOW_DISSECTOR, flow_dissector)
>
> BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
> BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops)
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index ce0e863f02a2..76be85ea392a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1194,10 +1194,23 @@ void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
> const struct flow_dissector_key *key,
> unsigned int key_count);
>
> +#ifdef CONFIG_NET
> int skb_flow_dissector_bpf_prog_attach(const union bpf_attr *attr,
> struct bpf_prog *prog);
>
> int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr);
> +#else
> +static inline int skb_flow_dissector_bpf_prog_attach(const union bpf_attr *attr,
> + struct bpf_prog *prog)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static inline int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr)
> +{
> + return -EOPNOTSUPP;
> +}
> +#endif
>
> bool __skb_flow_dissect(const struct sk_buff *skb,
> struct flow_dissector *flow_dissector,
>
--
~Randy
^ permalink raw reply
* Re: [PATCH] net: ethernet: remove redundant include
From: David Miller @ 2018-09-19 3:08 UTC (permalink / raw)
To: zhongjiang; +Cc: netanel, jcliburn, chris.snook, santosh, netdev, linux-kernel
In-Reply-To: <1537258213-43522-1-git-send-email-zhongjiang@huawei.com>
From: zhong jiang <zhongjiang@huawei.com>
Date: Tue, 18 Sep 2018 16:10:13 +0800
> module.h already contained moduleparam.h, so it is safe to remove
> the redundant include.
>
> The issue is detected with the help of Coccinelle.
>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
Applied.
^ permalink raw reply
* Re: [PATCH] net: wan: remove redundant include
From: David Miller @ 2018-09-19 3:08 UTC (permalink / raw)
To: zhongjiang; +Cc: khc, netdev, linux-kernel
In-Reply-To: <1537258775-44025-1-git-send-email-zhongjiang@huawei.com>
From: zhong jiang <zhongjiang@huawei.com>
Date: Tue, 18 Sep 2018 16:19:35 +0800
> module.h already contained moduleparam.h, so it is safe to remove
> the redundant include.
>
> The issue is detected with the help of Coccinelle.
>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
Applied.
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Florian Fainelli @ 2018-09-18 21:35 UTC (permalink / raw)
To: Eric Dumazet
Cc: songliubraving, Alexei Starovoitov, netdev, Jeff Kirsher,
Alexander Duyck, michael.chan, kernel-team
In-Reply-To: <CANn89iLzFhXS77T=Hu6SSrL5NR+VqvDwX3AXDxhSy15aCUEWMw@mail.gmail.com>
On 09/18/2018 02:28 PM, Eric Dumazet wrote:
> On Tue, Sep 18, 2018 at 2:25 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>> This would not be the only driver doing this unfortunately... should we
>> add a __must_check annotation to help catch those (mis)uses? Though that
>> could cause false positives for drivers using NAPI to clean their TX ring.
>>
>
> Well, before adding __must_check we would need to cook a (big) patch
> series to change all occurrences.
>
Sounds good, I don't mind submitting something unless you beat me to it.
>
> Not clear why netpoll is the trigger ?
>
Me either, this should be observable during normal operation as well,
though most likely you just get a spurious interrupt and nothing to
clean in the RX ring, so things just get unnoticed?
--
Florian
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Jeff Kirsher @ 2018-09-18 21:36 UTC (permalink / raw)
To: Eric Dumazet, Eric Dumazet, songliubraving
Cc: Alexei Starovoitov, netdev, Alexander Duyck, michael.chan,
kernel-team
In-Reply-To: <324c4a85-1749-9129-bf03-73a49d9a4059@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]
On Tue, 2018-09-18 at 14:21 -0700, Eric Dumazet wrote:
>
> On 09/18/2018 02:13 PM, Eric Dumazet wrote:
> > On Tue, Sep 18, 2018 at 1:37 PM Song Liu <songliubraving@fb.com>
> > wrote:
> > >
> > > Looks like a patch like the following fixes the issue for ixgbe.
> > > But I
> > > cannot explain it yet.
> > >
> > > Does this ring a bell?
> >
> > I dunno, it looks like the NIC is generating an interrupt while it
> > should not,
> > and constantly sets NAPI_STATE_MISSED.
> >
> > Or maybe we need to properly check napi_complete_done() return
> > value.
> >
> > diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> > b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> > index
> > d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedc
> > dcb72dc1c401813
> > 100644
> > --- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> > +++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> > @@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int
> > budget)
> > ixgb_clean_rx_irq(adapter, &work_done, budget);
> >
> > /* If budget not fully consumed, exit the polling mode */
> > - if (work_done < budget) {
> > - napi_complete_done(napi, work_done);
> > + if (work_done < budget &&
> > + napi_complete_done(napi, work_done)) {
> > if (!test_bit(__IXGB_DOWN, &adapter->flags))
> > ixgb_irq_enable(adapter);
> > }
> >
>
>
> ixgbe patch would be :
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index
> 604282f03d236e4358fc91e64d8ba00a9b36cb8c..80d00aecb6e3e3e950ce6309bfe
> 3639953dd73d9 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -3196,12 +3196,12 @@ int ixgbe_poll(struct napi_struct *napi, int
> budget)
> return budget;
>
> /* all work done, exit the polling mode */
> - napi_complete_done(napi, work_done);
> - if (adapter->rx_itr_setting & 1)
> - ixgbe_set_itr(q_vector);
> - if (!test_bit(__IXGBE_DOWN, &adapter->state))
> - ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector-
> >v_idx));
> -
> + if (napi_complete_done(napi, work_done)) {
> + if (adapter->rx_itr_setting & 1)
> + ixgbe_set_itr(q_vector);
> + if (!test_bit(__IXGBE_DOWN, &adapter->state))
> + ixgbe_irq_enable_queues(adapter,
> BIT_ULL(q_vector->v_idx));
> + }
> return min(work_done, budget - 1);
> }
>
Eric, after Song does some testing on these changes, will you be
submitting a formal patch? If so, make sure to send it to
intel-wired-lan@lists.osuosl.org mailing list so I can pick up the fix.
By the way, thanks!
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Song Liu @ 2018-09-18 21:36 UTC (permalink / raw)
To: Eric Dumazet
Cc: Florian Fainelli, Alexei Starovoitov, netdev, Jeff Kirsher,
Alexander Duyck, michael.chan@broadcom.com, Kernel Team
In-Reply-To: <CANn89iLzFhXS77T=Hu6SSrL5NR+VqvDwX3AXDxhSy15aCUEWMw@mail.gmail.com>
> On Sep 18, 2018, at 2:28 PM, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 18, 2018 at 2:25 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>> This would not be the only driver doing this unfortunately... should we
>> add a __must_check annotation to help catch those (mis)uses? Though that
>> could cause false positives for drivers using NAPI to clean their TX ring.
>>
>
> Well, before adding __must_check we would need to cook a (big) patch
> series to change all occurrences.
>
>
> Not clear why netpoll is the trigger ?
>From my observation, the trigger path is:
netpoll_poll_dev() => ndo_poll_controller() => napi_schedule_prep().
I guess it is a race between IRQ/netpoll=>napi_schedule_prep() and
not handled napi_complete_done(). netpoll just makes it triggers more
often?
Unfortunately for me, bnxt already checked napi_complete_done(), so
there should be another bug with bnxt (at least with our 4.11 based
kernel).
Song
^ permalink raw reply
* Re: [PATCH net 0/5] net/smc: fixes 2018-09-18
From: David Miller @ 2018-09-19 3:12 UTC (permalink / raw)
To: ubraun; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl,
linux-kernel
In-Reply-To: <20180918134638.90271-1-ubraun@linux.ibm.com>
From: Ursula Braun <ubraun@linux.ibm.com>
Date: Tue, 18 Sep 2018 15:46:33 +0200
> here are some fixes in different areas of the smc code for the net
> tree.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net] net: mvpp2: fix a txq_done race condition
From: David Miller @ 2018-09-19 3:14 UTC (permalink / raw)
To: antoine.tenart
Cc: netdev, linux-kernel, thomas.petazzoni, maxime.chevallier,
gregory.clement, miquel.raynal, nadavh, stefanc, ymarkman, mw
In-Reply-To: <20180918145847.11318-1-antoine.tenart@bootlin.com>
From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Tue, 18 Sep 2018 16:58:47 +0200
> When no Tx IRQ is available, the txq_done() routine (called from
> tx_done()) shouldn't be called from the polling function, as in such
> case it is already called in the Tx path thanks to an hrtimer. This
> mostly occurred when using PPv2.1, as the engine then do not have Tx
> IRQs.
>
> Fixes: edc660fa09e2 ("net: mvpp2: replace TX coalescing interrupts with hrtimer")
> Reported-by: Stefan Chulski <stefanc@marvell.com>
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Song Liu @ 2018-09-18 21:40 UTC (permalink / raw)
To: jeffrey.t.kirsher@intel.com
Cc: Eric Dumazet, Eric Dumazet, Alexei Starovoitov, netdev,
Alexander Duyck, michael.chan@broadcom.com, Kernel Team
In-Reply-To: <e3eb9c891f97adbcfbb8df7129696f45f4f78368.camel@intel.com>
> On Sep 18, 2018, at 2:36 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>
> On Tue, 2018-09-18 at 14:21 -0700, Eric Dumazet wrote:
>>
>> On 09/18/2018 02:13 PM, Eric Dumazet wrote:
>>> On Tue, Sep 18, 2018 at 1:37 PM Song Liu <songliubraving@fb.com>
>>> wrote:
>>>>
>>>> Looks like a patch like the following fixes the issue for ixgbe.
>>>> But I
>>>> cannot explain it yet.
>>>>
>>>> Does this ring a bell?
>>>
>>> I dunno, it looks like the NIC is generating an interrupt while it
>>> should not,
>>> and constantly sets NAPI_STATE_MISSED.
>>>
>>> Or maybe we need to properly check napi_complete_done() return
>>> value.
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> index
>>> d3e72d0f66ef428b08e4bd88508e05b734bc43a4..c4c565c982a98a5891603cedc
>>> dcb72dc1c401813
>>> 100644
>>> --- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
>>> @@ -1773,8 +1773,8 @@ ixgb_clean(struct napi_struct *napi, int
>>> budget)
>>> ixgb_clean_rx_irq(adapter, &work_done, budget);
>>>
>>> /* If budget not fully consumed, exit the polling mode */
>>> - if (work_done < budget) {
>>> - napi_complete_done(napi, work_done);
>>> + if (work_done < budget &&
>>> + napi_complete_done(napi, work_done)) {
>>> if (!test_bit(__IXGB_DOWN, &adapter->flags))
>>> ixgb_irq_enable(adapter);
>>> }
>>>
>>
>>
>> ixgbe patch would be :
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index
>> 604282f03d236e4358fc91e64d8ba00a9b36cb8c..80d00aecb6e3e3e950ce6309bfe
>> 3639953dd73d9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3196,12 +3196,12 @@ int ixgbe_poll(struct napi_struct *napi, int
>> budget)
>> return budget;
>>
>> /* all work done, exit the polling mode */
>> - napi_complete_done(napi, work_done);
>> - if (adapter->rx_itr_setting & 1)
>> - ixgbe_set_itr(q_vector);
>> - if (!test_bit(__IXGBE_DOWN, &adapter->state))
>> - ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector-
>>> v_idx));
>> -
>> + if (napi_complete_done(napi, work_done)) {
Shall we add likely() here?
>> + if (adapter->rx_itr_setting & 1)
>> + ixgbe_set_itr(q_vector);
>> + if (!test_bit(__IXGBE_DOWN, &adapter->state))
>> + ixgbe_irq_enable_queues(adapter,
>> BIT_ULL(q_vector->v_idx));
>> + }
>> return min(work_done, budget - 1);
>> }
>>
>
> Eric, after Song does some testing on these changes, will you be
> submitting a formal patch? If so, make sure to send it to
> intel-wired-lan@lists.osuosl.org mailing list so I can pick up the fix.
>
> By the way, thanks!
I would submit the patch if Eric prefer not to. :)
Thanks,
Song
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 21:46 UTC (permalink / raw)
To: songliubraving
Cc: Jeff Kirsher, Eric Dumazet, Alexei Starovoitov, netdev,
Alexander Duyck, michael.chan, kernel-team
In-Reply-To: <ADCEA4AC-0071-47B7-A539-5F219EF09206@fb.com>
On Tue, Sep 18, 2018 at 2:41 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> >
> I would submit the patch if Eric prefer not to. :)
Hmmm.... maybe the bug is really in ixgbe_netpoll()
This whole ndo_poll_controller() stuff is crazy.
All sane implementations should only call napi_schedule()
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Song Liu @ 2018-09-18 21:55 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jeff Kirsher, Eric Dumazet, Alexei Starovoitov, netdev,
Alexander Duyck, michael.chan@broadcom.com, Kernel Team
In-Reply-To: <CANn89iLFBJjFgFHZUZb9xXXuN39SwB8Fn3AkgSon8M1w_X_fwQ@mail.gmail.com>
> On Sep 18, 2018, at 2:46 PM, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 18, 2018 at 2:41 PM Song Liu <songliubraving@fb.com> wrote:
>>
>>
>>
>>>
>> I would submit the patch if Eric prefer not to. :)
>
>
> Hmmm.... maybe the bug is really in ixgbe_netpoll()
>
> This whole ndo_poll_controller() stuff is crazy.
>
> All sane implementations should only call napi_schedule()
Current implement is about identical to napi_schedule_irqoff(). Do
we really need napi_schedule() instead?
On the other hand, I think we should check napi_complete_done() in
ixgbe_poll() anyway.
Song
^ 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