zebra: zebra crash for zapi stream #18359

soumyar-roy · 2025-03-10T22:29:46Z

Issue:
If static route is created with a BGP route as nexthop, which recursively resolves over 512 ECMP v6 nexthops, zapi nexthop encode fails, as there is not enough memory allocated for stream. This causes assert/core dump in zebra. Right now we allocate fixed memory of ZEBRA_MAX_PACKET_SIZ size.

Fix:
1)Dynamically calculate required memory size for the stream 2)try to optimize memory usage

Testing:
No crash happens anymore with the fix
zebra: zebra crash for zapi stream

Issue:
If static route is created with a BGP route as nexthop, which recursively resolves over 512 ECMP v6 nexthops, zapi nexthop encode fails, as there is not enough memory allocated for stream. This causes assert/core dump in zebra. Right now we allocate fixed memory of ZEBRA_MAX_PACKET_SIZ size.

Fix:
1)Dynamically calculate required memory size for the stream 2)try to optimize memory usage

Testing:
No crash happens anymore with the fix
r1#
r1# sharp install routes 2100:cafe:: nexthop 2001:db8::1 1000 r1#

r2# conf
r2(config)# ipv6 route 2503:feca::100/128 2100:cafe::1 r2(config)# exit
r2#

mjstapp · 2025-03-11T15:45:37Z

lib/zclient.c

@@ -1086,6 +1086,93 @@ static void zapi_nexthop_group_sort(struct zapi_nexthop *nh_grp,
 	      &zapi_nexthop_cmp);
 }

+/* size needed by a stream for a zapi nexthop */
+size_t zapi_nh_encode_stream_size(const struct zapi_nexthop *api_nh)


yeah, no, it's not maintainable/safe to try to capture the sizes of our structs in code in this way.
can you take a look at the arithmetic approach that was recently added for the zapi_route struct, and see whether that or some adaptation of it could work here too?

That model is not directly applicable here. 1) In other model, they have bigger ref max size which is zebra route size(there they are encoding route, not nexthop), does msg_size = sizeof(struct zapi_route); then msg_size - nh size of unused struct zapi_nexthop, in my case, I dont have any reference to start except the max ZEBRA_MAX_PACKET_SIZ, size of zapi_nexthop == 320 bytes, for 520 it is 520x320 >150000, which is more than ZEBRA_MAX_PACKET_SIZ, so I can't do ZEBRA_MAX_PACKET_SIZ - no_of_unused_nhx320. (memory requirement in negative size) 2) Also for other model, it was just lucky that sizeof(struct zapi_route) ~= 320000, so that assuming sizeof(struct zapi_route) - no_of_unused_nh*320, will always work, but in my view this assumption itself can be wrong, considering there is variable part(pointer,array) in zapi_nexthop, i.e szieof(zapi_nexthop) may not reflect the actual size we need to encode.

hmm- I'll try to say what I meant in a different way.
the receiving side of this zapi message in lib/zclient uses struct zapi_route - and sizeof(struct zapi_route) is a good proxy for the maximum message size you need - isn't it? there are a couple of fields that aren't used in this specific path, but those are not large, mostly. the 'opaque' blob is the exception, and you can math that out? you'll be left with the fixed fields, and the embedded nexthops. you can also math out the right number of regular and backup nexthops. since those are used on the decode side, they're a good proxy for the space needed on the encode side?

donaldsharp · 2025-03-13T20:51:12Z

to go over the phone conversation I had:

a) adding a expandable ability to stream is desired. There should be a new stream_new_expandable() function( or one that is named better )
b) If a stream_putX is issued that uses a expandable stream, have the stream expand an appropriate amount
c) Existing users of the stream_putXXX functions who are not using a expandable stream should not be affected
d) The api should be the same for all intents and purposes and stream.c should do the handwaving in the background.

soumyar-roy · 2025-03-15T02:15:18Z

to go over the phone conversation I had:

a) adding a expandable ability to stream is desired. There should be a new stream_new_expandable() function( or one that is named better ) b) If a stream_putX is issued that uses a expandable stream, have the stream expand an appropriate amount c) Existing users of the stream_putXXX functions who are not using a expandable stream should not be affected d) The api should be the same for all intents and purposes and stream.c should do the handwaving in the background.

