Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Christina Jacob]

On Wed, Oct 11, 2017 at 3:07 AM, David Daney  wrote:
> On 10/10/2017 10:19 AM, Stephen Hemminger wrote:
>>
>> On Tue, 10 Oct 2017 12:58:52 +0530
>> Christina Jacob  wrote:
>>
>>> +/* Get the mac address of the interface given interface name */
>>> +static long *getmac(char *iface)
>>> +{
>>> +   int fd;
>>> +   struct ifreq ifr;
>>> +   long *mac = NULL;
>>> +
>>> +   fd = socket(AF_INET, SOCK_DGRAM, 0);
>>> +   ifr.ifr_addr.sa_family = AF_INET;
>>> +   strncpy(ifr.ifr_name, iface, IFNAMSIZ - 1);
>>> +   ioctl(fd, SIOCGIFHWADDR, );
>>> +   mac = (long *)ifr.ifr_hwaddr.sa_data;
>>> +   close(fd);
>>> +   return mac;
>>
>>
>> Always check return value of ioctl.
>> You are assuming sizeof(long) > 6 bytes.
>> Also the byte order.
>
>
>
> Also:
>
> Returning the address of a local variable (ifr.ifr_hwaddr.sa_data), and then
> dereferencing it outside of the function is not correct.
>
> The casting of the char sa_data[] to a long * may cause alignment faults on
> some architectures.  The may also be endinaness issues depending on how the
> data are manipulated if you pack all those chars into a long.
>
> If we think that a MAC address is char[6], then it may be best to define the
> data structures as such and manipulate it as an array instead of trying to
> pack it into a long.

How do I feed the MAC address to xdp.data ? Is it ok to do a manual
leftshift + biwise and  for the purpose?

>
> Keep working on this though, this program will surely be useful.
>
> David Daney

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Christina Jacob]

