zebra: vlan to dplane

[raja-rajasekar]

Commit-2 : zebra: vlan to dplane Offload from main

    Trigger: Zebra core seen when we convert l2vni to l3vni and back

    BackTrace:
    /usr/lib/x86_64-linux-gnu/frr/libfrr.so.0(_zlog_assert_failed+0xe9) [0x7f4af96989d9]
    /usr/lib/frr/zebra(zebra_vxlan_if_vni_up+0x250) [0x5561022ae030]
    /usr/lib/frr/zebra(netlink_vlan_change+0x2f4) [0x5561021fd354]
    /usr/lib/frr/zebra(netlink_parse_info+0xff) [0x55610220d37f]
    /usr/lib/frr/zebra(+0xc264a) [0x55610220d64a]
    /usr/lib/x86_64-linux-gnu/frr/libfrr.so.0(thread_call+0x7d) [0x7f4af967e96d]
    /usr/lib/x86_64-linux-gnu/frr/libfrr.so.0(frr_run+0xe8) [0x7f4af9637588]
    /usr/lib/frr/zebra(main+0x402) [0x5561021f4d32]
    /lib/x86_64-linux-gnu/libc.so.6(+0x2724a) [0x7f4af932624a]
    /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x85) [0x7f4af9326305]
    /usr/lib/frr/zebra(_start+0x21) [0x5561021f72f1]

    Root Cause:
    In working case,
     - We get a RTM_NEWLINK whose ctx is enqueued by zebra dplane and
       dequeued by zebra main and processed i.e.
       (102000 is deleted from vxlan99) before we handle RTM_NEWVLAN.
     - So in handling of NEWVLAN (vxlan99) we bail out since find with
       vlan id 703 does not exist.

    root@leaf2:mgmt:/var/log/frr# cat ~/raja_logs/working/nocras.log  | grep "RTM_NEWLINK\|QUEUED\|vxlan99\|in thread"
    2024/07/18 23:09:33.741105 ZEBRA: [KMXEB-K771Y] netlink_parse_info: netlink-dp-in (NS 0) type RTM_NEWLINK(16), len=616, seq=0, pid=0
    2024/07/18 23:09:33.744061 ZEBRA: [K8FXY-V65ZJ] Intf dplane ctx 0x7f2244000cf0, op INTF_INSTALL, ifindex (65), result QUEUED
    2024/07/18 23:09:33.767240 ZEBRA: [KMXEB-K771Y] netlink_parse_info: netlink-dp-in (NS 0) type RTM_NEWLINK(16), len=508, seq=0, pid=0
    2024/07/18 23:09:33.767380 ZEBRA: [K8FXY-V65ZJ] Intf dplane ctx 0x7f2244000cf0, op INTF_INSTALL, ifindex (73), result QUEUED
    2024/07/18 23:09:33.767389 ZEBRA: [NVFT0-HS1EX] INTF_INSTALL for vxlan99(73)
    2024/07/18 23:09:33.767404 ZEBRA: [TQR2A-H2RFY] Vlan-Vni(1186:1186-6000002:6000002) update for VxLAN IF vxlan99(73)
    2024/07/18 23:09:33.767422 ZEBRA: [TP4VP-XZ627] Del L2-VNI 102000 intf vxlan99(73)
    2024/07/18 23:09:33.767858 ZEBRA: [QYXB9-6RNNK] RTM_NEWVLAN bridge IF vxlan99 NS 0
    2024/07/18 23:09:33.767866 ZEBRA: [KKZGZ-8PCDW] Cannot find VNI for VID (703) IF vxlan99 for vlan state update >>>>BAIL OUT

    In failure case,
     - The NEWVLAN is received first even before processing RTM_NEWLINK.
     - Since the vxlan id 102000 is not removed from the vxlan99,
       the find with vlan id 703 returns the 102000 one and we invoke
       zebra_vxlan_if_vni_up where the interfaces don't match and assert.

    root@leaf2:mgmt:/var/log/frr# cat ~/raja_logs/noworking/crash.log | grep "RTM_NEWLINK\|QUEUED\|vxlan99\|in thread"
    2024/07/18 22:26:43.829370 ZEBRA: [KMXEB-K771Y] netlink_parse_info: netlink-dp-in (NS 0) type RTM_NEWLINK(16), len=616, seq=0, pid=0
    2024/07/18 22:26:43.829646 ZEBRA: [K8FXY-V65ZJ] Intf dplane ctx 0x7fe07c026d30, op INTF_INSTALL, ifindex (65), result QUEUED
    2024/07/18 22:26:43.853930 ZEBRA: [QYXB9-6RNNK] RTM_NEWVLAN bridge IF vxlan99 NS 0
    2024/07/18 22:26:43.853949 ZEBRA: [K61WJ-XQQ3X] Intf vxlan99(73) L2-VNI 102000 is UP >>> VLAN PROCESSED BEFORE INTF EVENT
    2024/07/18 22:26:43.853951 ZEBRA: [SPV50-BX2RP] RAJA zevpn_vxlanif vxlan48 and ifp vxlan99
    2024/07/18 22:26:43.854005 ZEBRA: [KMXEB-K771Y] netlink_parse_info: netlink-dp-in (NS 0) type RTM_NEWLINK(16), len=508, seq=0, pid=0
    2024/07/18 22:26:43.854241 ZEBRA: [KMXEB-K771Y] netlink_parse_info: netlink-dp-in (NS 0) type RTM_NEWLINK(16), len=516, seq=0, pid=0
    2024/07/18 22:26:43.854251 ZEBRA: [KMXEB-K771Y] netlink_parse_info: netlink-dp-in (NS 0) type RTM_NEWLINK(16), len=544, seq=0, pid=0
    ZEBRA: in thread kernel_read scheduled from zebra/kernel_netlink.c:505 kernel_read()

    Fix:
    Similar to #13396, where link change
    handling was offloaded to dplane, same is being done for vlan events.

    Note: Prior to this change, zebra main thread was interested in the
    RTNLGRP_BRVLAN. So all the kernel events pertaining to vlan was
    handled by zebra main.

    With this change change as well the handling of vlan events is still
    with Zebra main. However we offload it via Dplane thread.

    Ticket :#3878175

    Signed-off-by: Rajasekar Raja <[email protected]>

Commit-1 : zebra: vlan to dplane, Relocating some functions

    Relocating functions used by vlan in if_netlink into zebra vxlan

    Note: Static variable to the functions will be added back in the next
    commit.

    Ticket :#3878175

    Signed-off-by: Rajasekar Raja <[email protected]>

[raja-rajasekar]

ci:rerun

[raja-rajasekar]

ci:rerun

[mjstapp]

I think we'd better have a comment that the ctx takes over the memory - it's not copying, right?

[raja-rajasekar]

we just do ctx->u.vlan_info.vlan_array = vlan_array; and free it when ctx is freed in dplane_ctx_free_internal().

[mjstapp]

that's what I mean, exactly: the caller must allocate some memory, and must not free it, and the caller must not hand in info on the stack.

[raja-rajasekar]

I have added the comment in zebra_dplane.c where the function is defined.
https://github.com/FRRouting/frr/pull/16737/files#diff-8351b0eeaf9e92cf06ff9d3449825c62c2332dbe83109fe2df56d5606b5f75f5R3357

[mjstapp]

hmm, gosh - I really tried to say exactly what I meant to say. I'll say more:
I want the dplane api to be better, and more useful. That includes being easier, safer to work with.
there are several patterns in the dplane code that involve structs or chunks of memory. there's a pattern where the dplane ctx has an embedded, fixed struct, and the caller's data is copied in. there's a pattern where the dplane ctx allocates memory, and copies the caller's data; the ctx allocates the memory and must free it. and then there's the worst possible pattern: the caller allocates some memory, and hands it over to the dplane ctx, who becomes responsible for it. that's particularly painful with our memory library, because the caller and the dplane ctx code must agree about the MTYPE used for the allocation. and that third pattern is the one you've chosen to use. I didn't reject that out of hand - though I was tempted to. but I really do want there to be a comment there in the header file so that if someone ever wants to know this crucial piece of information without actually reading the dplane source code, there will be some chance that the information will be highlighted, and the pattern will be reinforced that useful comments about critical behavior in the api are good - and even expected.

[raja-rajasekar]

Thanks, Mark for the detailed explanation.
I will keep this in mind for future coding as well..

[raja-rajasekar]

ci:rerun

[mjstapp]

Looks good, thanks