- Bird-users - Mailing Lists

[PATCH] more list node initialisation
by Andreas Rammhold 25 Nov '20

25 Nov '20

Hi, In addition to those list node initialisation patches that went into master so far I still have these two commits in my local tree. One of them is about the logging configuration that I described in my initial email on the topic. The other patch is about new memory buckets in the BGP implementation. I am still hunting another variant of this that crashes BIRD (with --enable-debug) whenever a Babel peer is shutting down.. andi-

3 4

Derive OSPF stub network configuration from interface?
by Kenth Eriksson 25 Nov '20

25 Nov '20

Is it possible to configure OSPF stub network by giving the interface as parameter instead of the prefix? E.g. current syntax is area { stubnet prefix; } But can it be stated as interface instead of prefix so that if the interface address changes I don't need to update my configuration file?

2 2

[PATCH v3 1/7] sysdep: Add wrapper to get random bytes
by Toke Høiland-Jørgensen 24 Nov '20

24 Nov '20

From: Toke Høiland-Jørgensen <toke(a)toke.dk> The Babel authentication code added by a subsequent commit needs a way to get random bytes for generating nonces. This patch adds a wrapper function in sysdep to get random bytes, and the required checks in configure.ac to select how to do it. The configure script tries, in order, getrandom(), getentropy() and reading from /dev/urandom. Signed-off-by: Toke Høiland-Jørgensen <toke(a)toke.dk> --- conf/conf.c | 1 + configure.ac | 4 ++ lib/birdlib.h | 3 ++ sysdep/unix/main.c | 1 + sysdep/unix/random.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 97 insertions(+) diff --git a/conf/conf.c b/conf/conf.c index 6f64b5416..5f20f6ed5 100644 --- a/conf/conf.c +++ b/conf/conf.c @@ -515,6 +515,7 @@ order_shutdown(int gr) c->gr_down = gr; config_commit(c, RECONFIG_HARD, 0); + random_close(); shutting_down = 1; } diff --git a/configure.ac b/configure.ac index eabb3d562..d27bb327b 100644 --- a/configure.ac +++ b/configure.ac @@ -373,6 +373,10 @@ elif test "$bird_cv_lib_log" != yes ; then LIBS="$LIBS $bird_cv_lib_log" fi +AC_CHECK_FUNCS(getrandom) +AC_CHECK_FUNCS(getentropy) +AC_CHECK_HEADERS(sys/random.h) + if test "$enable_debug" = yes ; then AC_DEFINE([DEBUGGING], [1], [Define to 1 if debugging is enabled]) LDFLAGS="$LDFLAGS -rdynamic" diff --git a/lib/birdlib.h b/lib/birdlib.h index 23036c1b1..61098f929 100644 --- a/lib/birdlib.h +++ b/lib/birdlib.h @@ -192,5 +192,8 @@ asm( /* Pseudorandom numbers */ u32 random_u32(void); +int random_bytes(char *buf, size_t size); +void random_close(void); +void random_init(void); #endif diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c index 2c7e3ceff..3903ecccb 100644 --- a/sysdep/unix/main.c +++ b/sysdep/unix/main.c @@ -851,6 +851,7 @@ main(int argc, char **argv) parse_args(argc, argv); log_switch(1, NULL, NULL); + random_init(); net_init(); resource_init(); timer_init(); diff --git a/sysdep/unix/random.c b/sysdep/unix/random.c index b1f5086fe..de81f3ca2 100644 --- a/sysdep/unix/random.c +++ b/sysdep/unix/random.c @@ -7,6 +7,21 @@ */ #include <stdlib.h> +#include <fcntl.h> +#include <unistd.h> +#include <errno.h> + +#include "sysdep/config.h" + +#ifdef HAVE_SYS_STAT_H +# include <sys/stat.h> +#endif +#ifdef HAVE_LINUX_RANDOM_H +# include <linux/random.h> +#endif +#if defined(HAVE_SYS_RANDOM_H) && (defined(HAVE_GETRANDOM) || defined(HAVE_GETENTROPY)) +# include <sys/random.h> +#endif #include "nest/bird.h" @@ -19,3 +34,76 @@ random_u32(void) rand_high = random(); return (rand_low & 0xffff) | ((rand_high & 0xffff) << 16); } + +void +random_init() +{ + char buf; + /* get a single random byte to trip any errors early */ + random_bytes(&buf, sizeof(buf)); +} + +#if defined(HAVE_GETRANDOM) || defined(HAVE_GENTROPY) +int +random_bytes(char *buf, size_t size) +{ + int n; + int flags = 0; + while (0 < size) { +#if defined(HAVE_GETRANDOM) + n = getrandom(buf, size, flags); +#else + n = getentropy(buf, size); +#endif + if (n < 0) { + if (errno == EINTR) + continue; + die("Couldn't get random bytes: %m"); + } + buf += n; + size -= n; + } + + return 0; +} + +void random_close(void) {} + +#else + +static int urandom_fd = -1; +int random_bytes(char *buf, size_t size) +{ + int n; + + if (urandom_fd < 0) + { + urandom_fd = open("/dev/urandom", O_RDONLY); + if (urandom_fd < 0) + die("Couldn't open /dev/urandom: %m"); + } + + do + { + n = read(urandom_fd, buf, size); + if (n <= 0) { + if (errno == EINTR) + continue; + die("Couldn't read from /dev/urandom: %m"); + } + buf += n; + size -= n; + } while (size > 0); + + return 0; +} + +void +random_close(void) +{ + if (urandom_fd >= 0) { + close(urandom_fd); + urandom_fd = -1; + } +} +#endif

1 0

[PATCH v2] lib/slab: introduce sl_allocz() function and use it in Babel
by Toke Høiland-Jørgensen 24 Nov '20

24 Nov '20

The babel protocol code was initialising objects returned from the slab allocator by assigning to each of the struct members individually, but wasn't touching the NODE member while doing so. This leads to warnings on debug builds since commit: baac7009063d ("List expensive check.") To fix this, introduce an sl_allocz() variant of the slab allocator which will zero out the memory before returning it, and switch all the babel call sites to use this version. The overhead for doing this should be negligible for small objects, and in the case of babel, the largest object being allocated was being zeroed anyway, so we can drop the memset in babel_read_tlv(). Signed-off-by: Toke Høiland-Jørgensen <toke(a)toke.dk> --- v2: - Introduce sl_allocz() instead of adding more zeroing to the babel code. lib/resource.h | 1 + lib/slab.c | 24 ++++++++++++++++++++++++ proto/babel/babel.c | 7 +++---- proto/babel/packets.c | 7 +++---- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/lib/resource.h b/lib/resource.h index ad17d9ed4..b56bcff55 100644 --- a/lib/resource.h +++ b/lib/resource.h @@ -83,6 +83,7 @@ typedef struct slab slab; slab *sl_new(pool *, unsigned size); void *sl_alloc(slab *); +void *sl_allocz(slab *); void sl_free(slab *, void *); /* diff --git a/lib/slab.c b/lib/slab.c index c3035b45b..f31355e02 100644 --- a/lib/slab.c +++ b/lib/slab.c @@ -88,6 +88,14 @@ sl_alloc(slab *s) return o->data; } +void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->size); + return obj; +} + void sl_free(slab *s, void *oo) { @@ -278,6 +286,22 @@ no_partial: goto okay; } +/** + * sl_allocz - allocate an object from Slab and zero it + * @s: slab + * + * sl_allocz() allocates space for a single object from the + * Slab and returns a pointer to the object after zeroing out + * the object memory. + */ +void * +sl_allocz(slab *s) +{ + void *obj = sl_alloc(s); + memset(obj, 0, s->data_size); + return obj; +} + /** * sl_free - return a free object back to a Slab * @s: slab diff --git a/proto/babel/babel.c b/proto/babel/babel.c index 618abaa8a..4b6b9d7f9 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -113,7 +113,7 @@ babel_get_source(struct babel_proto *p, struct babel_entry *e, u64 router_id) if (s) return s; - s = sl_alloc(p->source_slab); + s = sl_allocz(p->source_slab); s->router_id = router_id; s->expires = current_time() + BABEL_GARBAGE_INTERVAL; s->seqno = 0; @@ -159,8 +159,7 @@ babel_get_route(struct babel_proto *p, struct babel_entry *e, struct babel_neigh if (r) return r; - r = sl_alloc(p->route_slab); - memset(r, 0, sizeof(*r)); + r = sl_allocz(p->route_slab); r->e = e; r->neigh = nbr; @@ -323,7 +322,7 @@ babel_add_seqno_request(struct babel_proto *p, struct babel_entry *e, } /* No entries found */ - sr = sl_alloc(p->seqno_slab); + sr = sl_allocz(p->seqno_slab); found: sr->router_id = router_id; diff --git a/proto/babel/packets.c b/proto/babel/packets.c index d4ecf649d..e135866f4 100644 --- a/proto/babel/packets.c +++ b/proto/babel/packets.c @@ -1144,7 +1144,6 @@ babel_read_tlv(struct babel_tlv *hdr, return PARSE_ERROR; state->current_tlv_endpos = tlv_data[hdr->type].min_length; - memset(msg, 0, sizeof(*msg)); int res = tlv_data[hdr->type].read_tlv(hdr, msg, state); if (res != PARSE_SUCCESS) @@ -1275,7 +1274,7 @@ void babel_send_unicast(union babel_msg *msg, struct babel_iface *ifa, ip_addr dest) { struct babel_proto *p = ifa->proto; - struct babel_msg_node *msgn = sl_alloc(p->msg_slab); + struct babel_msg_node *msgn = sl_allocz(p->msg_slab); list queue; msgn->msg = *msg; @@ -1304,7 +1303,7 @@ void babel_enqueue(union babel_msg *msg, struct babel_iface *ifa) { struct babel_proto *p = ifa->proto; - struct babel_msg_node *msgn = sl_alloc(p->msg_slab); + struct babel_msg_node *msgn = sl_allocz(p->msg_slab); msgn->msg = *msg; add_tail(&ifa->msg_queue, NODE msgn); babel_kick_queue(ifa); @@ -1386,7 +1385,7 @@ babel_process_packet(struct babel_pkt_header *pkt, int len, break; } - msg = sl_alloc(p->msg_slab); + msg = sl_allocz(p->msg_slab); res = babel_read_tlv(tlv, &msg->msg, &state); if (res == PARSE_SUCCESS) { -- 2.29.2

5 9

[patch]: Uninitialized list nodes
by Mikael 24 Nov '20

24 Nov '20

Hi, I have found two occurrences of uninitialized list nodes in bird's master branch. The following config causes asserts (when built with debugging). # Router ID in IPv4 format router id 62.168.0.1; protocol device { interface "*"; } # Static route feed template static t1 { ipv4 { export all; }; route 10.0.0.0/24 unreachable; } protocol static from t1 { } The attached patches solves the problems. /Mikael

2 1

Calling named filters from another filter
by Michael Rack 23 Nov '20

23 Nov '20

Hi, i did not find anything for this on the internet, so sorry if that topic was discussed already before. I have a named filter and I like to call this filter from another filter. filter peers_v4_in() { ...... }; > filter special_peer_v4_in() { ......; peers_v4_in(); }; > protocol bgp myspecialpeer { > ipv4 { import special_peer_v4_in(); }; > }; Is this even possible? Thank you. Michael.

2 1

Unable to get Bird 2 to listen on the BGP socket
by Skyler Mäntysaari 23 Nov '20

23 Nov '20

Hi there, I’m fairly certain that my issue is something rather small, or stupid but I’m unable to get Bird to listen on the 179 port. The other side has direct in the protocol definition which is correct, and I tried this side with that as well but it only says “as213021_hel_node: Waiting for fe80:10::1%wg1 to become my neighbor” so I don’t really have more clues. I did try to do strace and look for bind events, but couldn’t find any except for the control socket. My config can be seen here: https://dpaste.com/6P6NAUPNX <https://dpaste.com/6P6NAUPNX> & peer config: https://dpaste.com/HJ3SNL8RH <https://dpaste.com/HJ3SNL8RH>

10 20

BIRD crashing when --enable-debug is set
by andreas＠rammhold.de 23 Nov '20

23 Nov '20

Hi, I've been playing with the BIRD source code and implementing a new extension to Babel but that is not what this mail is about. While doing that I noticed passing --enable-debug to configure causes BIRD to crash regardless of configuration: > $ bt > #0 0x00007ffff7d9741a in raise () from /nix/store/2wrfwfdpklhaqhjxgq6yd257cagdxgph-glibc-2.32/lib/libc.so.6 > #1 0x00007ffff7d81523 in abort () from /nix/store/2wrfwfdpklhaqhjxgq6yd257cagdxgph-glibc-2.32/lib/libc.so.6 > #2 0x00000000004912c7 in bug (msg=msg@entry=0x4a06d8 "Assertion '%s' failed at %s:%d") at sysdep/unix/log.c:281 > #3 0x000000000049175f in add_tail (n=0x4e4880 <lc_syslog>, l=0x4e8060 <log_list>) at ./lib/lists.c:79 > #4 default_log_list (syslog_name=<synthetic pointer>, initial=0x0) at sysdep/unix/log.c:345 > #5 log_switch (initial=initial@entry=0x0, logs=logs@entry=0x4eecb0, new_syslog_name=0x0) at sysdep/unix/log.c:373 > #6 0x000000000049216b in sysdep_commit (new=new@entry=0x4eec70, old=<optimized out>) at sysdep/unix/main.c:188 > #7 0x0000000000420a1a in config_do_commit (c=c@entry=0x4eec70, type=type@entry=0x1) at conf/conf.c:265 > #8 0x0000000000420c29 in config_commit (c=c@entry=0x4eec70, type=type@entry=0x1, timeout=timeout@entry=0x0) at conf/conf.c:361 > #9 0x0000000000492f3a in main (argc=<optimized out>, argv=<optimized out>) at sysdep/unix/main.c:912 > #10 0x00007ffff7d82dbd in __libc_start_main () from /nix/store/2wrfwfdpklhaqhjxgq6yd257cagdxgph-glibc-2.32/lib/libc.so.6 > #11 0x000000000041087a in _start () at ../sysdeps/x86_64/start.S:120 The statement triggering this is the check for n->prev == NULL in lib/lists.c:79: 75 │ LIST_INLINE void 76 │ add_tail(list *l, node *n) 77 │ { 78 │ EXPENSIVE_CHECK(check_list(l, NULL)); -> 79 │ ASSUME(n->prev == NULL); 80 │ ASSUME(n->next == NULL); 81 │ 82 │ node *z = l->tail; 83 │ 84 │ n->next = &l->tail_node; 85 │ n->prev = z; 86 │ z->next = n; 87 │ l->tail = n; 88 │ } Note: Expensive checks are disabled. To reproduce build BIRD on the current master (3347aaf as of this writing) pass --enable-debug to the configure script. Create a simple configuration file that contains at least one protocol. I used just "protocol device {}" as the whole of the configuration file. Then execute "./bird -c minimal.conf -s test.ctl -f" and observe it crash. Bisecting the issue from v2.0.7 to the latest master branch showed that this seems to have been broken / uncovered in baac700: > commit baac7009063d94799821422ecc63ea2af41561ea (refs/bisect/bad) > Author: Maria Matejka <mq(a)ucw.cz> > Date: Wed Aug 14 16:23:58 2019 +0200 > > List expensive check. (https://gitlab.nic.cz/labs/bird/-/commit/baac7009063d94799821422ecc63ea2af4…) Looking at the backtrace we can see that `default_log_list` is being called just before the check is being made: sysdep/unix/log.c 341 │ #ifdef HAVE_SYSLOG_H 342 │ if (!dbgf) 343 │ { 344 │ static struct log_config lc_syslog = { .mask = ~0 }; -> 345 │ add_tail(&log_list, &lc_syslog.n); 346 │ *syslog_name = bird_name; 347 │ } 348 │ #endif This code path is being called twice during the startup. One of the call chains is the one visible in the backtrace (which happens after/during configuration commit) and the other happens very early in the process startup: sysdep/unix/main.c 843 │ int 844 │ main(int argc, char **argv) 845 │ { … 851 │ parse_args(argc, argv); -> 852 │ log_switch(1, NULL, NULL); … 912 │ config_commit(conf, RECONFIG_HARD, 0); // 2nd call, see bt Since the log_config struct is static (sysdep/unix/log.c:344) the same entry is being added twice to the log_list and thus at the 2nd attempt of adding it the prev field is no longer the expected NULL value and thus triggering the shown abort. The log_list itself is also static (in default_log_list) but initialized on every execution of the function. Probably purposely. This still leaves for a brief moment where structures are inconsistent. I am not yet familiar enough with the BIRD code base to reason about where threading is use and if this is an actual issue or not. Now the question is where the fix should happen? We could add code to the log_close logic that resets the node pointers of each of the entries? Another option could be in the default_log_list function by just reassigning the value of each fo the log_config structs on every single call to the function. As the log_list is also initialized on every call this is perhaps the simplest but also not cleanest way to deal with this? The initially described issue also occurs when not setting --enable-debug it just goes silently unnoticed. I went ahead and applied the below patch to make all ASSUME invocations log (in a normal build) to see how many more of those messages are going unnoticed. ------< snip >------ diff --git a/lib/birdlib.h b/lib/birdlib.h index 23036c1b..76848e2c 100644 --- a/lib/birdlib.h +++ b/lib/birdlib.h @@ -176,7 +176,7 @@ void debug(const char *msg, ...); /* Printf to debug output */ #endif #else #define ASSERT(x) do { if (!(x)) log(L_BUG "Assertion '%s' failed at %s:%d", #x, __FILE__, __LINE__); } while(0) -#define ASSUME(x) /* intentionally left blank */ +#define ASSUME(x) do { if (!(x)) log(L_BUG "Assertion '%s' failed at %s:%d\n", #x, __FILE__, __LINE__); } while(0) #endif ------< snip >------ On one of my machines with a few BGP sessions and Babel interfaces I instructed GDB to print a backtrace whenever the n->prev == NULL condition fails. Here the those few that I saw and could distinguish after a very brief look. > Breakpoint 1, add_tail (n=0x110f298, l=0x7fff7cb58b60) at ./lib/lists.c:80 > #0 add_tail (n=0x110f298, l=0x7fff7cb58b60) at ./lib/lists.c:80 > #1 babel_process_packet (pkt=<optimized out>, len=<optimized out>, saddr=..., ifa=<optimized out>) at proto/babel/packets.c:1393 > #2 0x000000000043e2aa in babel_rx_hook (sk=<optimized out>, len=<optimized out>) at proto/babel/packets.c:1472 > #3 0x000000000040a9d3 in sk_read (s=s@entry=0x110dc80, revents=<optimized out>) at sysdep/unix/io.c:1910 > #4 0x000000000040b394 in io_loop () at sysdep/unix/io.c:2342 > #5 0x000000000040b760 in main (argc=<optimized out>, argv=<optimized out>) at sysdep/unix/main.c:923 > Breakpoint 1, add_tail (n=0x110f6d0, l=0x1108848) at ./lib/lists.c:80 > #0 add_tail (n=0x110f6d0, l=0x1108848) at ./lib/lists.c:80 > #1 babel_enqueue (msg=<optimized out>, ifa=0x11087b0) at proto/babel/packets.c:1309 > #2 0x0000000000445143 in babel_send_seqno_request (p=p@entry=0x109bc20, e=e@entry=0x11050c8, sr=sr@entry=0x1271718) at proto/babel/babel.c:911 > #3 0x00000000004459c5 in babel_add_seqno_request (p=p@entry=0x109bc20, e=e@entry=0x11050c8, router_id=<optimized out>, seqno=<optimized out>, hop_count=hop_count@entry=0 '\000', > nbr=nbr@entry=0x0) at proto/babel/babel.c:337 > #4 0x0000000000445cf3 in babel_select_route (p=p@entry=0x109bc20, e=0x11050c8, mod=<optimized out>) at proto/babel/babel.c:775 > #5 0x0000000000445f13 in babel_update_cost (nbr=nbr@entry=0x1111710) at proto/babel/babel.c:611 > #6 0x000000000044626b in babel_expire_hello (p=p@entry=0x109bc20, nbr=nbr@entry=0x1111710, now_=now_@entry=242732001004) at proto/babel/babel.c:486 > #7 0x00000000004462ed in babel_expire_neighbors (p=p@entry=0x109bc20) at proto/babel/babel.c:506 > #8 0x00000000004473ae in babel_timer (t=<optimized out>) at proto/babel/babel.c:2072 > #9 0x000000000045da19 in timers_fire (loop=0x4db740 <main_timeloop>) at lib/timer.c:235 > #10 0x000000000040afe9 in io_loop () at sysdep/unix/io.c:2192 > #11 0x000000000040b760 in main (argc=<optimized out>, argv=<optimized out>) at sysdep/unix/main.c:923 > Breakpoint 1, add_tail (n=0x22c61a8, l=0x7ffe4c97c250) at ./lib/lists.c:80 > #0 add_tail (n=0x22c61a8, l=0x7ffe4c97c250) at ./lib/lists.c:80 > #1 babel_send_unicast (msg=0x7ffe4c97c2a0, ifa=0x22bfa20, dest=...) at proto/babel/packets.c:1283 > #2 0x00000000004451d9 in babel_send_route_request (p=<optimized out>, e=<optimized out>, n=0x22c8800) at proto/babel/babel.c:869 > #3 0x0000000000445233 in babel_refresh_route (p=p@entry=0x2252c20, r=r@entry=0x2423108) at proto/babel/babel.c:220 > #4 0x00000000004471c3 in babel_expire_routes_ (p=p@entry=0x2252c20, rtable=rtable@entry=0x2252d58) at proto/babel/babel.c:243 > #5 0x0000000000447388 in babel_expire_routes (p=p@entry=0x2252c20) at proto/babel/babel.c:293 > #6 0x00000000004473a6 in babel_timer (t=<optimized out>) at proto/babel/babel.c:2071 > #7 0x000000000045da19 in timers_fire (loop=0x4db740 <main_timeloop>) at lib/timer.c:235 > #8 0x000000000040afe9 in io_loop () at sysdep/unix/io.c:2192 > #9 0x000000000040b760 in main (argc=<optimized out>, argv=<optimized out>) at sysdep/unix/main.c:923 > Breakpoint 1, add_tail (n=0x234ac30, l=0x241f620) at ./lib/lists.c:82 > #0 add_tail (n=0x234ac30, l=0x241f620) at ./lib/lists.c:82 > #1 sl_alloc (s=0x21bd7d0) at lib/slab.c:264 > #2 0x000000000044f3e2 in rte_get_temp (a=0x22ffe18) at nest/rt-table.c:282 > #3 0x0000000000434220 in bgp_rte_update (s=s@entry=0x7ffe4c97c110, n=n@entry=0x7ffe4c97bf80, path_id=path_id@entry=0, a0=a0@entry=0x7ffe4c97bfd0) at proto/bgp/packets.c:1326 > #4 0x00000000004359db in bgp_decode_nlri_ip4 (s=0x7ffe4c97c110, pos=<optimized out>, len=<optimized out>, a=0x7ffe4c97bfd0) at proto/bgp/packets.c:1479 > #5 0x0000000000432f2b in bgp_decode_nlri (s=s@entry=0x7ffe4c97c110, afi=afi@entry=65537, nlri=<optimized out>, len=<optimized out>, ea=ea@entry=0x22fa4c0, nh=<optimized out>, > nh_len=<optimized out>) at proto/bgp/packets.c:2410 > #6 0x00000000004383a9 in bgp_rx_update (conn=conn@entry=0x2250728, pkt=pkt@entry=0x22c8a20 '\377' <repeats 16 times>, len=len@entry=227) at proto/bgp/packets.c:2505 > #7 0x0000000000438ef9 in bgp_rx_packet (conn=conn@entry=0x2250728, pkt=pkt@entry=0x22c8a20 '\377' <repeats 16 times>, len=227) at proto/bgp/packets.c:3097 > #8 0x0000000000438fb5 in bgp_rx (sk=0x22c88d0, size=<optimized out>) at proto/bgp/packets.c:3142 > #9 0x00000000004039a0 in call_rx_hook (s=s@entry=0x22c88d0, size=<optimized out>) at sysdep/unix/io.c:1796 > #10 0x000000000040a8dd in sk_read (s=s@entry=0x22c88d0, revents=1) at sysdep/unix/io.c:1884 > #11 0x000000000040b394 in io_loop () at sysdep/unix/io.c:2342 > #12 0x000000000040b760 in main (argc=<optimized out>, argv=<optimized out>) at sysdep/unix/main.c:923 Are these all false positives or should there be more cleanup code on these list entries? Sorry for the noise, Andi

4 8

[PATCH] babel: Initialise objects from slab completely
by Toke Høiland-Jørgensen 22 Nov '20

22 Nov '20

The babel protocol code was initialising objects returned from the slab allocator by assigning to each of the struct members individually, but wasn't touching the NODE member while doing so. This leads to warnings on debug builds since commit: baac7009063d ("List expensive check.") So let's change the babel code to make sure we also zero-initialise the list node. For the packet parser, just move the existing memset() out of babel_read_tlv() and in all other places, use struct assignment for initialising members, which is idiomatic for Bird. Signed-off-by: Toke Høiland-Jørgensen <toke(a)toke.dk> --- proto/babel/babel.c | 31 ++++++++++++++++++------------- proto/babel/packets.c | 7 ++++--- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/proto/babel/babel.c b/proto/babel/babel.c index 618abaa8a..588b29eaa 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -114,10 +114,12 @@ babel_get_source(struct babel_proto *p, struct babel_entry *e, u64 router_id) return s; s = sl_alloc(p->source_slab); - s->router_id = router_id; - s->expires = current_time() + BABEL_GARBAGE_INTERVAL; - s->seqno = 0; - s->metric = BABEL_INFINITY; + *s = (struct babel_source) { + .router_id = router_id, + .expires = current_time() + BABEL_GARBAGE_INTERVAL, + .seqno = 0, + .metric = BABEL_INFINITY, + }; add_tail(&e->sources, NODE s); return s; @@ -160,10 +162,10 @@ babel_get_route(struct babel_proto *p, struct babel_entry *e, struct babel_neigh return r; r = sl_alloc(p->route_slab); - memset(r, 0, sizeof(*r)); - - r->e = e; - r->neigh = nbr; + *r = (struct babel_route) { + .e = e, + .neigh = nbr + }; add_tail(&e->routes, NODE r); add_tail(&nbr->routes, NODE &r->neigh_route); @@ -326,11 +328,14 @@ babel_add_seqno_request(struct babel_proto *p, struct babel_entry *e, sr = sl_alloc(p->seqno_slab); found: - sr->router_id = router_id; - sr->seqno = seqno; - sr->hop_count = hop_count; - sr->count = 0; - sr->expires = current_time() + BABEL_SEQNO_REQUEST_EXPIRY; + *sr = (struct babel_seqno_request) { + .router_id = router_id, + .seqno = seqno, + .hop_count = hop_count, + .count = 0, + .expires = current_time() + BABEL_SEQNO_REQUEST_EXPIRY, + }; + babel_lock_neighbor(sr->nbr = nbr); add_tail(&e->requests, NODE sr); diff --git a/proto/babel/packets.c b/proto/babel/packets.c index d4ecf649d..98037ce69 100644 --- a/proto/babel/packets.c +++ b/proto/babel/packets.c @@ -1144,7 +1144,6 @@ babel_read_tlv(struct babel_tlv *hdr, return PARSE_ERROR; state->current_tlv_endpos = tlv_data[hdr->type].min_length; - memset(msg, 0, sizeof(*msg)); int res = tlv_data[hdr->type].read_tlv(hdr, msg, state); if (res != PARSE_SUCCESS) @@ -1278,7 +1277,7 @@ babel_send_unicast(union babel_msg *msg, struct babel_iface *ifa, ip_addr dest) struct babel_msg_node *msgn = sl_alloc(p->msg_slab); list queue; - msgn->msg = *msg; + *msgn = (struct babel_msg_node) { .msg = *msg }; init_list(&queue); add_tail(&queue, NODE msgn); babel_write_queue(ifa, &queue); @@ -1305,7 +1304,7 @@ babel_enqueue(union babel_msg *msg, struct babel_iface *ifa) { struct babel_proto *p = ifa->proto; struct babel_msg_node *msgn = sl_alloc(p->msg_slab); - msgn->msg = *msg; + *msgn = (struct babel_msg_node) { .msg = *msg }; add_tail(&ifa->msg_queue, NODE msgn); babel_kick_queue(ifa); } @@ -1387,6 +1386,8 @@ babel_process_packet(struct babel_pkt_header *pkt, int len, } msg = sl_alloc(p->msg_slab); + memset(msg, 0, sizeof(*msg)); + res = babel_read_tlv(tlv, &msg->msg, &state); if (res == PARSE_SUCCESS) { -- 2.29.2

2 1

handling of unreachable routes with gateway direct
by Alexander Zubkov 22 Nov '20

22 Nov '20

Hello, We have some routes that are passed via transit bird bgp-daemons without installing them to the kernel. And for some of these routes we need to keep their next hop attribute, because it is meaningful on the destination host, but may be unreachable on those transit hosts. If we have "gateway recursive" sessions with these transit hosts, then such routes are accepted, just marked unreachable, but we can work with them. And if we make the sessions "gateway direct", then such routes are ignored by the bird with "Invalid NEXT_HOP attribute" messages in log. Looks like it is caused by this piece of code in proto/bgp/packets.c in function bgp_apply_next_hop(): if (!nbr || (nbr->scope == SCOPE_HOST)) WITHDRAW(BAD_NEXT_HOP); It seems reasonable, of course, but it breaks things for our case. So I want to suggest some changes to make our setup possible. At the moment, I consider the possibility of adding something like "gateway <direct_with_recursive_fallback>" so that it would behave like "gateway direct", but in the case of a non-reachable neighbour, it would switch to recursive logic. What do you think? Whether this is a good idea or maybe you can suggest some better variants? We have a couple of reasons why we want "gateway direct": We want to avoid double recursion, which is not allowed - we might have some other recursive routes that use direct routes received in this session. We could make separate sessions for direct and transit routes, but I'm not sure that the whole thing would be simpler. And there is an issue with next-hop resolution when we have the same ips on different interfaces. As I understand, bird daemon currently does not support that case.

2 4