xdp-project / BNG-router

BNG - Linux router project
GNU General Public License v2.0
20 stars 8 forks source link

dhcp-relay: Handle server responses #9

Open yoelcaspersen opened 2 years ago

yoelcaspersen commented 2 years ago

Initial checks:

If option 82 is set, do the following:

Final action:

TBD: Should we look at BOOTP flags (unicast vs. broadcast response)?

sachintiptur commented 2 years ago

@tohojo any task that I can pick it up?

yoelcaspersen commented 2 years ago

@tohojo any task that I can pick it up?

@sachints123 I just found out that the relay doesn't re-calculate the UDP checksum. If you can find an example of how to do that in XDP, it would be much appreciated - thanks in advance.

sachintiptur commented 2 years ago

@yoelcaspersen it can be done this way usingxdp helper function,

struct csum_offset csum = {};
struct udphdr oldudp;

oldudp = *udp;

.....
....
...
csum_l4_offset_and_flags(udp, &csum);
off = ((void *)udp - data) & 0x3fff;

if(csum_l4_replace(ctx, off, &csum, &oldudp, udp, BPF_F_PSEUDO_HDR) < 0)
    return -1;

Helper function is in bpf/include/bpf/ctx/xdp.h. I hope this helps.

yoelcaspersen commented 2 years ago

it can be done this way usingxdp helper function,

Thanks, @sachints123, but are you sure it works? It looks like csum_l4_offset_and_flags() and csum_l4_replace() are from a different project (Cilium) - and my compiler can't find bpf/include/bpf/ctx/xdp.h.

sachintiptur commented 2 years ago

@yoelcaspersen yes but we need to copy the implementation of helpers from cilium, like xdp_store_bytes().

Other way we can try is, calculate the csum as we are doing for IP in relay code and then replace the csum value in udp part. Because bpf helpers for l4 csum replace is present only for skb.

yoelcaspersen commented 2 years ago

@sachints123 thanks for your reply.

@netoptimizer suggested that we should just clear the checksum completely:

udp->check = 0;

I tried that, and it works - the packets are received by the DHCP server now.

Are there any good reasons to re-calculate the UDP checksum, or should we just rely on the checks in the underlying IP and ethernet layers?

sachintiptur commented 2 years ago

UDP checksum field for IPv4 is not mandatory, so it should be fine i think.

yoelcaspersen commented 2 years ago

@sachints123 I have made a pull request (#11).

It works for DHCP requests received as QinQ (double VLAN tags), buf if I disable the check:

    if (vlans.id[1] == 0) {
        bpf_printk("No inner VLAN tag set\n");
        goto out;
    }

to allow the XDP program to inspect single-tagged VLAN packets (server responses are singled tagged in my setup), the verifier complains about the program being too large.

Can you take a look at this and see if you can find the issue? I guess it has something to do with the memcpy_var / option 82 loops that the verifier can't work out.

I compile the program with LLVM 13 as the verifier should be better than in older releases.

Thanks in advance.

sachintiptur commented 2 years ago

I am not sure why the removal of this check causing verifier issuer, initially when this check was added, it was working even with out any vlan tags. We can still have check without goto statement and it works i feel.

sachintiptur commented 2 years ago

PS: @yoelcaspersen @tohojo fyi...my github username changed to @sachintiptur but the account remains same.