Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relieve length check for NeighAttributes #208

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Conversation

jsimonetti
Copy link
Owner

The attribute length has more context then previously presumed. Especially when using tunnels and other link types, the attributes can use different lengths then expected.

@SuperQ and @dswarbrick you have been previously involved in this part and are (afaik) using these parts in node_exporter.
Would this by ok by you or would this introduce more issues on your end?

This PR is made in lieu of prometheus/node_exporter#2849

@jsimonetti jsimonetti force-pushed the neigh_no_length_check branch from 19f322c to 7dbdd69 Compare December 28, 2023 17:41
The attribute length has more context then previously presumed.
Especially when using tunnels and other link types, the attributes
can use different lengths then expected.

Signed-off-by: Jeroen Simonetti <jeroen@simonetti.nl>
@jsimonetti jsimonetti force-pushed the neigh_no_length_check branch from 7dbdd69 to 7acbaad Compare December 28, 2023 17:41
@jsimonetti jsimonetti changed the title Releive length check for NeighAttributes Relieve length check for NeighAttributes Dec 28, 2023
@dswarbrick
Copy link
Contributor

@jsimonetti Can we isolate whether the problem is occurring in the decoding of NDA_DST or NDA_LLADDR attributes?

My gut feeling about #200 was that it treated the symptoms, not the cause. As I already mentioned in prometheus/node_exporter#2849, node_exporter should already correctly handle NDA_LLADDR responses which have State NUD_NOARP - provided that rtnetlink does not overzealously treat certain length responses as errors.

In other words, I don't think that there need to be explicit workarounds for zero-length responses, so long as there also aren't enforced ideas such as:

			if l != 6 && l != 8 && l != 20 {
				return errInvalidNeighMessageAttr
			}

I'm pretty sure that if rtnetlink simply decoded any length response and handed it to the consumer, with the understanding that the consumer will check for things such as address family and NUD_NOARP in the State flags, all will be well.

It would be nice if we could determine a simple test case to reproduce this. So far it seems to involve Docker, bridges and Wireguard. But I have a hunch that it should be reproducible with GRE or SIT tunnels, since point-to-point tunnels typically do not use ARP (since there is no need).

jsimonetti and others added 2 commits January 25, 2024 09:55
Signed-off-by: Jeroen Simonetti <jeroen@simonetti.nl>
@jsimonetti
Copy link
Owner Author

This latest version completely removes any length check for neighbor attributes. This places the burden of checking for correctness to the consumer of this library.

@jsimonetti jsimonetti merged commit 8f6dc23 into master Jan 25, 2024
15 checks passed
@jsimonetti jsimonetti deleted the neigh_no_length_check branch January 25, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants