FRRouting
isisd: fix ip/ipv6 reachability tlvs
Don't allocate subtlvs container if there's nothing to add to it. If the container is allocated, the "sub-TLVs presence" bit is set in the TLVs even if there's no actual sub-TLVs, what violates the RFC.
Fixes #14514.
backport stable/9.0 stable/9.1 dev/10.0
❌ No backport have been created
- #15996 isisd: fix ip/ipv6 reachability tlvs (backport #15655) has been created for branch
stable/9.0 - #15997 isisd: fix ip/ipv6 reachability tlvs (backport #15655) has been created for branch
stable/9.1 - Backport to branch
dev/10.0failed
GitHub error: Branch not found
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
That's exactly what I looked at.
pcfgs is defined as a fixed-length array which is always non-NULL:
struct sr_prefix_cfg *pcfgs[SR_ALGORITHM_COUNT] = {NULL};.
Then it is populated in the loop, but if there are no algorithms configured it will stay full of NULL pointers, which means we won't have any sub-TLVs. So when we get to isis_tlvs_add_extended_ip_reach the pcfgs pointer is never NULL even if all the pointers inside it are NULL. We must not allocate the container in this case.
That's exactly what I looked at.
pcfgsis defined as a fixed-length array which is always non-NULL:struct sr_prefix_cfg *pcfgs[SR_ALGORITHM_COUNT] = {NULL};.Then it is populated in the loop, but if there are no algorithms configured it will stay full of NULL pointers, which means we won't have any
sub-TLVs. So when we get toisis_tlvs_add_extended_ip_reachthepcfgspointer is never NULL even if all the pointers inside it are NULL. We must not allocate the container in this case.
OK. But, in this case, the modification should be done inside isis_lsp.c to not allocate an empty structure to pcfgs and pass a NULL pointer to isis_tlvs_add_extended_ip_reach if there is no Algorithms.
Why do you think it's better? It would just be much more code changes. isis_tlvs_add_extended_ip_reach and isis_tlvs_add_ipv6_reach are both called from 3 places where this needs to be fixed instead of a single place inside the function.
r->subtlvs is checked in several places in the code, not only in pack_item_extended_ip_reach. I think my fix is better than both suggestions, because it restores the previous behavior before flex-algo support – the struct is not allocated if there's no subTLVs. Both suggestions from @odd22 and @louis-6wind require much more changes to the code and more complicated checks instead of a simple NULL check.
Regarding the topotest question, I don't have time unfortunately to look into the topotests for that. You can add you own topotest commit to this branch if you want to have it before merging this PR.
I have tried to figure out how to make a topotest but it is much more complicated. Output from https://github.com/FRRouting/frr/issues/14514 description is from the Arista device. No output shows the issue in FRR.
@idryzhov @odd22, what happens if we have a prefix SID and we unconfigure it?
I have tested that with your fix @idryzhov and it works because TLVs are built from scratch each time the config changes.
I think my fix is better than both suggestions, because it restores the previous behavior before flex-algo support
correct
backport stable/9.0 stable/9.1 stable/10.0
✅ Backports have been created
- #15996 isisd: fix ip/ipv6 reachability tlvs (backport #15655) has been created for branch
stable/9.0 - #15997 isisd: fix ip/ipv6 reachability tlvs (backport #15655) has been created for branch
stable/9.1 - #15998 isisd: fix ip/ipv6 reachability tlvs (backport #15655) has been created for branch
stable/10.0
I can confirm that the fix makes IS-IS work between frr:master and Arista cEOS 4.32.0F. Great job, thank you!
