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

Handle IPv4 options #571

Open
luqmana opened this issue Jul 25, 2024 · 1 comment · May be fixed by #585
Open

Handle IPv4 options #571

luqmana opened this issue Jul 25, 2024 · 1 comment · May be fixed by #585
Labels
feature Planned and requested features

Comments

@luqmana
Copy link
Contributor

luqmana commented Jul 25, 2024

Today we just swallow any IPv4 options we encounter and never emit any ourselves:

// TODO: actually capture and re-emit ipv4 options.
// before, they were accidentally *becoming* the ULP.
// now, we're at least skipping them.
let remaining_bytes = (hdr_len as usize) - Ipv4HdrRaw::SIZE;
rdr.seek(remaining_bytes)
.map_err(|_| Ipv4HdrError::HeaderTruncated { hdr_len })?;

Need to decide if we'll just let them through as-is or on a per-option basis. Will we need to rewrite the contents on ingress/egress?

@luqmana luqmana added the feature Planned and requested features label Jul 25, 2024
@FelixMcFelix
Copy link
Collaborator

It should be said that there is a bug in swallowing these options properly -- we need to adjust the IHL and total_length back down to baseline. As it stands, these are unchanged and so get fed back out when we generate a fresh IP header:

impl From<&Ipv4Meta> for Ipv4HdrRaw {
#[inline]
fn from(meta: &Ipv4Meta) -> Self {
Ipv4HdrRaw {
ver_hdr_len: 0x45,
dscp_ecn: 0x0,
total_len: meta.total_len.to_be_bytes(),
ident: meta.ident.to_be_bytes(),
frag_and_flags: [0x40, 0x0],
ttl: meta.ttl,
proto: u8::from(meta.proto),
csum: meta.csum,
src: meta.src.bytes(),
dst: meta.dst.bytes(),
}
}
}

The better fix is just to correctly re-emit them, as I promised we should eventually do...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Planned and requested features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants