On Thu, May 12, 2022 at 02:12:13PM +0200, Toke Høiland-Jørgensen wrote:
The Babel seqno request code keeps track of which seqno requests are outstanding for a neighbour by putting them onto a per-neighbour list. When reusing a seqno request, it will try to remove this node, but if the seqno request in question was a multicast request with no neighbour attached this will result in a crash because it tries to remove a list node that wasn't added to any list.
Fix this by making the list remove conditional. Also add a check so that seqno requests are only reused if the neighbour also matches, allowing multiple outstanding requests for the same router ID.
Hi I wanted to merge the patch but i found some discrepancies in seqno request handling that are confusing: 1) RFC 6126/8966 describes that entries in the table of pending requests have "the neighbour, if any, on behalf of which we are forwarding this request". In contrast, the 'nbr' field in struct babel_seqno_request describes the neighbor to whom we send the request. 2) It seems that we do not keep the 'neighbor on behalf we are forwarding', not sure if that is important or not. 3) The stored neighbor is used just for resending requests when they timeout. 4) Your patch changes it to allow multiple pending requests to different neighbors, but why? Seems to me that we should send requests (for fixed entry and router id) to only one neighbor (the current best one). 5) When an iface is removed, neighbor is removed, which also leads to removal of all associated seqno requests. Is this correct approach? Shouldn't we instead resend pending requests to the new best neighbor for that entry+router_id? Seems to me that we could just eliminate the nbr pointer from the struct babel_seqno_request, remove associated bookkeeping (per-neighbor list and code handling it). When a seqno request timeouts and must be resent, we can just find the appropriate neighbor from route entries. This simplifies the code and also handles 5). Is this reasonable or are there any issues with this approach? -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."