refcount_warn_saturate WARNING (was: Re: cls_flower: Fix the behavior using port ranges with hw-offload)

refcount_warn_saturate WARNING (was: Re: cls_flower: Fix the behavior using port ranges with hw-offload)

[Geert Uytterhoeven]

Hi Komachi-san,

On Sun, Dec 8, 2019 at 10:40 PM Linux Kernel Mailing List
<linux-kernel@vger.kernel.org> wrote:
> Commit:     8ffb055beae58574d3e77b4bf9d4d15eace1ca27
> Parent:     2f23cd42e19c22c24ff0e221089b7b6123b117c5
> Refname:    refs/heads/master
> Web:        https://git.kernel.org/torvalds/c/8ffb055beae58574d3e77b4bf9d4d15eace1ca27
> Author:     Yoshiki Komachi <komachi.yoshiki@gmail.com>
> AuthorDate: Tue Dec 3 19:40:12 2019 +0900
> Committer:  David S. Miller <davem@davemloft.net>
> CommitDate: Tue Dec 3 11:55:46 2019 -0800
>
>     cls_flower: Fix the behavior using port ranges with hw-offload
>
>     The recent commit 5c72299fba9d ("net: sched: cls_flower: Classify
>     packets using port ranges") had added filtering based on port ranges
>     to tc flower. However the commit missed necessary changes in hw-offload
>     code, so the feature gave rise to generating incorrect offloaded flow
>     keys in NIC.
>
>     One more detailed example is below:
>
>     $ tc qdisc add dev eth0 ingress
>     $ tc filter add dev eth0 ingress protocol ip flower ip_proto tcp \
>       dst_port 100-200 action drop
>
>     With the setup above, an exact match filter with dst_port == 0 will be
>     installed in NIC by hw-offload. IOW, the NIC will have a rule which is
>     equivalent to the following one.
>
>     $ tc qdisc add dev eth0 ingress
>     $ tc filter add dev eth0 ingress protocol ip flower ip_proto tcp \
>       dst_port 0 action drop
>
>     The behavior was caused by the flow dissector which extracts packet
>     data into the flow key in the tc flower. More specifically, regardless
>     of exact match or specified port ranges, fl_init_dissector() set the
>     FLOW_DISSECTOR_KEY_PORTS flag in struct flow_dissector to extract port
>     numbers from skb in skb_flow_dissect() called by fl_classify(). Note
>     that device drivers received the same struct flow_dissector object as
>     used in skb_flow_dissect(). Thus, offloaded drivers could not identify
>     which of these is used because the FLOW_DISSECTOR_KEY_PORTS flag was
>     set to struct flow_dissector in either case.
>
>     This patch adds the new FLOW_DISSECTOR_KEY_PORTS_RANGE flag and the new
>     tp_range field in struct fl_flow_key to recognize which filters are applied
>     to offloaded drivers. At this point, when filters based on port ranges
>     passed to drivers, drivers return the EOPNOTSUPP error because they do
>     not support the feature (the newly created FLOW_DISSECTOR_KEY_PORTS_RANGE
>     flag).
>
>     Fixes: 5c72299fba9d ("net: sched: cls_flower: Classify packets using port ranges")
>     Signed-off-by: Yoshiki Komachi <komachi.yoshiki@gmail.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>

I still see the below warning on m68k/ARAnyM during boot with v5.5-rc2
and next-20191219.
Reverting commit 8ffb055beae58574 ("cls_flower: Fix the behavior using
port ranges with hw-offload") fixes that.

As this is networking, perhaps this is seen on big-endian only?
Or !CONFIG_SMP?

Do you have a clue?
I'm especially worried as this commit is already being backported to stable.
Thanks!

EXT4-fs (sda1): re-mounted. Opts:
EXT4-fs (sda1): re-mounted. Opts: errors=remount-ro
ext3 filesystem being remounted at / supports timestamps until 2038 (0x7fffffff)
------------[ cut here ]------------
WARNING: CPU: 0 PID: 7 at lib/refcount.c:28 refcount_warn_saturate+0x54/0x100
refcount_t: underflow; use-after-free.
Modules linked in:
CPU: 0 PID: 7 Comm: ksoftirqd/0 Not tainted 5.5.0-rc2-next-20191219-atari #246
Stack from 00c31e88:
        00c31e88 00387ebc 00023d96 0039b5b0 0000001c 00000009 00a6d680 00023dda
        0039b5b0 0000001c 001a9658 00000009 00000000 00c31ed0 00000001 00000000
        04208040 0000000a 0039b5eb 00c31ef0 00c30000 001a9658 0039b5b0 0000001c
        00000009 0039b5eb 0027071c 00326d1c 00000003 00326cd8 00271840 00000001
        00326d1c 00000000 00a6d680 0024339c 00a6d680 00000000 00000200 000ab5e6
        00048632 00a6d680 0000000a 00400d78 003fbc00 002f88a6 00400d78 002f8b5e
Call Trace: [<00023d96>] __warn+0xb2/0xb4
 [<00023dda>] warn_slowpath_fmt+0x42/0x64
 [<001a9658>] refcount_warn_saturate+0x54/0x100
 [<001a9658>] refcount_warn_saturate+0x54/0x100
 [<0027071c>] refcount_sub_and_test.constprop.77+0x38/0x3e
 [<00271840>] ipv4_dst_destroy+0x24/0x42
 [<0024339c>] dst_destroy+0x40/0xae
 [<000ab5e6>] kfree+0x0/0x3e
 [<00048632>] rcu_process_callbacks+0x9a/0x9c
 [<002f88a6>] __do_softirq+0x146/0x182
 [<002f8b5e>] schedule+0x0/0xb4
 [<00035f3e>] kthread_parkme+0x0/0x10
 [<00035a86>] __init_completion+0x0/0x20
 [<0003836c>] smpboot_thread_fn+0x0/0x100
 [<000360a2>] kthread_should_stop+0x0/0x12
 [<00036096>] kthread_should_park+0x0/0xc
 [<00025c28>] run_ksoftirqd+0x12/0x20
 [<00038466>] smpboot_thread_fn+0xfa/0x100
 [<00024914>] do_exit+0x0/0x6d4
 [<0003f620>] complete+0x0/0x34
 [<0003665c>] kthread+0xb2/0xbc
 [<00035a86>] __init_completion+0x0/0x20
 [<000365aa>] kthread+0x0/0xbc
 [<00002ab8>] ret_from_kernel_thread+0xc/0x14
---[ end trace cddc6a39eb5bb237 ]---

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: refcount_warn_saturate WARNING (was: Re: cls_flower: Fix the behavior using port ranges with hw-offload)

[Cong Wang]

On Thu, Dec 19, 2019 at 2:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> I still see the below warning on m68k/ARAnyM during boot with v5.5-rc2
> and next-20191219.
> Reverting commit 8ffb055beae58574 ("cls_flower: Fix the behavior using
> port ranges with hw-offload") fixes that.
>
> As this is networking, perhaps this is seen on big-endian only?
> Or !CONFIG_SMP?
>
> Do you have a clue?
> I'm especially worried as this commit is already being backported to stable.
> Thanks!

I did a quick look at the offending commit, I can't even connect it to
any dst refcnt.

Do you have any more information? Like what happened before the
warning? Does your system use cls_flower filters at all? If so, please
share your tc configurations.

Thanks.

Re: refcount_warn_saturate WARNING (was: Re: cls_flower: Fix the behavior using port ranges with hw-offload)

[Geert Uytterhoeven]

Hi Cong,

On Thu, Dec 19, 2019 at 9:50 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Dec 19, 2019 at 2:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I still see the below warning on m68k/ARAnyM during boot with v5.5-rc2
> > and next-20191219.
> > Reverting commit 8ffb055beae58574 ("cls_flower: Fix the behavior using
> > port ranges with hw-offload") fixes that.
> >
> > As this is networking, perhaps this is seen on big-endian only?
> > Or !CONFIG_SMP?
> >
> > Do you have a clue?
> > I'm especially worried as this commit is already being backported to stable.
> > Thanks!
>
> I did a quick look at the offending commit, I can't even connect it to
> any dst refcnt.
>
> Do you have any more information? Like what happened before the
> warning? Does your system use cls_flower filters at all? If so, please
> share your tc configurations.

No, I don't use clf_flower filters.  This is just a normal old Debian boot,
where the root file system is being remounted, followed by the warning.

To me, it also looks very strange.  But it's 100% reproducible for me.
Git bisect pointed to this commit, and reverting it fixes the issue.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: refcount_warn_saturate WARNING (was: Re: cls_flower: Fix the behavior using port ranges with hw-offload)

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Thu, Dec 19, 2019 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Cong,
>
> On Thu, Dec 19, 2019 at 9:50 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Thu, Dec 19, 2019 at 2:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > I still see the below warning on m68k/ARAnyM during boot with v5.5-rc2
> > > and next-20191219.
> > > Reverting commit 8ffb055beae58574 ("cls_flower: Fix the behavior using
> > > port ranges with hw-offload") fixes that.
> > >
> > > As this is networking, perhaps this is seen on big-endian only?
> > > Or !CONFIG_SMP?
> > >
> > > Do you have a clue?
> > > I'm especially worried as this commit is already being backported to stable.
> > > Thanks!
> >
> > I did a quick look at the offending commit, I can't even connect it to
> > any dst refcnt.
> >
> > Do you have any more information? Like what happened before the
> > warning? Does your system use cls_flower filters at all? If so, please
> > share your tc configurations.
>
> No, I don't use clf_flower filters.  This is just a normal old Debian boot,
> where the root file system is being remounted, followed by the warning.
>
> To me, it also looks very strange.  But it's 100% reproducible for me.
> Git bisect pointed to this commit, and reverting it fixes the issue.

Hmm, does the attached patch have any luck to fix it?

Thanks.

[-- Attachment #2: flow_dissector.diff --]
[-- Type: application/octet-stream, Size: 927 bytes --]

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index d524a693e00f..0884f987acd3 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1766,6 +1766,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
 		.key_id = FLOW_DISSECTOR_KEY_PORTS,
 		.offset = offsetof(struct flow_keys, ports),
 	},
+	{
+		.key_id = FLOW_DISSECTOR_KEY_PORTS_RANGE,
+		.offset = offsetof(struct flow_keys, ports),
+	},
 	{
 		.key_id = FLOW_DISSECTOR_KEY_VLAN,
 		.offset = offsetof(struct flow_keys, vlan),
@@ -1801,6 +1805,10 @@ static const struct flow_dissector_key flow_keys_dissector_symmetric_keys[] = {
 		.key_id = FLOW_DISSECTOR_KEY_PORTS,
 		.offset = offsetof(struct flow_keys, ports),
 	},
+	{
+		.key_id = FLOW_DISSECTOR_KEY_PORTS_RANGE,
+		.offset = offsetof(struct flow_keys, ports),
+	},
 };
 
 static const struct flow_dissector_key flow_keys_basic_dissector_keys[] = {

Re: refcount_warn_saturate WARNING (was: Re: cls_flower: Fix the behavior using port ranges with hw-offload)

[Geert Uytterhoeven]

Hi Cong,

On Thu, Dec 19, 2019 at 10:52 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Dec 19, 2019 at 1:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 19, 2019 at 9:50 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > On Thu, Dec 19, 2019 at 2:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > I still see the below warning on m68k/ARAnyM during boot with v5.5-rc2
> > > > and next-20191219.
> > > > Reverting commit 8ffb055beae58574 ("cls_flower: Fix the behavior using
> > > > port ranges with hw-offload") fixes that.
> > > >
> > > > As this is networking, perhaps this is seen on big-endian only?
> > > > Or !CONFIG_SMP?
> > > >
> > > > Do you have a clue?
> > > > I'm especially worried as this commit is already being backported to stable.
> > > > Thanks!
> > >
> > > I did a quick look at the offending commit, I can't even connect it to
> > > any dst refcnt.
> > >
> > > Do you have any more information? Like what happened before the
> > > warning? Does your system use cls_flower filters at all? If so, please
> > > share your tc configurations.
> >
> > No, I don't use clf_flower filters.  This is just a normal old Debian boot,
> > where the root file system is being remounted, followed by the warning.
>
> > To me, it also looks very strange.  But it's 100% reproducible for me.
> > Git bisect pointed to this commit, and reverting it fixes the issue.
>
> Hmm, does the attached patch have any luck to fix it?

Thanks, but unfortunately it doesn't help.

More investigation revealed that the above commit caused some shifts in the
kernel binary, moving dst_default_metrics to a 2-byte aligned address.
This is incompatible with the use of DST_METRICS_FLAGS and
__DST_METRICS_PTR().

Fix sent: "[PATCH] net: dst: Force 4-byte alignment of dst_metrics"
https://lore.kernel.org/linux-m68k/20191220133140.5684-1-geert@linux-m68k.org/

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds