Initial thoughts on a networking overhaul?

[kbknapp]

I've been working on the networking portion of redbpf-probes to address #78 (mutating packets) as well as abstracting over XdpContext and SkBuff in a cohesive manner (since one is directly mutable and the other is not) along with using the Rust type system to provide a safer codebase such as using mutable references instead of raw pointers where possible. I've got a proof of concept working nicely, and allows writing both XDP and socket code in the same manner, i.e.:

#[xdp]
pub fn packet_size(mut ctx: XdpContext) -> XdpResult {
    use L3Proto::*;
    use L4Proto::*;

    let eth = ctx.data().parse_as::<Ethernet<_>>()?;
    if let Ipv4(ip) = eth.parse()? {
        if let Tcp(mut tcp) = ip.parse()? {
            if tcp.dest() == 5081 {
                let payload_len = tcp.len();

                // Only modify packets of a specific size
                if payload_len < 290 || payload_len > 294 {
                   tcp.set_dest(5082);
                }
            }
        }
    }


    Ok(XdpAction::Pass)
}

All protocols and packet types end up looking like Ipv4<T> or Udp<T>, etc. Where T is some kind of context (such as SkBuff or XdpContext). Which makes implementing "mutable" methods (i.e. Ipv4::set_source) easy to determine because you just have a impl<T: Mutable> Ipv4<T> or the like. So when working with SkBuff all the getter type methods are implemented, but all the setter type methods are not.

---

This fork has quite a few breaking changes so could not be implemented until a v2.0 however while working on this fork, in the hopes of it one day getting accepted I have a few questions:

• In the xdp module, PerfMap::insert_with_flags requires a mutable reference to the XdpContext, however this is somewhat incompatible with mutating packets safely in the manner presented above because parsing a packet requires borrowing the underlying buffer. While the buffer is borrowed, handing out an additional mutable reference isn't safely doable. My question is: Is the mutable reference required for some reason, or just because the API says *mut T? There are workarounds, but if the fix is to just not require a mutable reference beause one isn't truely required, thats an easy fix. I'm not familiar enoguh with the map internals to say though.

Bike Sheddy type questions:

• The codebase as it stands uses less idiomatic error/result names NetworkError vs a more stdlib name like net::Error: since it wouldn't be until a v2 anyways, is this something Red Sift has strong feelings about one way or the other?

• In a similar manner, stdlib tends to prefer shorter names only with common abbreviations, i.e .Context->Ctx or Buffer->Buf, Network->Net, etc. Some of these combine, so in current codebase NetworkBuffer could be shorted to either NetBuf or net::Buf.

• In the current codebase terms like Transport are used for L4 protocols. In my fork I went with L{2,3,4}Proto since its easily distingishable. However if the folks at Red Sift have strong feelings about the names that can be changed. Other options are Frames, Packets, and Segments...but those terms are so often overloaded with eachother (even if incorrectly) I thought it would cause confusion.

---

Prior to me considering a PR I need to

• clean up a bunch items
• Finish documenting public items
• squash WIP commits into actual readable commits
• Figure out a way ahead for L3-in-L3, L4-in-L4, or even L3-in-L4 packets which doesn't play super nice with enum variants (I have a workaround right now, but I want to play with other ideas)
• Do a full write up of my fork so the authors of redbpf can easily follow the code and make a decision. This includes how everything fits together along with design decisions.

---

Not addressed in my fork:

I think a re-look at the map sitution could be warranted, perhaps with a redbpf-maps crate. It would be nice to be able to support both TC (i.e. iproute2) maps, as well as libbpf or perf maps since they are not compatible within Linux generally. However, since this library is simply abstracting over everything, there seems to be no reason redbpf couldn't allow one to target iproute2/TC maps as well as libbpf maps assuming its documented that you can't mix and match them in the same BPF program.

I only mention this because it's so common to require special maps in networking programs.

[rsdy]

