isisd: fix ip/ipv6 reachability tlvs

[idryzhov]

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.

[idryzhov]

@Mergifyio backport stable/9.0 stable/9.1 dev/10.0

[mergify]

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.0 failed

GitHub error: Branch not found

[odd22]

I'm disagree with this modification. In fact, if pcfgs is not null, this means that there is at least one subtlv to add. Thus, it is necessary to allocate memory for at least this subtlv. So, for me, it is not pertinent to move the subtlv memory allocation later in the loop. You could have a look to isis_lsp.c line 912 & 982 where the isis_tlvs_add_extended_ip_reach() function is called. You'll see that the pcfgs is the result of the loop around the SR algorithms.

[idryzhov]

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.

[odd22]

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.

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.

[idryzhov]

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.

[louis-6wind]

@idryzhov @odd22, what happens if we have a prefix SID and we unconfigure it?

IMO, you should modify pack_item_extended_ip_reach() and its IPv6 equivalent.

Only set flag and return subTLV if one of the list count is not zero.

[louis-6wind]

Could you amend an existing topotest to check the change?

[idryzhov]

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.

[louis-6wind]

LGTM

[louis-6wind]

I have tried to figure out how to make a topotest but it is much more complicated. Output from #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

[ton31337]

@Mergifyio backport stable/9.0 stable/9.1 stable/10.0

[mergify]

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

[ipspace]

I can confirm that the fix makes IS-IS work between frr:master and Arista cEOS 4.32.0F. Great job, thank you!