From: Cong Wang <hidden> Date: 2017-09-20 18:23:12
On Wed, Sep 20, 2017 at 10:55 AM, Paweł Staszewski
[off-list ref] wrote:
W dniu 2017-09-20 o 19:50, Cong Wang pisze:
On Wed, Sep 20, 2017 at 6:11 AM, Eric Dumazet [off-list ref]
wrote:
Sorry for top-posting, but this is to give context to Wei, since Pawel
used a top posting way to report his bisection.
Wei, can you take a look at Pawel report ?
Crash happens in dst_destroy() at following :
if (dst->dev)
dev_put(dst->dev); <<CRASH>>
dst->dev is not NULL, but netdev->pcpu_refcnt is NULL
65 ff 08 decl %gs:(%rax) // CRASH since rax = NULL
Pawel, please share your netdevices and routing setup ?
Looks like a double dev_put() on some dev...
Pawel, do you have any idea how this is triggered? Does your
test try to remove some network device? If so which one?
I noticed you have at least multiple vlan, bond and ixgbe
devices.
Just after i start bgp sessions
So when host is starting i have all bgp sessions to upstreams shutdown
To trigger panic i just enable all 6x bgp sessions at once to upstreams -
and zebra is start to pull prefixes and push them to the kernel
Then some traffic is generated from test hosts thru this backup router and
panic is generated - every time after 10 to 15 seconds after bgp sessions
are connected.
I'm not removing any interface at this time or do anything with interfaces -
just wait.
And yes there are vlans attached to the bond devices
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
From: Eric Dumazet <hidden> Date: 2017-09-20 18:30:33
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other kernel
user.
From: Cong Wang <hidden> Date: 2017-09-20 18:36:39
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet [off-list ref] wrote:
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other kernel
user.
Sure, but only unregister could trigger a free. If there is no unregister,
like what Pawel claims, then there is no free, the refcnt just goes to
0 but the memory is still there.
From: Paweł Staszewski <hidden> Date: 2017-09-20 19:15:13
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet [off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other kernel
user.
Sure, but only unregister could trigger a free. If there is no unregister,
like what Pawel claims, then there is no free, the refcnt just goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too early
that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch 'next'
of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid using
stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp: remove
cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce a new
function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call dst_dev_put() properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark DST_NOGC
and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4:
mark DST_NOGC and remove the operation of dst_free()
From: Paweł Staszewski <hidden> Date: 2017-09-20 19:25:57
W dniu 2017-09-20 o 21:13, Paweł Staszewski pisze:
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
quoted
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet
[off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other kernel
user.
Sure, but only unregister could trigger a free. If there is no
unregister,
like what Pawel claims, then there is no free, the refcnt just goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too
early that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch 'next'
of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid
using stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp: remove
cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce a
new function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call dst_dev_put()
properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark DST_NOGC
and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4:
mark DST_NOGC and remove the operation of dst_free()
What i can say more
I can reproduce this on any server with similar configuration
the difference can be teamd instead of bonding
ixgbe or i40e and mlx5
Same problems
vlans - more or less prefixes learned from bgp -> zebra -> netlink -> kernel
But normally in lab when using only plain routing no bgpd and about 128
vlans - with 128 routes - cant reproduce this - this apperas only with
bgp - minimum where i can reproduce this was about 130k prefixes with
about 286 nexthops
From: Paweł Staszewski <hidden> Date: 2017-09-20 21:12:56
W dniu 2017-09-20 o 21:23, Paweł Staszewski pisze:
W dniu 2017-09-20 o 21:13, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
quoted
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet
[off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other
kernel
user.
Sure, but only unregister could trigger a free. If there is no
unregister,
like what Pawel claims, then there is no free, the refcnt just goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too
early that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch
'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid
using stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp: remove
cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce a
new function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call dst_dev_put()
properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark DST_NOGC
and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4:
mark DST_NOGC and remove the operation of dst_free()
What i can say more
I can reproduce this on any server with similar configuration
the difference can be teamd instead of bonding
ixgbe or i40e and mlx5
Same problems
vlans - more or less prefixes learned from bgp -> zebra -> netlink ->
kernel
But normally in lab when using only plain routing no bgpd and about
128 vlans - with 128 routes - cant reproduce this - this apperas only
with bgp - minimum where i can reproduce this was about 130k prefixes
with about 286 nexthops
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid of the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
From: Paweł Staszewski <hidden> Date: 2017-09-20 21:26:14
W dniu 2017-09-20 o 23:10, Paweł Staszewski pisze:
W dniu 2017-09-20 o 21:23, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:13, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
quoted
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet
[off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other
kernel
user.
Sure, but only unregister could trigger a free. If there is no
unregister,
like what Pawel claims, then there is no free, the refcnt just goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too
early that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch
'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid
using stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp: remove
cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce a
new function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call dst_dev_put()
properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark
DST_NOGC and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4:
mark DST_NOGC and remove the operation of dst_free()
What i can say more
I can reproduce this on any server with similar configuration
the difference can be teamd instead of bonding
ixgbe or i40e and mlx5
Same problems
vlans - more or less prefixes learned from bgp -> zebra -> netlink ->
kernel
But normally in lab when using only plain routing no bgpd and about
128 vlans - with 128 routes - cant reproduce this - this apperas only
with bgp - minimum where i can reproduce this was about 130k prefixes
with about 286 nexthops
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid of the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the
calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
From: Paweł Staszewski <hidden> Date: 2017-09-20 21:27:53
W dniu 2017-09-20 o 23:24, Paweł Staszewski pisze:
W dniu 2017-09-20 o 23:10, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:23, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:13, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
quoted
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet
[off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other
kernel
user.
Sure, but only unregister could trigger a free. If there is no
unregister,
like what Pawel claims, then there is no free, the refcnt just
goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too
early that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch
'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid
using stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp: remove
cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce a
new function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call dst_dev_put()
properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark
DST_NOGC and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911]
ipv4: mark DST_NOGC and remove the operation of dst_free()
What i can say more
I can reproduce this on any server with similar configuration
the difference can be teamd instead of bonding
ixgbe or i40e and mlx5
Same problems
vlans - more or less prefixes learned from bgp -> zebra -> netlink
-> kernel
But normally in lab when using only plain routing no bgpd and about
128 vlans - with 128 routes - cant reproduce this - this apperas
only with bgp - minimum where i can reproduce this was about 130k
prefixes with about 286 nexthops
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid of
the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the
calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
From: Paweł Staszewski <hidden> Date: 2017-09-20 21:29:27
W dniu 2017-09-20 o 23:25, Paweł Staszewski pisze:
W dniu 2017-09-20 o 23:24, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 23:10, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:23, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:13, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
quoted
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet
[off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other
kernel
user.
Sure, but only unregister could trigger a free. If there is no
unregister,
like what Pawel claims, then there is no free, the refcnt just
goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too
early that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch
'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid
using stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp:
remove cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce
a new function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call
dst_dev_put() properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark
DST_NOGC and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911]
ipv4: mark DST_NOGC and remove the operation of dst_free()
What i can say more
I can reproduce this on any server with similar configuration
the difference can be teamd instead of bonding
ixgbe or i40e and mlx5
Same problems
vlans - more or less prefixes learned from bgp -> zebra -> netlink
-> kernel
But normally in lab when using only plain routing no bgpd and about
128 vlans - with 128 routes - cant reproduce this - this apperas
only with bgp - minimum where i can reproduce this was about 130k
prefixes with about 286 nexthops
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid
of the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the
calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
@@ -3331,7 +3331,15 @@ void netdev_run_todo(void);*/staticinlinevoiddev_put(structnet_device*dev){-this_cpu_dec(*dev->pcpu_refcnt);+int__percpu*pref=READ_ONCE(dev->pcpu_refcnt);++if(!pref){+pr_err("no pcpu_refcnt on dev %p(%s) state %d dismantle %d\n",+dev,dev->name,dev->reg_state,dev->dismantle);+for(;;)+cpu_relax();+}+this_cpu_dec(*pref);}/**
Have just halted console - no output no reaction on kbd
nothing in any syslog/log and catched only something from perf top
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid of the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the
calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
On Wed, Sep 20, 2017 at 2:25 PM, Paweł Staszewski [off-list ref] wrote:
W dniu 2017-09-20 o 23:24, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 23:10, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:23, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:13, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
quoted
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet
[off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other
kernel
user.
Sure, but only unregister could trigger a free. If there is no
unregister,
like what Pawel claims, then there is no free, the refcnt just goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too early
that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch 'next'
of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid using
stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp: remove
cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce a new
function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call dst_dev_put()
properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark DST_NOGC
and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4:
mark DST_NOGC and remove the operation of dst_free()
What i can say more
I can reproduce this on any server with similar configuration
the difference can be teamd instead of bonding
ixgbe or i40e and mlx5
Same problems
vlans - more or less prefixes learned from bgp -> zebra -> netlink ->
kernel
But normally in lab when using only plain routing no bgpd and about 128
vlans - with 128 routes - cant reproduce this - this apperas only with bgp -
minimum where i can reproduce this was about 130k prefixes with about 286
nexthops
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid of the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the
calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
Hi Pawel,
Could you try the following debug patch on top of net-next branch and
reproduce the issue check if there are warning msg showing?
*dst, unsigned long time)
static inline struct dst_entry *dst_clone(struct dst_entry *dst)
{
if (dst)
- atomic_inc(&dst->__refcnt);
+ dst_hold(dst);
return dst;
}
Thanks.
Wei
On Wed, Sep 20, 2017 at 3:09 PM, Wei Wang [off-list ref] wrote:
quoted
quoted
quoted
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid of the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the
calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
On Wed, Sep 20, 2017 at 2:25 PM, Paweł Staszewski [off-list ref] wrote:
quoted
W dniu 2017-09-20 o 23:24, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 23:10, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:23, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 21:13, Paweł Staszewski pisze:
quoted
W dniu 2017-09-20 o 20:36, Cong Wang pisze:
quoted
On Wed, Sep 20, 2017 at 11:30 AM, Eric Dumazet
[off-list ref] wrote:
quoted
On Wed, 2017-09-20 at 11:22 -0700, Cong Wang wrote:
quoted
but dmesg at this time shows nothing about interfaces or flaps.
This is very odd.
We only free netdevice in free_netdev() and it is only called when
we unregister a netdevice. Otherwise pcpu_refcnt is impossible
to be NULL.
If there is a missing dev_hold() or one dev_put() in excess,
this would allow the netdev to be freed too soon.
-> Use after free.
memory holding netdev could be reallocated-cleared by some other
kernel
user.
Sure, but only unregister could trigger a free. If there is no
unregister,
like what Pawel claims, then there is no free, the refcnt just goes to
0 but the memory is still there.
About possible mistake from my side with bisect - i can judge too early
that some bisect was good
the road was:
git bisect start
# bad: [ac7b75966c9c86426b55fe1c50ae148aa4571075] Merge tag
'pinctrl-v4.13-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad ac7b75966c9c86426b55fe1c50ae148aa4571075
# good: [e24dd9ee5399747b71c1d982a484fc7601795f31] Merge branch 'next'
of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security
git bisect good e24dd9ee5399747b71c1d982a484fc7601795f31
# bad: [9cc9a5cb176ccb4f2cda5ac34da5a659926f125f] datapath: Avoid using
stack larger than 1024.
git bisect bad 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
# good: [073cf9e20c333ab29744717a23f9e43ec7512a20] Merge branch
'udp-reduce-cache-pressure'
git bisect good 073cf9e20c333ab29744717a23f9e43ec7512a20
# bad: [8abd5599a520e9f188a750f1bde9dde5fb856230] Merge branch
's390-net-updates-part-2'
git bisect bad 8abd5599a520e9f188a750f1bde9dde5fb856230
# good: [2fae5d0e647c6470d206e72b5fc24972bb900f70] Merge branch
'bpf-ctx-narrow'
git bisect good 2fae5d0e647c6470d206e72b5fc24972bb900f70
# good: [41500c3e2a19ffcf40a7158fce1774de08e26ba2] rds: tcp: remove
cp_outgoing
git bisect good 41500c3e2a19ffcf40a7158fce1774de08e26ba2
# bad: [8917a777be3ba566377be05117f71b93a5fd909d] tcp: md5: add
TCP_MD5SIG_EXT socket option to set a key address prefix
git bisect bad 8917a777be3ba566377be05117f71b93a5fd909d
# good: [4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36] net: introduce a new
function dst_dev_put()
And currently have this running for about 4 hours without problems.
git bisect good 4a6ce2b6f2ecabbddcfe47e7cf61dd0f00b10e36
# bad: [a4c2fd7f78915a0d7c5275e7612e7793157a01f2] net: remove
DST_NOCACHE flag
Here for sure - panic
git bisect bad a4c2fd7f78915a0d7c5275e7612e7793157a01f2
# bad: [ad65a2f05695aced349e308193c6e2a6b1d87112] ipv6: call
dst_hold_safe() properly
git bisect bad ad65a2f05695aced349e308193c6e2a6b1d87112
# good: [9df16efadd2a8a82731dc76ff656c771e261827f] ipv4: call
dst_hold_safe() properly
git bisect good 9df16efadd2a8a82731dc76ff656c771e261827f
# bad: [1cfb71eeb12047bcdbd3e6730ffed66e810a0855] ipv6: take
dst->__refcnt for insertion into fib6 tree
im not 100% sure tor last two
Will test them again starting from
[95c47f9cf5e028d1ae77dc6c767c1edc8a18025b] ipv4: call dst_dev_put()
properly
git bisect bad 1cfb71eeb12047bcdbd3e6730ffed66e810a0855
# bad: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4: mark DST_NOGC
and remove the operation of dst_free()
git bisect bad b838d5e1c5b6e57b10ec8af2268824041e3ea911
# first bad commit: [b838d5e1c5b6e57b10ec8af2268824041e3ea911] ipv4:
mark DST_NOGC and remove the operation of dst_free()
What i can say more
I can reproduce this on any server with similar configuration
the difference can be teamd instead of bonding
ixgbe or i40e and mlx5
Same problems
vlans - more or less prefixes learned from bgp -> zebra -> netlink ->
kernel
But normally in lab when using only plain routing no bgpd and about 128
vlans - with 128 routes - cant reproduce this - this apperas only with bgp -
minimum where i can reproduce this was about 130k prefixes with about 286
nexthops
bisected again and same result:
b838d5e1c5b6e57b10ec8af2268824041e3ea911 is the first bad commit
commit b838d5e1c5b6e57b10ec8af2268824041e3ea911
Author: Wei Wang [off-list ref]
Date: Sat Jun 17 10:42:32 2017 -0700
ipv4: mark DST_NOGC and remove the operation of dst_free()
With the previous preparation patches, we are ready to get rid of the
dst gc operation in ipv4 code and release dst based on refcnt only.
So this patch adds DST_NOGC flag for all IPv4 dst and remove the
calls
to dst_free().
At this point, all dst created in ipv4 code do not use the dst gc
anymore and will be destroyed at the point when refcnt drops to 0.
Signed-off-by: Wei Wang [off-list ref]
Acked-by: Martin KaFai Lau [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
:040000 040000 9b7e7fb641de6531fc7887473ca47ef7cb6a11da
831a73b71d3df1755f3e24c0d3c86d7a93fd55e2 M net
Will add now version 2 of patch from Eric and we will see
From: Eric Dumazet <hidden> Date: 2017-09-21 01:17:22
On Wed, 2017-09-20 at 18:09 -0700, Wei Wang wrote:
quoted hunk
quoted
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
Hi Pawel,
Could you try the following debug patch on top of net-next branch and
reproduce the issue check if there are warning msg showing?
Yes, we believe skb_dst_force() and skb_dst_force_safe() should be
unified (to the 'safe' version)
We no longer have gc to protect from 0 -> 1 transition of dst refcount.
From: Paweł Staszewski <hidden> Date: 2017-09-21 09:08:24
W dniu 2017-09-21 o 03:17, Eric Dumazet pisze:
On Wed, 2017-09-20 at 18:09 -0700, Wei Wang wrote:
quoted
quoted
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
Hi Pawel,
Could you try the following debug patch on top of net-next branch and
reproduce the issue check if there are warning msg showing?
Yes, we believe skb_dst_force() and skb_dst_force_safe() should be
unified (to the 'safe' version)
We no longer have gc to protect from 0 -> 1 transition of dst refcount.
From: Eric Dumazet <hidden> Date: 2017-09-21 11:03:50
On Thu, 2017-09-21 at 11:06 +0200, Paweł Staszewski wrote:
W dniu 2017-09-21 o 03:17, Eric Dumazet pisze:
quoted
On Wed, 2017-09-20 at 18:09 -0700, Wei Wang wrote:
quoted
quoted
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
Hi Pawel,
Could you try the following debug patch on top of net-next branch and
reproduce the issue check if there are warning msg showing?
Yes, we believe skb_dst_force() and skb_dst_force_safe() should be
unified (to the 'safe' version)
We no longer have gc to protect from 0 -> 1 transition of dst refcount.
OK we have two problems here
1) We need to unify skb_dst_force() ( for net tree )
2) Vlan devices should try to correctly handle IFF_XMIT_DST_RELEASE from
lower device. This will considerably help your performance.
For 1), this is what I had in mind, can you try it ?
Thanks a lot !
From: Paweł Staszewski <hidden> Date: 2017-09-21 11:14:17
W dniu 2017-09-21 o 13:03, Eric Dumazet pisze:
quoted hunk
On Thu, 2017-09-21 at 11:06 +0200, Paweł Staszewski wrote:
quoted
W dniu 2017-09-21 o 03:17, Eric Dumazet pisze:
quoted
On Wed, 2017-09-20 at 18:09 -0700, Wei Wang wrote:
quoted
quoted
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
Hi Pawel,
Could you try the following debug patch on top of net-next branch and
reproduce the issue check if there are warning msg showing?
Yes, we believe skb_dst_force() and skb_dst_force_safe() should be
unified (to the 'safe' version)
We no longer have gc to protect from 0 -> 1 transition of dst refcount.
OK we have two problems here
1) We need to unify skb_dst_force() ( for net tree )
2) Vlan devices should try to correctly handle IFF_XMIT_DST_RELEASE from
lower device. This will considerably help your performance.
For 1), this is what I had in mind, can you try it ?
Thanks a lot !
Thanks
What is weird i have this part in my net-next from git:
/**
* skb_dst_force_safe - makes sure skb dst is refcounted
* @skb: buffer
*
* If dst is not yet refcounted and not destroyed, grab a ref on it.
*/
static inline void skb_dst_force_safe(struct sk_buff *skb)
{
if (skb_dst_is_noref(skb)) {
struct dst_entry *dst = skb_dst(skb);
if (!dst_hold_safe(dst))
dst = NULL;
skb->_skb_refdst = (unsigned long)dst;
}
}
From: Paweł Staszewski <hidden> Date: 2017-09-21 11:16:29
W dniu 2017-09-21 o 13:12, Paweł Staszewski pisze:
W dniu 2017-09-21 o 13:03, Eric Dumazet pisze:
quoted
On Thu, 2017-09-21 at 11:06 +0200, Paweł Staszewski wrote:
quoted
W dniu 2017-09-21 o 03:17, Eric Dumazet pisze:
quoted
On Wed, 2017-09-20 at 18:09 -0700, Wei Wang wrote:
quoted
quoted
Thanks very much Pawel for the feedback.
I was looking into the code (specifically IPv4 part) and found
that in
free_fib_info_rcu(), we call free_nh_exceptions() without holding
the
fnhe_lock. I am wondering if that could cause some race condition on
fnhe->fnhe_rth_input/output so a double call on dst_dev_put() on the
same dst could be happening.
But as we call free_fib_info_rcu() only after the grace period, and
the lookup code which could potentially modify
fnhe->fnhe_rth_input/output all holds rcu_read_lock(), it seems
fine...
Hi Pawel,
Could you try the following debug patch on top of net-next branch and
reproduce the issue check if there are warning msg showing?
Yes, we believe skb_dst_force() and skb_dst_force_safe() should be
unified (to the 'safe' version)
We no longer have gc to protect from 0 -> 1 transition of dst
refcount.
OK we have two problems here
1) We need to unify skb_dst_force() ( for net tree )
2) Vlan devices should try to correctly handle IFF_XMIT_DST_RELEASE from
lower device. This will considerably help your performance.
For 1), this is what I had in mind, can you try it ?
Thanks a lot !
sk_buff *skb)
}
}
+/**
+ * skb_dst_force - makes sure skb dst is refcounted
+ * @skb: buffer
+ *
+ * If dst is not yet refcounted, let's do it
+ */
+static inline void skb_dst_force(struct sk_buff *skb)
+{
+ if (skb_dst_is_noref(skb)) {
+ struct dst_entry *dst = skb_dst(skb);
+
+ WARN_ON(!rcu_read_lock_held());
+ if (!dst_hold_safe(dst))
+ dst = NULL;
+ skb->_skb_refdst = (unsigned long)dst;
+ }
+}
/**
* __skb_tunnel_rx - prepare skb for rx reinsert
Thanks
What is weird i have this part in my net-next from git:
/**
* skb_dst_force_safe - makes sure skb dst is refcounted
* @skb: buffer
*
* If dst is not yet refcounted and not destroyed, grab a ref on it.
*/
static inline void skb_dst_force_safe(struct sk_buff *skb)
{
if (skb_dst_is_noref(skb)) {
struct dst_entry *dst = skb_dst(skb);
if (!dst_hold_safe(dst))
dst = NULL;
skb->_skb_refdst = (unsigned long)dst;
}
}
ok the difference is skb_dst_force_safe not skb_dst_force
From: Paweł Staszewski <hidden> Date: 2017-09-21 11:33:47
W dniu 2017-09-21 o 13:03, Eric Dumazet pisze:
quoted hunk
OK we have two problems here
1) We need to unify skb_dst_force() ( for net tree )
2) Vlan devices should try to correctly handle IFF_XMIT_DST_RELEASE from
lower device. This will considerably help your performance.
For 1), this is what I had in mind, can you try it ?
Thanks a lot !
From: Paweł Staszewski <hidden> Date: 2017-09-21 13:20:59
W dniu 2017-09-21 o 13:31, Paweł Staszewski pisze:
W dniu 2017-09-21 o 13:03, Eric Dumazet pisze:
quoted
OK we have two problems here
1) We need to unify skb_dst_force() ( for net tree )
2) Vlan devices should try to correctly handle IFF_XMIT_DST_RELEASE from
lower device. This will considerably help your performance.
For 1), this is what I had in mind, can you try it ?
Thanks a lot !
From: Eric Dumazet <hidden> Date: 2017-09-21 16:15:49
From: Eric Dumazet <edumazet@google.com>
In linux-4.13, Wei worked hard to convert dst to a traditional
refcounted model, removing GC.
We now want to make sure a dst refcount can not transition from 0 back
to 1.
The problem here is that input path attached a not refcounted dst to an
skb. Then later, because packet is forwarded and hits skb_dst_force()
before exiting RCU section, we might try to take a refcount on one dst
that is about to be freed, if another cpu saw 1 -> 0 transition in
dst_release() and queued the dst for freeing after one RCU grace period.
Lets unify skb_dst_force() and skb_dst_force_safe(), since we should
always perform the complete check against dst refcount, and not assume
it is not zero.
Bugzilla : https://bugzilla.kernel.org/show_bug.cgi?id=197005
[ 989.919496] skb_dst_force+0x32/0x34
[ 989.919498] __dev_queue_xmit+0x1ad/0x482
[ 989.919501] ? eth_header+0x28/0xc6
[ 989.919502] dev_queue_xmit+0xb/0xd
[ 989.919504] neigh_connected_output+0x9b/0xb4
[ 989.919507] ip_finish_output2+0x234/0x294
[ 989.919509] ? ipt_do_table+0x369/0x388
[ 989.919510] ip_finish_output+0x12c/0x13f
[ 989.919512] ip_output+0x53/0x87
[ 989.919513] ip_forward_finish+0x53/0x5a
[ 989.919515] ip_forward+0x2cb/0x3e6
[ 989.919516] ? pskb_trim_rcsum.part.9+0x4b/0x4b
[ 989.919518] ip_rcv_finish+0x2e2/0x321
[ 989.919519] ip_rcv+0x26f/0x2eb
[ 989.919522] ? vlan_do_receive+0x4f/0x289
[ 989.919523] __netif_receive_skb_core+0x467/0x50b
[ 989.919526] ? tcp_gro_receive+0x239/0x239
[ 989.919529] ? inet_gro_receive+0x226/0x238
[ 989.919530] __netif_receive_skb+0x4d/0x5f
[ 989.919532] netif_receive_skb_internal+0x5c/0xaf
[ 989.919533] napi_gro_receive+0x45/0x81
[ 989.919536] ixgbe_poll+0xc8a/0xf09
[ 989.919539] ? kmem_cache_free_bulk+0x1b6/0x1f7
[ 989.919540] net_rx_action+0xf4/0x266
[ 989.919543] __do_softirq+0xa8/0x19d
[ 989.919545] irq_exit+0x5d/0x6b
[ 989.919546] do_IRQ+0x9c/0xb5
[ 989.919548] common_interrupt+0x93/0x93
[ 989.919548] </IRQ>
Similarly dst_clone() can use dst_hold() helper to have additional
debugging, as a follow up to commit 44ebe79149ff ("net: add debug
atomic_inc_not_zero() in dst_hold()")
In net-next we will convert dst atomic_t to refcount_t for peace of
mind.
Fixes: a4c2fd7f7891 ("net: remove DST_NOCACHE flag")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <redacted>
Reported-by: Paweł Staszewski <redacted>
Bisected-by: Paweł Staszewski [off-list ref]
---
include/net/dst.h | 22 ++++------------------
include/net/route.h | 2 +-
include/net/sock.h | 2 +-
3 files changed, 6 insertions(+), 20 deletions(-)
@@ -856,7 +856,7 @@ void sk_stream_write_space(struct sock *sk);staticinlinevoid__sk_add_backlog(structsock*sk,structsk_buff*skb){/* dont let skb dst not refcounted, we are going to leave rcu lock */-skb_dst_force_safe(skb);+skb_dst_force(skb);if(!sk->sk_backlog.tail)sk->sk_backlog.head=skb;
On Thu, Sep 21, 2017 at 9:15 AM, Eric Dumazet [off-list ref] wrote:
From: Eric Dumazet <edumazet@google.com>
In linux-4.13, Wei worked hard to convert dst to a traditional
refcounted model, removing GC.
We now want to make sure a dst refcount can not transition from 0 back
to 1.
The problem here is that input path attached a not refcounted dst to an
skb. Then later, because packet is forwarded and hits skb_dst_force()
before exiting RCU section, we might try to take a refcount on one dst
that is about to be freed, if another cpu saw 1 -> 0 transition in
dst_release() and queued the dst for freeing after one RCU grace period.
Lets unify skb_dst_force() and skb_dst_force_safe(), since we should
always perform the complete check against dst refcount, and not assume
it is not zero.
Bugzilla : https://bugzilla.kernel.org/show_bug.cgi?id=197005
[ 989.919496] skb_dst_force+0x32/0x34
[ 989.919498] __dev_queue_xmit+0x1ad/0x482
[ 989.919501] ? eth_header+0x28/0xc6
[ 989.919502] dev_queue_xmit+0xb/0xd
[ 989.919504] neigh_connected_output+0x9b/0xb4
[ 989.919507] ip_finish_output2+0x234/0x294
[ 989.919509] ? ipt_do_table+0x369/0x388
[ 989.919510] ip_finish_output+0x12c/0x13f
[ 989.919512] ip_output+0x53/0x87
[ 989.919513] ip_forward_finish+0x53/0x5a
[ 989.919515] ip_forward+0x2cb/0x3e6
[ 989.919516] ? pskb_trim_rcsum.part.9+0x4b/0x4b
[ 989.919518] ip_rcv_finish+0x2e2/0x321
[ 989.919519] ip_rcv+0x26f/0x2eb
[ 989.919522] ? vlan_do_receive+0x4f/0x289
[ 989.919523] __netif_receive_skb_core+0x467/0x50b
[ 989.919526] ? tcp_gro_receive+0x239/0x239
[ 989.919529] ? inet_gro_receive+0x226/0x238
[ 989.919530] __netif_receive_skb+0x4d/0x5f
[ 989.919532] netif_receive_skb_internal+0x5c/0xaf
[ 989.919533] napi_gro_receive+0x45/0x81
[ 989.919536] ixgbe_poll+0xc8a/0xf09
[ 989.919539] ? kmem_cache_free_bulk+0x1b6/0x1f7
[ 989.919540] net_rx_action+0xf4/0x266
[ 989.919543] __do_softirq+0xa8/0x19d
[ 989.919545] irq_exit+0x5d/0x6b
[ 989.919546] do_IRQ+0x9c/0xb5
[ 989.919548] common_interrupt+0x93/0x93
[ 989.919548] </IRQ>
Similarly dst_clone() can use dst_hold() helper to have additional
debugging, as a follow up to commit 44ebe79149ff ("net: add debug
atomic_inc_not_zero() in dst_hold()")
In net-next we will convert dst atomic_t to refcount_t for peace of
mind.
Fixes: a4c2fd7f7891 ("net: remove DST_NOCACHE flag")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <redacted>
Reported-by: Paweł Staszewski <redacted>
Bisected-by: Paweł Staszewski [off-list ref]
---
Thanks a lot for the fix Eric. It makes sense to unify all the usage
of skb_dst_force() to always check on the refcnt not being 0.
And thank you Pawel for reporting and testing on this.
Acked-by: Wei Wang <redacted>
@@ -856,7 +856,7 @@ void sk_stream_write_space(struct sock *sk);staticinlinevoid__sk_add_backlog(structsock*sk,structsk_buff*skb){/* dont let skb dst not refcounted, we are going to leave rcu lock */-skb_dst_force_safe(skb);+skb_dst_force(skb);if(!sk->sk_backlog.tail)sk->sk_backlog.head=skb;
From: Martin KaFai Lau <hidden> Date: 2017-09-21 17:12:30
On Thu, Sep 21, 2017 at 04:15:46PM +0000, Eric Dumazet wrote:
From: Eric Dumazet <edumazet@google.com>
In linux-4.13, Wei worked hard to convert dst to a traditional
refcounted model, removing GC.
We now want to make sure a dst refcount can not transition from 0 back
to 1.
The problem here is that input path attached a not refcounted dst to an
skb. Then later, because packet is forwarded and hits skb_dst_force()
before exiting RCU section, we might try to take a refcount on one dst
that is about to be freed, if another cpu saw 1 -> 0 transition in
dst_release() and queued the dst for freeing after one RCU grace period.
Lets unify skb_dst_force() and skb_dst_force_safe(), since we should
always perform the complete check against dst refcount, and not assume
it is not zero.
Acked-by: Martin KaFai Lau <redacted>
quoted hunk
Bugzilla : https://bugzilla.kernel.org/show_bug.cgi?id=197005
[ 989.919496] skb_dst_force+0x32/0x34
[ 989.919498] __dev_queue_xmit+0x1ad/0x482
[ 989.919501] ? eth_header+0x28/0xc6
[ 989.919502] dev_queue_xmit+0xb/0xd
[ 989.919504] neigh_connected_output+0x9b/0xb4
[ 989.919507] ip_finish_output2+0x234/0x294
[ 989.919509] ? ipt_do_table+0x369/0x388
[ 989.919510] ip_finish_output+0x12c/0x13f
[ 989.919512] ip_output+0x53/0x87
[ 989.919513] ip_forward_finish+0x53/0x5a
[ 989.919515] ip_forward+0x2cb/0x3e6
[ 989.919516] ? pskb_trim_rcsum.part.9+0x4b/0x4b
[ 989.919518] ip_rcv_finish+0x2e2/0x321
[ 989.919519] ip_rcv+0x26f/0x2eb
[ 989.919522] ? vlan_do_receive+0x4f/0x289
[ 989.919523] __netif_receive_skb_core+0x467/0x50b
[ 989.919526] ? tcp_gro_receive+0x239/0x239
[ 989.919529] ? inet_gro_receive+0x226/0x238
[ 989.919530] __netif_receive_skb+0x4d/0x5f
[ 989.919532] netif_receive_skb_internal+0x5c/0xaf
[ 989.919533] napi_gro_receive+0x45/0x81
[ 989.919536] ixgbe_poll+0xc8a/0xf09
[ 989.919539] ? kmem_cache_free_bulk+0x1b6/0x1f7
[ 989.919540] net_rx_action+0xf4/0x266
[ 989.919543] __do_softirq+0xa8/0x19d
[ 989.919545] irq_exit+0x5d/0x6b
[ 989.919546] do_IRQ+0x9c/0xb5
[ 989.919548] common_interrupt+0x93/0x93
[ 989.919548] </IRQ>
Similarly dst_clone() can use dst_hold() helper to have additional
debugging, as a follow up to commit 44ebe79149ff ("net: add debug
atomic_inc_not_zero() in dst_hold()")
In net-next we will convert dst atomic_t to refcount_t for peace of
mind.
Fixes: a4c2fd7f7891 ("net: remove DST_NOCACHE flag")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <redacted>
Reported-by: Paweł Staszewski <redacted>
Bisected-by: Paweł Staszewski [off-list ref]
---
include/net/dst.h | 22 ++++------------------
include/net/route.h | 2 +-
include/net/sock.h | 2 +-
3 files changed, 6 insertions(+), 20 deletions(-)
@@ -856,7 +856,7 @@ void sk_stream_write_space(struct sock *sk);staticinlinevoid__sk_add_backlog(structsock*sk,structsk_buff*skb){/* dont let skb dst not refcounted, we are going to leave rcu lock */-skb_dst_force_safe(skb);+skb_dst_force(skb);if(!sk->sk_backlog.tail)sk->sk_backlog.head=skb;
From: Eric Dumazet <edumazet@google.com>
In linux-4.13, Wei worked hard to convert dst to a traditional
refcounted model, removing GC.
We now want to make sure a dst refcount can not transition from 0 back
to 1.
The problem here is that input path attached a not refcounted dst to an
skb. Then later, because packet is forwarded and hits skb_dst_force()
before exiting RCU section, we might try to take a refcount on one dst
that is about to be freed, if another cpu saw 1 -> 0 transition in
dst_release() and queued the dst for freeing after one RCU grace period.
Lets unify skb_dst_force() and skb_dst_force_safe(), since we should
always perform the complete check against dst refcount, and not assume
it is not zero.
Bugzilla : https://bugzilla.kernel.org/show_bug.cgi?id=197005
...
Similarly dst_clone() can use dst_hold() helper to have additional
debugging, as a follow up to commit 44ebe79149ff ("net: add debug
atomic_inc_not_zero() in dst_hold()")
In net-next we will convert dst atomic_t to refcount_t for peace of
mind.
Fixes: a4c2fd7f7891 ("net: remove DST_NOCACHE flag")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <redacted>
Reported-by: Paweł Staszewski <redacted>
Bisected-by: Paweł Staszewski [off-list ref]