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

dhcp-relay: Handle server responses #9

Open
12 of 15 tasks
yoelcaspersen opened this issue Oct 10, 2021 · 10 comments
Open
12 of 15 tasks

dhcp-relay: Handle server responses #9

yoelcaspersen opened this issue Oct 10, 2021 · 10 comments

Comments

@yoelcaspersen
Copy link
Contributor

yoelcaspersen commented Oct 10, 2021

Initial checks:

  • Destination MAC != FF:FF:FF:FF:FF:FF
  • Destination IP = relay IP address
  • UDP destination port = 68
  • OP == 2
  • GIADDR == relay IP address
  • If hops > 16: Discard packet
  • If option 82 is not set, discard

If option 82 is set, do the following:

  • Rewrite destination MAC = client MAC (use CHADDR)
  • Rewrite source MAC (must be the MAC of the relay interface) (perhaps optional)
  • Set destination IP = broadcast address
  • Set source IP = GIADDR
  • Increment hops field
  • Set new VLAN headers
  • Re-calc IP checksum

Final action:

  • Return XDP_TX (bounce packet) - will only work if client VLANs are tagged on the same interface we use for receiving server responses

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

@sachintiptur
Copy link

@tohojo any task that I can pick it up?

@yoelcaspersen
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

yoelcaspersen commented Oct 25, 2021

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
Copy link

@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
Copy link
Contributor Author

@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
Copy link

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

@yoelcaspersen
Copy link
Contributor Author

@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
Copy link

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
Copy link

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

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

No branches or pull requests

2 participants