First working prototype - DHCPv4 #11

yoelcaspersen · 2021-10-24T13:06:15Z

This version works with kernel 5.15 and LLVM 13.

Improvements:

Interface name is added to Option 82 Circuit ID
On server replies, Option 82 is wiped completely
Char array parameters replaced with structs

Functions that previously used char arrays as function parameters now
use structs instead - otherwise the verifier cannot perform proper
boundary checks.

To-do:

IPv6 support
Multiple interface support

Built-in memcpy and memset functions cause verifier to complain about misaligned stack access. To get around that (and be able to copy a dynamic number of bytes via memcpy), two custom functions are added. This version of the relay uses interface name + VLAN identifiers in option 82 circuit ID (left-aligned, length IF_NAMESIZE with trailing null bytes). Signed-off-by: Yoel Caspersen <yoel@kviknet.dk>

When allowing packets with 1 VLAN tag only, verifier complains about BPF program being too large. Compiled with LLVM 13. Signed-off-by: Yoel Caspersen <yoel@kviknet.dk>

dhcp-relay/Makefile

dhcp-relay/dhcp-relay.h

dhcp-relay/dhcp_kern_xdp.c

In this version of the relay, we assume that client requests are received on a double tagged VLAN interface and server replies are received on a single tagged VLAN interface. Test scripts are added: - test.sh (configures BNG and sets L3 config) - cleanup_test.sh (removes BNG configuration) - trace.sh (opens trace pipe for debugging) The code for adding interface name is disabled because it makes the verifier complain about the BPF program being too large. To-do: - Fix verifier issue to include interface name in option 82 - For server replies, erase option 82 instead of (partial) overwrite Signed-off-by: Yoel Caspersen <yoel@kviknet.dk>

This version works with kernel 5.15 and LLVM 13. Improvements: - Interface name is added to Option 82 Circuit ID - On server replies, Option 82 is wiped completely - Char array parameters replaced with structs Functions that previously used char arrays as function parameters now use structs instead - otherwise the verifier cannot perform proper boundary checks. To-do: - IPv6 support - Multiple interface support Signed-off-by: Yoel Caspersen <yoel@kviknet.dk>

tohojo

Nice job getting something to work! Unfortunately the code has turned a wee bit spaghetti in the process; I guess most of the comments below could be summed up as changes that fix that :)

tohojo · 2021-11-12T21:48:23Z

dhcp-relay/dhcp-relay.h

@@ -38,6 +43,11 @@ struct dhcp_option_255 {
 __u8 t;
 };

+struct dev_name {
+ char name[IF_NAMESIZE];
+ __u8 len;


len is unneeded, see below

dhcp-relay/dhcp_kern_xdp.c

tohojo · 2021-11-12T21:52:11Z

dhcp-relay/dhcp_kern_xdp.c

+ }
+
+ if (i > 0) {
+ opt->val[--i] = '.';


The function is called u16_to_ascii, so what is this doing here? Better leave this to the caller.

tohojo · 2021-11-12T21:52:23Z

dhcp-relay/dhcp_kern_xdp.c

+ }
+
+ if (i > RAI_OPTION_LEN)
+ return -1;


This check is not needed...

tohojo · 2021-11-12T22:30:53Z

dhcp-relay/dhcp_kern_xdp.c

-}
+ /* Adjust IP offset for VLAN header when VLAN header has been added */
+ if (head_adjusted) {
+ ip = data + ip_offset + sizeof (struct vlan_hdr);


Why carry this head_adjusted all the way down here instead of just modifying ip_offset directly when doing the adjustment?

tohojo · 2021-11-12T22:31:08Z

dhcp-relay/dhcp_kern_xdp.c

 return XDP_ABORTED;
+ }


Unnecessary braces

tohojo · 2021-11-12T22:31:27Z

dhcp-relay/dhcp_kern_xdp.c