Done

mjstapp · 2025-03-17T13:23:55Z

lib/stream.h

@@ -115,6 +116,8 @@ struct stream_fifo {
 #define STREAM_SIZE(S)  ((S)->size)
 /* number of bytes which can still be written */
 #define STREAM_WRITEABLE(S) ((S)->size - (S)->endp)
+/* Extra size  needed for a stream in bytes, given new write size */
+#define EXPAND_SIZE(S, WSZ) ((WSZ)-STREAM_WRITEABLE(S))


is there a reason to expose this computation?
what will happen if someone checks a stream and there is enough space - if WRITEABLE is > the increment?
and ... if you really think you have to add a macro, please follow the existing pattern and prefix it with STREAM_...

agreed, no need to expose, let me change it.

moved in stream.c its mostly for internal usage in lib itself.

mjstapp · 2025-03-17T13:26:08Z

lib/stream.h

 extern void stream_free(struct stream *);
 /* Copy 'src' into 'dest', returns 'dest' */
 extern struct stream *stream_copy(struct stream *dest,
 				  const struct stream *src);
 extern struct stream *stream_dup(const struct stream *s);

-extern size_t stream_resize_inplace(struct stream **sptr, size_t newsize);
+extern size_t stream_resize_inplace(struct stream *sptr, size_t newsize);


why change the signature - doesn't that increase the impact of the changes?

** was need anymore, * is enough after we realloc only for *sptr->data, but true, this has bigger impact, let me work to change the signature to original one

reverted back to original signature

mjstapp · 2025-03-17T13:29:50Z

lib/stream.c

+	if (!s->allow_expansion)
+		return false;
+
+	if ((needed_size == 0) || (needed_size > MAX_STREAM_EXPANSION_SZ)) {


no, I think if the code needs the buffer to grow, it should grow. what's the point of failing to allocate 4K + 1, or 32K, etc?
the point, for me, of adding a bit of arithmetic to the code is to avoid repeated reallocs of a few octets, not to prevent code from allocating a relatively small initial buffer but discovering later that it needs a large one.

we can have different size for MAX_STREAM_EXPANSION_SZ, say it to zapi_route size or some other agreed value, but without having limit/check, this is library code, so anyone can send any request, that may be some very big size , say X due to some bug. Now say the system had just enough memory Y > X. so it will satisfy, so no issue so far, as no assert. But as Y-X too little, then subsequent all memory allocations can fail, where it may be very difficult find out, it was actually started by some buggy request before?. here at least caller can assert as memory request will fail, so it will pin point when/where that bad big memory request came from.

I think your concern is misplaced: we allow stream_new() to allocate a large buffer, don't we? this new behavior shouldn't impose any limitation that the existing api imposes.

Changed code.

mjstapp · 2025-03-17T20:08:29Z

please fix the python style errors?

soumyar-roy · 2025-03-18T15:26:10Z

please fix the python style errors?

Done

mjstapp · 2025-03-19T15:12:29Z

lib/stream.c

+static void stream_expand(struct stream *s, size_t expand_size)
+{
+	/* 
+     * Growth strategy:


this comment block looks mis-aligned - is that an artifact of the webui, or is it actually wrong?

I ran git clang-format on this file, added whatever got. Please check latest.

mjstapp · 2025-03-19T15:13:17Z

lib/stream.c

+     * For small expansions (<= min expansion bytes): grow by min size
+     * otherwise grow by needed size 
+     */
+	size_t actual_expand_size;


let's put the declaration at the start of the function, and maybe just init it to the default size? that will simplify the if clause

mjstapp · 2025-03-19T15:15:01Z

lib/stream.c

+	}
+
+	/* Calculate new total size */
+	size_t new_size = s->size + actual_expand_size;


again, variable at the start of the function

mjstapp · 2025-03-19T17:25:24Z

lib/stream.c

+	size_t actual_expand_size = expand_size;
+
+	/* Growth strategy:
+     * For small expansions (<= min expansion bytes): grow by min size


gosh, I don't know what to say about this: please indent things one level, with a tab. doesn't it look wrong in your editor?

sorry it didnt get added during push, now pushed it.

mjstapp · 2025-03-19T17:28:06Z

lib/zclient.h

@@ -1158,7 +1158,6 @@ zclient_send_rnh(struct zclient *zclient, int command, const struct prefix *p,
 		 vrf_id_t vrf_id);
 int zapi_nexthop_encode(struct stream *s, const struct zapi_nexthop *api_nh,
 			uint32_t api_flags, uint32_t api_message);
-extern int zapi_redistribute_stream_size(struct zapi_route *api);


oh man - I'm sorry to say I just registered what you've done here.
The purpose of this function was to compute a smaller stream buffer size, to avoid using a large buffer in a situation where that led to an excessive memory footprint. That's sort of the reverse of your case, where you want a really big buffer to satisfy your requirement in a different code path. I think you should leave this unchanged, and only change your own path at this time.

1)Main purpose is to allocate memory optimum way 2) zapi_redistribute_stream_size will still return 150K memory for say 512 nexthops, whereas for that we need only ~20k for next hop encoding, same thing for which we agreed to not do arithmetic calculation for nexthop encoding, and used new api for expand size 3) so I was suggested to fix this part too using new api for expand, ie revert back original other commit change 4) new expand api is not for to satisfy big buffer size, rather increase buffer amount as per need 5) if you think it's risky I can revert back , but I am just making sure about the behavior and expectation.

mjstapp · 2025-03-19T17:28:46Z

zebra/zapi_msg.c

@@ -610,8 +609,7 @@ int zsend_redistribute_route(int cmd, struct zserv *client, const struct route_n
 	SET_FLAG(api.message, ZAPI_MESSAGE_MTU);
 	api.mtu = re->mtu;

-	stream_size = zapi_redistribute_stream_size(&api);
-	struct stream *s = stream_new(stream_size);
+	struct stream *s = stream_new_expandable(ZEBRA_MAX_PACKET_SIZ);


this is what I referred to in another comment: we don't want to go back to the behavior that led to a problem in this code path.

Issue: Currently, during encode time, if required memory is more than available space in stream buffer, stream buffer can't be expanded. This fix introduces new apis to support stream buffer expansion. Testing: Tested with zebra nexthop encoding with 512 nexthops, which triggers this new code changes, it works fine. Without fix, for same trigger it asserts. Signed-off-by: Soumya Roy <souroy@nvidia.com>

Signed-off-by: Soumya Roy <souroy@nvidia.com>

Issue: If static route is created with a BGP route as nexthop, which recursively resolves over 512 ECMP v6 nexthops, zapi nexthop encode fails, as there is not enough memory allocated for stream. This causes assert/core dump in zebra. Right now we allocate fixed memory of ZEBRA_MAX_PACKET_SIZ size. Fix: 1)Dynamically calculate required memory size for the stream 2)try to optimize memory usage Testing: No crash happens anymore with the fix zebra: zebra crash for zapi stream Issue: If static route is created with a BGP route as nexthop, which recursively resolves over 512 ECMP v6 nexthops, zapi nexthop encode fails, as there is not enough memory allocated for stream. This causes assert/core dump in zebra. Right now we allocate fixed memory of ZEBRA_MAX_PACKET_SIZ size. Fix: 1)Dynamically calculate required memory size for the stream 2)try to optimize memory usage Testing: No crash happens anymore with the fix r1# r1# sharp install routes 2100:cafe:: nexthop 2001:db8::1 1000 r1# r2# conf r2(config)# ipv6 route 2503:feca::100/128 2100:cafe::1 r2(config)# exit r2# Signed-off-by: Soumya Roy <souroy@nvidia.com>

This commit undo 8c9b007 stream lib has been modified to expand the stream if needed Now for zapi route encode, we use expandable stream Signed-off-by: Soumya Roy <souroy@nvidia.com>

frrbot bot added the zebra label Mar 10, 2025

soumyar-roy marked this pull request as draft March 10, 2025 22:29

github-actions bot added size/L master labels Mar 10, 2025

frrbot bot added the tests Topotests, make check, etc label Mar 10, 2025

soumyar-roy force-pushed the soumya/streamsize branch from 7c7ee72 to b392bef Compare March 11, 2025 03:36

soumyar-roy marked this pull request as ready for review March 11, 2025 06:21

mjstapp reviewed Mar 11, 2025

View reviewed changes

soumyar-roy marked this pull request as draft March 12, 2025 21:08

soumyar-roy force-pushed the soumya/streamsize branch from b392bef to d1a9970 Compare March 13, 2025 19:44

frrbot bot added the libfrr label Mar 13, 2025

soumyar-roy force-pushed the soumya/streamsize branch 7 times, most recently from 65bf686 to 16b9835 Compare March 14, 2025 22:35

soumyar-roy marked this pull request as ready for review March 15, 2025 02:15

mjstapp reviewed Mar 17, 2025

View reviewed changes

soumyar-roy force-pushed the soumya/streamsize branch from 16b9835 to 79c28ca Compare March 17, 2025 18:17

github-actions bot added the rebase PR needs rebase label Mar 17, 2025

soumyar-roy force-pushed the soumya/streamsize branch 2 times, most recently from 8fb2c5c to 0a25d5a Compare March 18, 2025 01:13

mjstapp reviewed Mar 19, 2025

View reviewed changes

soumyar-roy force-pushed the soumya/streamsize branch from 0a25d5a to 1c217c0 Compare March 19, 2025 16:52

mjstapp reviewed Mar 19, 2025

View reviewed changes

soumyar-roy added 4 commits March 19, 2025 11:19

tests: Add staticd/ospfd/ospf6d/pimd for high ecmp

5258448

Signed-off-by: Soumya Roy <souroy@nvidia.com>

zebra: reduce memory usage by streams when redistributing routes

cb2b2fe

This commit undo 8c9b007 stream lib has been modified to expand the stream if needed Now for zapi route encode, we use expandable stream Signed-off-by: Soumya Roy <souroy@nvidia.com>

soumyar-roy force-pushed the soumya/streamsize branch from 1c217c0 to cb2b2fe Compare March 19, 2025 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zebra: zebra crash for zapi stream #18359

zebra: zebra crash for zapi stream #18359

soumyar-roy commented Mar 10, 2025

mjstapp Mar 11, 2025

soumyar-roy Mar 11, 2025 •

edited

Loading

mjstapp Mar 11, 2025

donaldsharp commented Mar 13, 2025

soumyar-roy commented Mar 15, 2025

mjstapp Mar 17, 2025

soumyar-roy Mar 17, 2025

soumyar-roy Mar 17, 2025

mjstapp Mar 17, 2025

soumyar-roy Mar 17, 2025 •

edited

Loading

soumyar-roy Mar 17, 2025

mjstapp Mar 17, 2025

soumyar-roy Mar 17, 2025 •

edited

Loading

mjstapp Mar 17, 2025

soumyar-roy Mar 18, 2025

mjstapp commented Mar 17, 2025

soumyar-roy commented Mar 18, 2025

mjstapp Mar 19, 2025

soumyar-roy Mar 19, 2025

mjstapp Mar 19, 2025

soumyar-roy Mar 19, 2025

mjstapp Mar 19, 2025

soumyar-roy Mar 19, 2025

mjstapp Mar 19, 2025

soumyar-roy Mar 19, 2025

mjstapp Mar 19, 2025

soumyar-roy Mar 19, 2025 •

edited

Loading

mjstapp Mar 19, 2025

zebra: zebra crash for zapi stream #18359

Are you sure you want to change the base?

zebra: zebra crash for zapi stream #18359

Conversation

soumyar-roy commented Mar 10, 2025

Choose a reason for hiding this comment

soumyar-roy Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldsharp commented Mar 13, 2025

soumyar-roy commented Mar 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyar-roy Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyar-roy Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjstapp commented Mar 17, 2025

soumyar-roy commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyar-roy Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyar-roy Mar 11, 2025 •

edited

Loading

soumyar-roy Mar 17, 2025 •

edited

Loading

soumyar-roy Mar 17, 2025 •

edited

Loading

soumyar-roy Mar 19, 2025 •

edited

Loading