On Wed, Oct 11, 2017 at 3:07 AM, David Daney  wrote:
> On 10/10/2017 10:19 AM, Stephen Hemminger wrote:
>>
>> On Tue, 10 Oct 2017 12:58:52 +0530
>> Christina Jacob  wrote:
>>
>>> +/* Get the mac address of the interface given interface name */
>>> +static long *getmac(char *iface)
>>> +{
>>> +   int fd;
>>> +   struct ifreq ifr;
>>> +   long *mac = NULL;
>>> +
>>> +   fd = socket(AF_INET, SOCK_DGRAM, 0);
>>> +   ifr.ifr_addr.sa_family = AF_INET;
>>> +   strncpy(ifr.ifr_name, iface, IFNAMSIZ - 1);
>>> +   ioctl(fd, SIOCGIFHWADDR, );
>>> +   mac = (long *)ifr.ifr_hwaddr.sa_data;
>>> +   close(fd);
>>> +   return mac;
>>
>>
>> Always check return value of ioctl.
>> You are assuming sizeof(long) > 6 bytes.
>> Also the byte order.
>
>
>
> Also:
>
> Returning the address of a local variable (ifr.ifr_hwaddr.sa_data), and then
> dereferencing it outside of the function is not correct.
>
> The casting of the char sa_data[] to a long * may cause alignment faults on
> some architectures.  The may also be endinaness issues depending on how the
> data are manipulated if you pack all those chars into a long.
>
> If we think that a MAC address is char[6], then it may be best to define the
> data structures as such and manipulate it as an array instead of trying to
> pack it into a long.

How do I feed the MAC address to xdp.data ? Is it ok to do a manual
leftshift + biwise and  for the purpose?

>
> Keep working on this though, this program will surely be useful.
>
> David Daney

RE: [PATCH v2] xdp: Sample xdp program implementing ip forward

[David Laight]

From: Jesper Dangaard Brouer
> Sent: 10 October 2017 20:06
...
> > +   int src_ip = 0, dest_ip = 0;
...
> > +   key4.b8[4] = dest_ip % 0x100;
> > +   key4.b8[5] = (dest_ip >> 8) % 0x100;
> > +   key4.b8[6] = (dest_ip >> 16) % 0x100;
> > +   key4.b8[7] = (dest_ip >> 24) % 0x100;

Do you really want signed remainders done here?

David

RE: [PATCH v2] xdp: Sample xdp program implementing ip forward

[David Laight]

From: Jesper Dangaard Brouer
> Sent: 10 October 2017 20:06
...
> > +   int src_ip = 0, dest_ip = 0;
...
> > +   key4.b8[4] = dest_ip % 0x100;
> > +   key4.b8[5] = (dest_ip >> 8) % 0x100;
> > +   key4.b8[6] = (dest_ip >> 16) % 0x100;
> > +   key4.b8[7] = (dest_ip >> 24) % 0x100;

Do you really want signed remainders done here?

David

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[David Daney]

On 10/10/2017 10:19 AM, Stephen Hemminger wrote:

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:


+/* Get the mac address of the interface given interface name */
+static long *getmac(char *iface)
+{
+   int fd;
+   struct ifreq ifr;
+   long *mac = NULL;
+
+   fd = socket(AF_INET, SOCK_DGRAM, 0);
+   ifr.ifr_addr.sa_family = AF_INET;
+   strncpy(ifr.ifr_name, iface, IFNAMSIZ - 1);
+   ioctl(fd, SIOCGIFHWADDR, );
+   mac = (long *)ifr.ifr_hwaddr.sa_data;
+   close(fd);
+   return mac;


Always check return value of ioctl.
You are assuming sizeof(long) > 6 bytes.
Also the byte order.



Also:

Returning the address of a local variable (ifr.ifr_hwaddr.sa_data), and 
then dereferencing it outside of the function is not correct.


The casting of the char sa_data[] to a long * may cause alignment faults 
on some architectures.  The may also be endinaness issues depending on 
how the data are manipulated if you pack all those chars into a long.


If we think that a MAC address is char[6], then it may be best to define 
the data structures as such and manipulate it as an array instead of 
trying to pack it into a long.


Keep working on this though, this program will surely be useful.

David Daney

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[David Daney]

On 10/10/2017 10:19 AM, Stephen Hemminger wrote:

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:


+/* Get the mac address of the interface given interface name */
+static long *getmac(char *iface)
+{
+   int fd;
+   struct ifreq ifr;
+   long *mac = NULL;
+
+   fd = socket(AF_INET, SOCK_DGRAM, 0);
+   ifr.ifr_addr.sa_family = AF_INET;
+   strncpy(ifr.ifr_name, iface, IFNAMSIZ - 1);
+   ioctl(fd, SIOCGIFHWADDR, );
+   mac = (long *)ifr.ifr_hwaddr.sa_data;
+   close(fd);
+   return mac;


Always check return value of ioctl.
You are assuming sizeof(long) > 6 bytes.
Also the byte order.



Also:

Returning the address of a local variable (ifr.ifr_hwaddr.sa_data), and 
then dereferencing it outside of the function is not correct.


The casting of the char sa_data[] to a long * may cause alignment faults 
on some architectures.  The may also be endinaness issues depending on 
how the data are manipulated if you pack all those chars into a long.


If we think that a MAC address is char[6], then it may be best to define 
the data structures as such and manipulate it as an array instead of 
trying to pack it into a long.


Keep working on this though, this program will surely be useful.

David Daney

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Jesper Dangaard Brouer]

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:

> +SEC("xdp3")
> +int xdp_prog3(struct xdp_md *ctx)
> +{
> + void *data_end = (void *)(long)ctx->data_end;
> + void *data = (void *)(long)ctx->data;
> + struct ethhdr *eth = data;
> + int rc = XDP_DROP, forward_to;
> + long *value;
> + struct trie_value *prefix_value;
> + long *dest_mac = NULL, *src_mac = NULL;
> + u16 h_proto;
> + u64 nh_off;
> + u32 ipproto;
> + union key_4 key4;

Reverse-xmas tree rule: Prefer ordering declarations longest to shortest.


[...]
> + if (h_proto == htons(ETH_P_ARP)) {
> + return XDP_PASS;
> + } else if (h_proto == htons(ETH_P_IP)) {
> + int src_ip = 0, dest_ip = 0;
> + struct direct_map *direct_entry;
> +
> + ipproto = parse_ipv4(data, nh_off, data_end, _ip, _ip);
> + direct_entry = (struct direct_map *)bpf_map_lookup_elem
> + (_match, _ip);
> + /*check for exact match, this would give a faster lookup*/
> + if (direct_entry && direct_entry->mac && direct_entry->arp.mac) 
> {
> + src_mac = _entry->mac;
> + dest_mac = _entry->arp.mac;
> + forward_to = direct_entry->ifindex;
> + } else {
> + /*Look up in the trie for lpm*/
> + key4.b32[0] = 32;
> + key4.b8[4] = dest_ip % 0x100;
> + key4.b8[5] = (dest_ip >> 8) % 0x100;
> + key4.b8[6] = (dest_ip >> 16) % 0x100;
> + key4.b8[7] = (dest_ip >> 24) % 0x100;
> + prefix_value = ((struct trie_value *)bpf_map_lookup_elem
> + (_map, ));
> + if (!prefix_value) {
> + return XDP_DROP;
> + } else {
> + src_mac = _value->value;
> + if (src_mac) {
> + dest_mac = (long *)bpf_map_lookup_elem
> + (_table, _ip);
> + if (!dest_mac) {
> + if (prefix_value->gw) {
> + dest_ip = *(__be32 
> *)_value->gw;
> + dest_mac = (long 
> *)bpf_map_lookup_elem(_table, _ip);
> + } else {
> + return XDP_DROP;
> + }
> + }
> + forward_to = prefix_value->ifindex;
> + } else {
> + return XDP_DROP;
> + }
> + }
> + }
> + } else {
> + ipproto = 0;
> + }

The nesting in this function is getting annoying to read.
Kernel code often use "early return" style coding to solve this.

A quick search turns up this guide, look at section "Early return"
 
https://en.wikibooks.org/wiki/Computer_Programming/Coding_Style/Minimize_nesting

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Jesper Dangaard Brouer]

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:

> +SEC("xdp3")
> +int xdp_prog3(struct xdp_md *ctx)
> +{
> + void *data_end = (void *)(long)ctx->data_end;
> + void *data = (void *)(long)ctx->data;
> + struct ethhdr *eth = data;
> + int rc = XDP_DROP, forward_to;
> + long *value;
> + struct trie_value *prefix_value;
> + long *dest_mac = NULL, *src_mac = NULL;
> + u16 h_proto;
> + u64 nh_off;
> + u32 ipproto;
> + union key_4 key4;

Reverse-xmas tree rule: Prefer ordering declarations longest to shortest.


[...]
> + if (h_proto == htons(ETH_P_ARP)) {
> + return XDP_PASS;
> + } else if (h_proto == htons(ETH_P_IP)) {
> + int src_ip = 0, dest_ip = 0;
> + struct direct_map *direct_entry;
> +
> + ipproto = parse_ipv4(data, nh_off, data_end, _ip, _ip);
> + direct_entry = (struct direct_map *)bpf_map_lookup_elem
> + (_match, _ip);
> + /*check for exact match, this would give a faster lookup*/
> + if (direct_entry && direct_entry->mac && direct_entry->arp.mac) 
> {
> + src_mac = _entry->mac;
> + dest_mac = _entry->arp.mac;
> + forward_to = direct_entry->ifindex;
> + } else {
> + /*Look up in the trie for lpm*/
> + key4.b32[0] = 32;
> + key4.b8[4] = dest_ip % 0x100;
> + key4.b8[5] = (dest_ip >> 8) % 0x100;
> + key4.b8[6] = (dest_ip >> 16) % 0x100;
> + key4.b8[7] = (dest_ip >> 24) % 0x100;
> + prefix_value = ((struct trie_value *)bpf_map_lookup_elem
> + (_map, ));
> + if (!prefix_value) {
> + return XDP_DROP;
> + } else {
> + src_mac = _value->value;
> + if (src_mac) {
> + dest_mac = (long *)bpf_map_lookup_elem
> + (_table, _ip);
> + if (!dest_mac) {
> + if (prefix_value->gw) {
> + dest_ip = *(__be32 
> *)_value->gw;
> + dest_mac = (long 
> *)bpf_map_lookup_elem(_table, _ip);
> + } else {
> + return XDP_DROP;
> + }
> + }
> + forward_to = prefix_value->ifindex;
> + } else {
> + return XDP_DROP;
> + }
> + }
> + }
> + } else {
> + ipproto = 0;
> + }

The nesting in this function is getting annoying to read.
Kernel code often use "early return" style coding to solve this.

A quick search turns up this guide, look at section "Early return"
 
https://en.wikibooks.org/wiki/Computer_Programming/Coding_Style/Minimize_nesting

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Stephen Hemminger]

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:

> + bzero(, sizeof(route));
> + bzero(dsts, sizeof(dsts));
> + bzero(dsts_len, sizeof(dsts_len));
> + bzero(gws, sizeof(gws));
> + bzero(ifs, sizeof(ifs));
> + bzero(, sizeof(route));

This is all unnecessary.
It looks like you write OpenBSD security code.
Only a security person would zero stack variables before return
and only BSD people use bzero(), Linux style is to use memset.

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Stephen Hemminger]

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:

> + bzero(, sizeof(route));
> + bzero(dsts, sizeof(dsts));
> + bzero(dsts_len, sizeof(dsts_len));
> + bzero(gws, sizeof(gws));
> + bzero(ifs, sizeof(ifs));
> + bzero(, sizeof(route));

This is all unnecessary.
It looks like you write OpenBSD security code.
Only a security person would zero stack variables before return
and only BSD people use bzero(), Linux style is to use memset.

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Stephen Hemminger]

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:

