netinet6: fix proxy NDP

[takahiro.kurosawa_gmail.com]

Summary

We could insert proxy NDP entries by the ndp command, but the host
with proxy ndp entries had not responded to Neighbor Solicitations.
Change the following points for proxy NDP to work as expected:

• join solicited-node multicast addresses for proxy NDP entries in order to receive Neighbor Solicitations.
• look up proxy NDP entries not on the routing table but on the link-level address table when receiving Neighbor Solicitations.

Test Plan

Insert proxy NDP entry with "ndp -s <proxy-addr> <ether-addr> proxy"
and check joining the solicited-node multicast address with /usr/sbin/ifmcstat.

[takahiro.kurosawa_gmail.com]

Fix spell check warnings

[melifaro]

Thank you for the submission!
LGTM, added some comments inline.
Could you also add a test, verifying the actual behaviour? It should be relatively straight-forwarded, based on tests/sys/netinet6/forward6.sh (2 VNETs) && tests/sys/netinet6/ndp.sh (general NDP checks),

sys/net/if_llatbl.c

713

I'd rather simplify this check to only verify LLE_PUB flag set.
LLE_STATIC generally means that the entry is not temporary (and personally I don't think that temporary entries can't be proxy ones).
LLE_IFADDR cannot be set by the user code. If there are concerns about this case, I'd add another check in lla_rt_output() instead off doing this in the runtime.

Finally, why not move it to in6_lltable_delete_entry() callback?

1022

Worth introducing llt_post_resolved() (or similar) callback so we can move IPv4/IPv6 logic outside of if_llatbl.c,

[takahiro.kurosawa_gmail.com]

In D35307#800525, @melifaro wrote:

Could you also add a test, verifying the actual behaviour? It should be relatively straight-forwarded, based on tests/sys/netinet6/forward6.sh (2 VNETs) && tests/sys/netinet6/ndp.sh (general NDP checks),

Thank you for the review!
I'll add the test in the next update.

sys/net/if_llatbl.c

713

I'd rather simplify this check to only verify LLE_PUB flag set.

I think your comment is reasonable. I'll simply the check to verify the LLE_PUB flag only and move the code to in6_llatable_delete_entry().

1022

Worth introducing llt_post_resolved() (or similar) callback so we can move IPv4/IPv6 logic outside of if_llatbl.c,

Good idea! I'll post a separate diff that introduce the callback and move the existing ifdef INET/INET6s to the protocol specific code.

[melifaro]

sys/netinet6/in6.c

2648

I'd prefer using existing bits&pieces that we have instead of introducing another layer of locking.

I'd suggest doing the following:

• mark lltable (for example, by introducing special bitflag has_proxy @ space of llat_af (which actually need only 1 byte)) as one containing proxy entries when adding the first one
• on interface destruction, acquire llt rlock, check if bit is set, if yes, unlink/grab all matching lles (similar to htable_prefix_free()) and delete them.

2671–2691

Could you factor out this code from in6_update_ifa_join_mc() into a separate function, which can be used both here and in in6_update_ifa_join_mc() so we don't duplicate the logic?

sys/netinet6/nd6_nbr.c

269

Nit: no need to check LLE_STATIC and LLE_IFADDR here.

[takahiro.kurosawa_gmail.com]

sys/netinet6/in6.c

2648

I'd suggest doing the following:

• mark lltable (for example, by introducing special bitflag has_proxy @ space of llat_af (which actually need only 1 byte)) as one containing proxy entries when adding the first one

I have submitted D35323 to prepare for adding the member. Could you review it?

• on interface destruction, acquire llt rlock, check if bit is set, if yes, unlink/grab all matching lles (similar to htable_prefix_free()) and delete them.

I'll try to change the code as you proposed.

[takahiro.kurosawa_gmail.com]

• Add the lltable.llt_flags member and LLT_ADDEDPROXY flag in it
• Add lltable_flags_free() that frees LLEs with specified la_flags
• Use lltable_flags_free() for freeing in6_multis in if_detach path
• Add the test script

[takahiro.kurosawa_gmail.com]

sys/netinet6/in6.c

2648

I've added lltable_flags_free() for purging LLEs on if_detach case. If I miss the point, let me know.

2671–2691

I've added in6_solicited_node_maddr() for factoring out the common part.

sys/netinet6/nd6_nbr.c

269

Fixed. Thanks!

[melifaro]

Thank you for addressing all the feedback & adding tests!
I added a couple of more comments - should be the last round prior committing the feature.

sys/net/if_llatbl.c

733–779

Thank you for addressing the feedback!
Looks almost perfect, I'd just try to make it a bit more generic, factoring out the delete actions (that should be common) and the matching logic.
I've attached an example of what I have in mind.

sys/netinet6/in6.c

2669

Probably worth splitting add&delete use cases to the separation functions as they don't share anything except 1 line (in6_solicited_node_maddr).

2712–2717

Nit: probably be a bit simpler.

sys/netinet6/nd6_nbr.c

260–261

Nit: worth considering moving it out as a separate function? like, fill_proxyndp(struct infect *ifp, struct in6_addr *taddr, struct. sockaddr_dl *proxydl).
Should improve readability a bit.

[takahiro.kurosawa_gmail.com]

• Add lltable_delete_conditional() instead of lltable_flags_free().
• Use lltable_delete_conditional() for purging in6_multi for solicited node multicast addresses.
• Move out the sockaddr_dl setup code from nd6_ns_input() to a new function.

[takahiro.kurosawa_gmail.com]

sys/net/if_llatbl.c

733–779

Introduced lltable_delete_conditional() according to your advice.
The code has got much simpler, Thanks for your great advice!

sys/netinet6/in6.c

2669

Applied, thanks!

2712–2717

Applied.

sys/netinet6/nd6_nbr.c

260–261

Applied, readability gets improved indeed.

[melifaro]

Thank you! Will land everything in a day or two.