Page MenuHomeFreeBSD
Differential D55850

nd6: Fix delayed proxy address
Needs ReviewPublic
Actions

Authored by pouria on Fri, Mar 13, 11:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 27, 1:27 AM
Unknown Object (File)
Wed, Mar 25, 8:30 AM
Unknown Object (File)
Tue, Mar 24, 9:40 AM
Unknown Object (File)
Tue, Mar 24, 8:52 AM
Unknown Object (File)
Tue, Mar 24, 7:12 AM
Unknown Object (File)
Mon, Mar 23, 9:51 PM
Unknown Object (File)
Mon, Mar 23, 7:13 AM
Unknown Object (File)
Mon, Mar 23, 7:13 AM
View All 14 Files
Subscribers
ae
asomers
bms
imp
markj
melifaro

Details

Reviewers
glebius
zlei
bz
markj
Group Reviewers
network
Summary

delayed proxy addresses needs special handling since
it can use linklocal ifa as their source address and different
link-layer data in their response.

Fixes: f37fbe30f559
Fixes: 75f1665f3346

Test Plan
kyua debug -k /usr/tests/Kyuafile sys/netinet6/proxy_ndp

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 71768
Build 68651: arc lint + arc unit

Event Timeline

pouria created this revision.Fri, Mar 13, 11:26 PM
Herald added subscribers: melifaro, ae, asomers, imp. · View Herald TranscriptFri, Mar 13, 11:26 PM
pouria requested review of this revision.Fri, Mar 13, 11:26 PM
Harbormaster completed remote builds in B71394: Diff 173667.Fri, Mar 13, 11:26 PM
bms added a subscriber: bms.Fri, Mar 13, 11:27 PM
pouria added a comment.Fri, Mar 13, 11:53 PM

Hi @glebius,

Please look at the panic below produced by this patch when you have time.
I already reverted the delayed proxy address changes from the main branch, so this won't happen in production.
Honestly, delayed proxy addresses aren't that important.
However, it looks like we need to use a different lock to handle delayed proxy addresses.

panic: /usr/src/sys/netinet6/nd6_nbr.c:1665: nd6_queue_rel: Bad link elm 0xfffff80028a00900 prev->next != elm
cpuid = 2
time = 1773444926
KDB: stack backtrace:
#0 0xffffffff80bed6cd at kdb_backtrace+0x5d
#1 0xffffffff80b9c2fe at vpanic+0x16e
#2 0xffffffff80b9c183 at panic+0x43
#3 0xffffffff80df90cc at nd6_queue_rel+0xdc
#4 0xffffffff80bb9e8b at softclock_call_cc+0x19b
#5 0xffffffff80bbb51d at softclock_thread+0xed
#6 0xffffffff80b4cce2 at fork_exit+0x82
#7 0xffffffff810acd7e at fork_trampoline+0xe
KDB: enter: panic

I already tried many ideas like getting the ifp reference and/or updating the ifp/in6_extra based on sdl->sdl_index value using ifnet_byindex.
This happens because of:

sys/netinet6/nd6_nbr.c
281

This line here.
Since proxy address don't have a local ifa, they get a link-local address to use as its source.
Because of that, I can't use the ifa lock to protect the nd_queue list.
To reproduce, run the proxy_ndp tests with this patch.

At this point, I'm stuck and don't know how to resolve this specific scenario.

pouria added a comment.Fri, Mar 13, 11:57 PM

I also tried to replace the TAILQ with STAILQ to avoid this line:

panic: /usr/src/sys/netinet6/nd6_nbr.c:1665: nd6_queue_rel: Bad link elm 0xfffff80028a00900 prev->next != elm

But the actual problem is the data is already freed and this is not just a consistency check failure from TAILQ (prev->next != elm).

pouria edited the summary of this revision. (Show Details)Fri, Mar 13, 11:58 PM
pouria edited the summary of this revision. (Show Details)
pouria updated this revision to Diff 173900.Thu, Mar 19, 2:15 PM

Rebase to D55905.

