-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pim 5549 Additions #304
Pim 5549 Additions #304
Conversation
Convert the pim address family from a #define into an enum. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
This code change adds the ability to specify that we should be able to work with older versions of PIM. In future commits we will actually use this data. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
When we are checking RP addresses and looking at the secondary address. With the addition of the ability to handle v6 addresses in the secondary list. Assuming that the secondary address is a v4 address is a no go. Convert to prefix_same. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
With RFC 5549 we need a methodology to find a neighbor based upon a nexthop that is v6 based. This commit sets us up for that by allowing you to find the neigbor by the secondary list. In a future commit we will add code to pass the v6 secondary addresses. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Add ability to encode/decode the v6 secondary addresses if they are passed to us. This also fixes the issue where if we are passed a v6 secondary address list we will not refuse to form neighbors. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Send v6 secondary addresses to our neighbor in hello's. Additionally allow the disabling it via the cli introduced earlier. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
When we receive a v6 nexthop in v4, lookup the nbr by it's secondary address. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Add code to properly receive v6 addresses up from zebra and to properly place them into our interface secondary address list. Additionally cleanup some code in pim_cmd.c that was broken by these changes. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Continous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-404/ This is a comment from an EXPERIMENTAL automated CI system. |
Continous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-405/ This is a comment from an EXPERIMENTAL automated CI system. |
Continous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-418/ This is a comment from an EXPERIMENTAL automated CI system. |
Continous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-440/ This is a comment from an EXPERIMENTAL automated CI system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good overall, but several places could use some sizeof
pimd/pim_cmd.c
Outdated
@@ -828,7 +829,8 @@ static void pim_show_interfaces_single(struct vty *vty, const char *ifname, u_ch | |||
|
|||
sec_list = json_object_new_array(); | |||
for (ALL_LIST_ELEMENTS_RO(pim_ifp->sec_addr_list, sec_node, sec_addr)) { | |||
json_object_array_add(sec_list, json_object_new_string(inet_ntoa(sec_addr->addr))); | |||
json_object_array_add(sec_list, | |||
json_object_new_string(prefix2str(&sec_addr->addr, pbuf, PREFIX2STR_BUFFER))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style / defensive coding: sizeof(pbuf)
instead of PREFIX2STR_BUFFER
pimd/pim_cmd.c
Outdated
for (ALL_LIST_ELEMENTS_RO(pim_ifp->sec_addr_list, sec_node, sec_addr)) { | ||
vty_out(vty, " %s%s", | ||
inet_ntoa(sec_addr->addr), VTY_NEWLINE); | ||
prefix2str(&sec_addr->addr, pbuf, PREFIX2STR_BUFFER), VTY_NEWLINE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style / defensive coding: sizeof(pbuf)
instead of PREFIX2STR_BUFFER
pimd/pim_cmd.c
Outdated
|
||
pim_inet4_dump("<src?>", p->u.prefix4, | ||
neigh_sec_str, sizeof(neigh_sec_str)); | ||
prefix2str(p, neigh_sec_str, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
urgh, the "100" sticks out like a sore thumb ... PREFIX2STR_BUFFER
above and sizeof
here?
pimd/pim_tlv.c
Outdated
|
||
p->family = AF_INET6; | ||
p->prefixlen = IPV6_MAX_PREFIXLEN; | ||
memcpy(&p->u.prefix6, addr, 16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizeof(struct in6_addr)
? (16
in one line + sizeof
in the next looks really weird)
pimd/pim_zlookup.c
Outdated
|
||
p.family = AF_INET6; | ||
p.prefixlen = IPV6_MAX_PREFIXLEN; | ||
memcpy (&p.u.prefix6, &nexthop_tab[num_ifindex].nexthop_addr.u.prefix6, 16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again :)
btw, yes, i'm nitpicking on this... it's just that the confusion with BUFSIZ is what led to the previous security issue in the IPv6 RA code, so i'm a bit sensitised on this. For a "defensive" coding style, only the variable declaration should have a number (or macro/constant), everything else should always be (And, yes, I'm working on a coccinelle semantic-patch to fix this across the codebase!) |
David -> All are reasonable, I'll fix the issues. |
@donaldsharp Will give this a go once you address the issues pointed out by @eqvinox |
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Continous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-449/ This is a comment from an EXPERIMENTAL automated CI system. |
This implements option #2 of https://tools.ietf.org/html/draft-pim-with-ipv4-prefix-over-ipv6-nh-00
PIM was partially implementing 5549, in that a received v6 NH would be looked up by the neighbor out that interface. In the case of shared media this method would fail.
During IETF discussion of this draft I strongly encouraged dropping options 1 and 3 and just going with option 2