@@ -499,7 +685,6 @@ int xdp_dhcp_relay(struct xdp_md *ctx) {
 /* Re-calculate ip checksum */
 __u32 sum = calc_ip_csum(&oldip, ip, oldip.check);
 ip->check = ~sum;
- rc = XDP_PASS;

 goto out;


Redundant goto

tohojo · 2021-11-12T22:32:21Z

dhcp-relay/dhcp_kern_xdp.c

@@ -499,7 +685,6 @@ int xdp_dhcp_relay(struct xdp_md *ctx) {
 /* Re-calculate ip checksum */
 __u32 sum = calc_ip_csum(&oldip, ip, oldip.check);
 ip->check = ~sum;
- rc = XDP_PASS;


See above; this is actually the right place to change the return code back into something that sends out the packet, as this is where the packet is once again valid...

tohojo · 2021-11-12T22:34:26Z

dhcp-relay/test.sh

+OUTER_VLAN=83
+INNER_VLAN=20
+UPLINK_VLAN=84
+IF="ens6f0"


It should be possible to test this using network namespaces; having a test that is runnable without needing special hardware makes it way easier to test. Such a test should really be added so it can be run as part of a CI job.

This makes several changes to improve the readability of write_dhcp_option_82(): The main change is in reworking the string helpers to be smaller and more modular, and using a separate buffer for stringifying the VLAN tags before copying them to the DHCP option field. Get rid of str_len() and the len member of struct dev_name. To aid in this, the whole containing function is made global (i.e., a separate BPF subprog) instead of the helpers. In addition, the local variables are renamed to be more descriptive, and the option struct is initialised using struct initialisation syntax. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

The case statements of the option parsing 'switch' had grown an extra level of indentation. Get rid of that. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Add a -v option to turn on verbose logging in libbpf; this makes it possible to see which relocations libbpf is making, which is useful to see which functions get turned into separate sub functions. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

tohojo · 2021-11-12T23:21:00Z

Took the liberty of fixing some of the issues I pointed out myself, and pushed them to your branch :)

yoelcaspersen requested a review from tohojo October 24, 2021 13:06

yoelcaspersen force-pushed the main branch from b7d2d94 to 83b9795 Compare October 24, 2021 13:07

Upstream direction works with QinQ packets only

8d91b14

When allowing packets with 1 VLAN tag only, verifier complains about BPF program being too large. Compiled with LLVM 13. Signed-off-by: Yoel Caspersen <yoel@kviknet.dk>

yoelcaspersen force-pushed the main branch from fa17a0e to 8d91b14 Compare October 25, 2021 20:26

yoelcaspersen mentioned this pull request Oct 25, 2021

dhcp-relay: Handle server responses #9

Open

15 tasks

tohojo reviewed Oct 25, 2021

View reviewed changes

Yoel Caspersen added 2 commits November 5, 2021 14:37

yoelcaspersen changed the title ~~Adding custom memcpy and memset functions to pass verifier checks~~ First working prototype - DHCPv4 Nov 9, 2021

yoelcaspersen added this to the Working DHCP relay (IPv4) milestone Nov 11, 2021

tohojo requested changes Nov 12, 2021

View reviewed changes

tohojo added 3 commits November 13, 2021 00:15

dhcp_user_xdp: Fix indentation of switch cases

b970472

The case statements of the option parsing 'switch' had grown an extra level of indentation. Get rid of that. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First working prototype - DHCPv4 #11

First working prototype - DHCPv4 #11

yoelcaspersen commented Oct 24, 2021 •

edited

Loading

tohojo left a comment

tohojo Nov 12, 2021

tohojo Nov 12, 2021

tohojo Nov 12, 2021

tohojo Nov 12, 2021

tohojo Nov 12, 2021

tohojo Nov 12, 2021

tohojo Nov 12, 2021

tohojo Nov 12, 2021

tohojo commented Nov 12, 2021

First working prototype - DHCPv4 #11

Are you sure you want to change the base?

First working prototype - DHCPv4 #11

Conversation

yoelcaspersen commented Oct 24, 2021 • edited Loading

tohojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tohojo commented Nov 12, 2021

yoelcaspersen commented Oct 24, 2021 •

edited

Loading