sys/netinet6/proxy_ndp:pndp_add_gu_success  ->  passed  [2.529s]
sys/netinet6/proxy_ndp:pndp_del_gu_success  ->  passed  [4.840s]
sys/netinet6/proxy_ndp:pndp_ifdestroy_success  ->  passed  [2.899s]
sys/netinet6/proxy_ndp:pndp_neighbor_advert  ->  passed  [5.192s]
Harbormaster completed remote builds in B71511: Diff 173900.Thu, Mar 19, 2:15 PM
pouria retitled this revision from ndp: Fix delayed proxy address (WIP) to ndp: Fix delayed proxy address.Thu, Mar 19, 2:16 PM
pouria edited the test plan for this revision. (Show Details)
pouria added reviewers: zlei, bz.
pouria added a parent revision: D55905: ndp: Accept multiple queued ND for non-GRAND NAs.
pouria updated this revision to Diff 173901.Thu, Mar 19, 2:20 PM

No need to touch proxy_ndp tests.
Remove it.

Harbormaster completed remote builds in B71512: Diff 173901.Thu, Mar 19, 2:20 PM
glebius accepted this revision as: glebius.Thu, Mar 19, 5:58 PM

Code wise looks good to me. But I'm not protocol expert.

sys/netinet6/nd6_nbr.c
1804

I'd suggest to expand the macro. Right now it has * which negates the inside &. IMHO, this is mind blowing and doesn't improve readability.

This revision is now accepted and ready to land.Thu, Mar 19, 5:58 PM
pouria updated this revision to Diff 173946.Thu, Mar 19, 6:14 PM

Address @glebius comment

This revision now requires review to proceed.Thu, Mar 19, 6:14 PM
Harbormaster completed remote builds in B71547: Diff 173946.Thu, Mar 19, 6:14 PM
pouria added a reviewer: markj.Fri, Mar 27, 11:09 AM
pouria added a subscriber: markj.

@markj could please review this change too?
even C-wise would help.

markj added a comment.Sat, Mar 28, 11:07 AM

I don't see any obvious problems with the change, but it's hard to understand the intent of the patch from the existing description.

sys/netinet6/nd6_nbr.c
1757

Some comment explaining the difference between taddr and daddr would be useful.

1804

Or just use memcpy() like you do above.

pouria updated this revision to Diff 174437.Sat, Mar 28, 1:27 PM
pouria marked an inline comment as done.

Add comments for nd6_queue_add and describe what are the ifa, daddr, taddr, and sdl.
@markj done.
You're right, even I was confused by those terms too, they definitely needed explanations.
Is it better now?

Harbormaster completed remote builds in B71768: Diff 174437.Sat, Mar 28, 1:27 PM
markj added inline comments.Sun, Mar 29, 7:27 AM
sys/netinet6/nd6_nbr.c
1725

We already initialized tlladdr = ND6_NA_OPT_LLA at the beginning of the function.

1804

I still don't like the mix of memcpy() and direct assignment. We should be consistent one way or the other.

pouria updated this revision to Diff 174474.Sun, Mar 29, 10:45 AM
pouria marked 3 inline comments as done.

Address @markj comments on memcpy and duplicate flags.

Harbormaster completed remote builds in B71791: Diff 174474.Sun, Mar 29, 10:45 AM
pouria added inline comments.Sun, Mar 29, 10:46 AM
sys/netinet6/nd6_nbr.c
1725

Oh, I left this one from previous revision. Thanks!

1804

Done.
Honestly, I don't like to use memcpy whenever possible.
But for consistency, make sense.
Thanks.

pouria retitled this revision from ndp: Fix delayed proxy address to nd6: Fix delayed proxy address.Wed, Apr 1, 4:49 PM
pouria added a comment.Wed, Apr 1, 5:32 PM

kind ping: @markj

Revision Contents
Changeset List

PathSize
sys/
netinet6/
1 line
73 lines

Diff 174437

View Options

sys/netinet6/nd6.h

Loading...
View Options

sys/netinet6/nd6_nbr.c

Loading...