#[xdp]
pub fn packet_size(mut ctx: XdpContext) -> XdpResult {
    use L3Proto::*;
    use L4Proto::*;

    let eth = ctx.data().parse_as::<Ethernet<_>>()?;
    if let Ipv4(ip) = eth.parse()? {
        if let Tcp(mut tcp) = ip.parse()? {
            if tcp.dest() == 5081 {
                let payload_len = tcp.len();

                // Only modify packets of a specific size
                if payload_len < 290 || payload_len > 294 {
                   tcp.set_dest(5082);
                }
            }
        }
    }


    Ok(XdpAction::Pass)
}

This looks great! Really happy that you could get something like this working.

This fork has quite a few breaking changes so could not be implemented until a v2.0

I think it's time we started a v2.0 window, and start breaking APIs. There have been a few ideas floating around for a while, and it feels like versioning is being a bottleneck for rolling out improvements, so let's do it.

My question is*: Is the mutable reference required for some reason, or just because the API says *mut T?

I don't think it's necessary to make it mutable for mutation, since it will leave the kernel space almost immediately after, and there's nothing in the underlying code that would require mutation to the buffer itself.

Bike Sheddy type questions:

* The codebase as it stands uses less idiomatic error/result names
  `NetworkError` vs a more stdlib name like `net::Error`: since it wouldn't be
  until a v2 anyways, is this something Red Sift has strong feelings about one
  way or the other?

net::Error certainly feels cleaner to me, and that's a good direction to go.

* In a similar manner, stdlib tends to prefer shorter names
  only with common abbreviations, i.e .`Context`->`Ctx` or `Buffer`->`Buf`,
  `Network`->`Net`, etc. Some of these combine, so in current codebase
  `NetworkBuffer` could be shorted to either `NetBuf` or `net::Buf`.

This is where I actually disagree with the standard. It leads to more pleasant code to read (strictly IMAO) when you don't need to figure out what abbreviations mean in context. Similarly, whenever I see a Context or Ctx, I have to cringe a little, because that literally has no intrinsic meaning whatsoever. I'm open to going the other way if there's disagreement on this, though.

* In the current codebase terms like `Transport` are used for L4 protocols. In my
  fork I went with `L{2,3,4}Proto` since its easily distingishable. However if
  the folks at Red Sift have strong feelings about the names that can be
  changed. Other options are Frames, Packets, and Segments...but those terms are
  so often overloaded with eachother (even if incorrectly) I thought it would
  cause confusion.

I have absolutely no preference. L{2,3,4} seems fine to me.

However, since this library is simply abstracting over everything, there seems to be no reason redbpf couldn't allow one to target iproute2/TC maps as well as libbpf maps assuming its documented that you can't mix and match them in the same BPF program.

I think it would be neat to abstract away the underlying details of maps. I think we might be able to come up with a solution where we set the map type for the compilation unit, but we'll need to investigate where to put this.

Thanks for this, it looks like a pretty cool way forward.

Please also note that the conventions of the project are less about Red Sift's agenda than about the agreement of significant contributors. We're looking at formalizing the process better, and some changes will come soon, but Red Sift merely sponsors my (admittedly limited nowadays) work on the library.

[kbknapp]

This is where I actually disagree with the standard. It leads to more pleasant code to read (strictly IMAO) when you don't need to figure out what abbreviations mean in context. Similarly, whenever I see a Context or Ctx, I have to cringe a little, because that literally has no intrinsic meaning whatsoever. I'm open to going the other way if there's disagreement on this, though.

I think when combined with point about using modules net::Error, using net::Buffer doesn't sound bad to me either. As for Context->Ctx that was just an example, my fork still uses the full XdpContext since I don't have strong feelings about that either way.

[imheresamir]

Just curious, has this work been merged? Looking forward to the nice API for mutating packets

[rsdy]

@imheresamir 2.0 should be tagged any day now, and I haven't seen further progress on this unfortunately. I personally have limited time to contribute to RedBPF since neither the project nor I am funded by Red Sift, but would be happy to help shape up a PR for merge if anybody's keen on working on this.

[djahandarie]

As a new user of the library I have to say the biggest issue I've faced is figuring out what the intended way to modify packets is while working with xdp and tc (especially the latter because bpf_skb_load_bytes is a bit frightening and there is no example using it). I hope this rework can continue at some point!