Integrate ptr_at function in the crate?

[ThomasCartier]

Hi,

Is it reasonable to integrate

#[inline(always)] 
fn ptr_at<T>(ctx: &XdpContext, offset: usize) -> Result<*const T, ()> {

        let start = ctx.data();

        let end = ctx.data_end();

        let len = mem::size_of::<T>();

        if start + offset + len > end {

                return Err(());
        }

        Ok((start + offset) as *const T)
}

into the crate so that we don't have to write it ourselves? That's a tiny dev experience improvement (in a very good crate).
It's generic enough so it should not be an issue?

[tamird]

How will the user know what offset to pass?

[ThomasCartier]

You pass it as usual, the only difference being the library users don't need to create this function themselves. In the code we directly write let ethhdr: *const EthHdr = unsafe { ptr_at(&ctx, 0)? }; Right now, every dev has to implement this convenient function by copy pasting

[tamird]

My question is: how does the user know to pass 0? If the knowledge is static, then we should have better helpers than ptr_at.

[ThomasCartier]

Got it, I am currently following the examples in the book because I am learning how to use your crate and this detail sparked.

#[inline(always)]
unsafe fn ptr_at<T>(ctx: &XdpContext, offset: usize) -> Result<*const T, ()> {
    let (start, end) = (ctx.data(), ctx.data_end());
    let len = mem::size_of::<T>();

    if start + offset + len > end {
        return Err(());
    }

    let ptr = (start + offset) as *const T;
    Ok(unsafe { &*ptr })
}

// (2)
fn block_ip(address: u32) -> bool {
    unsafe { BLOCKLIST.get(&address).is_some() }
}

fn try_xdp_firewall(ctx: XdpContext) -> Result<u32, ()> {
    let ethhdr: *const EthHdr = unsafe { ptr_at(&ctx, 0)? }; <------- here
    match unsafe { (*ethhdr).ether_type() } {
        Ok(EtherType::Ipv4) => {}
        _ => return Ok(xdp_action::XDP_PASS),
    }

    let ipv4hdr: *const Ipv4Hdr = unsafe { ptr_at(&ctx, EthHdr::LEN)? };
    let source = u32::from_be_bytes(unsafe { (*ipv4hdr).src_addr });

    // (3)
    let action = if block_ip(source) {
        xdp_action::XDP_DROP
    } else {
        xdp_action::XDP_PASS
    };
    info!(&ctx, "SRC: {:i}, ACTION: {}", source, action);

    Ok(action)
}

it's hardcoded and I guess this is on purpose here. But if you have another way I've not discovered yet, I take it.

[tamird]

I don't, but this is the point I am making. ptr_at is not a good API.

[vadorovsky]

I think we should rather go away from ptr_at (it's bad, unsafe API) and - in general - from exposing raw pointer operation from the user.

One idea I have in mind is:

1. Adding XdpContext::as_bytes() and TcContext::as_bytes() which would return a slice of bytes (&[u8]) based on data and data_end pointers.
2. Implementing some kind of lightweight byte deserialization for types in network-types - e.g., EthHdr::from_bytes() that takes a byte slice and casts it to &EthHdr if the slice has a correct size and is correctly aligned.

I think point 1) is trivial. The point 2) is going to be a bit tricker and controversial.

My rough imagination of the end code is:

fn try_xdp_firewall(ctx: XdpContext) -> Result<u32, ()> {
    let packet_bytes = ctx.as_bytes()?; // throws an error if we can't create a correct slice out of context's `data` and `data_end`

    let eth_hdr = EthHdr::from_bytes(packet_bytes[offset..EthHdr::LEN])?; // throws an error if the slice size doesn't match of if a slice is misaligned
    let mut offset = 0;

    match ethhdr.ether_type() {
        Ok(EtherType::Ipv4) => {
            let ipv4hdr = Ipv4Hdr::from_bytes(packet_bytes[offset..offset + Ipv4Hdr::LEN])?;
            offset += Ipv4Hdr::LEN;

            let source_addr = ipv4hdr.src_addr();
            let source_port = ipv4hdr.proto {
                IpProto::Tcp => {
                    let tcphdr = TcpHdr::from_bytes(packet_bytes[offset..offset + TcpHdr::LEN])?;
                    tcphdr.src_port()
                }
                IpProto::Udp => {
                    let udphdr = UdpHdr::from_bytes(packet_bytes[offset..offset + UdpHdr::LEN])?;
                    udphdr.src_port()
                }
                _ => return Ok(xdp_action::XDP_PASS),
            };

            info!(&ctx, "SRC IP: {:i}, SRC PORT: {}", source_addr, source_port);
        }
        Ok(EtherType::Ipv6) => {
            [...]
        }
        _ => {},
    }

    Ok(xdp_action::XDP_PASS)
}

In this API, people would still use offsets to access parts of the packet, but at least the as_bytes() methods would ensure size and alignment and reduce the risk of UB. And the average XDP/TC code written in Aya would stop being a mess full of unsafe { (*foo).bar }.

Of course once such API is made, one could think of building one layer more and try to parse the full hierarchy of the packet at once. But I don't think we should try to even design this before:

• We have the more low-level, but still safe API ironed out.
• We support enough network protocols in network-types.

The question is how do we provide these from_bytes() methods. The options I see:

• Implementing by hand, where each implementation looks roughly like:

fn from_bytes(s: &[u8]) -> Result<Self, DeserializationError> {
    let expected = mem::size_of::<Self>();
    let len = s.len();
    if len != expected {
        return Err(DeserializationError::SizeMismatch { expected, len });
    }

    let align = mem::align_of::<Self>();
    let ptr = s.as_ptr() as usize;
    if ptr % align != 0 {
        return Err(DeserializationError::AlignmentMismatch { align, ptr });
    }

    // SAFETY: We ensured the size and alignment.
    Ok(unsafe { &*(s.as_ptr() as *const Self) })
}

• bytemuck
• wincode which is currently shaping out a zero-copy API: Add ZeroCopy marker and SchemaRead support anza-xyz/wincode#24