> +/* Get the mac address of the interface given interface name */
> +static long *getmac(char *iface)
> +{
> + int fd;
> + struct ifreq ifr;
> + long *mac = NULL;
> +
> + fd = socket(AF_INET, SOCK_DGRAM, 0);
> + ifr.ifr_addr.sa_family = AF_INET;
> + strncpy(ifr.ifr_name, iface, IFNAMSIZ - 1);
> + ioctl(fd, SIOCGIFHWADDR, );
> + mac = (long *)ifr.ifr_hwaddr.sa_data;
> + close(fd);
> + return mac;

Always check return value of ioctl.
You are assuming sizeof(long) > 6 bytes.
Also the byte order.

Re: [PATCH v2] xdp: Sample xdp program implementing ip forward

[Stephen Hemminger]

On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob  wrote:

> +/* Get the mac address of the interface given interface name */
> +static long *getmac(char *iface)
> +{
> + int fd;
> + struct ifreq ifr;
> + long *mac = NULL;
> +
> + fd = socket(AF_INET, SOCK_DGRAM, 0);
> + ifr.ifr_addr.sa_family = AF_INET;
> + strncpy(ifr.ifr_name, iface, IFNAMSIZ - 1);
> + ioctl(fd, SIOCGIFHWADDR, );
> + mac = (long *)ifr.ifr_hwaddr.sa_data;
> + close(fd);
> + return mac;

Always check return value of ioctl.
You are assuming sizeof(long) > 6 bytes.
Also the byte order.