UofG-netlab / BPFabric

BPFabric implementations. Details about this work are available in the research paper "BPFabric: Data Plane Programmability for Software Defined Networks" published at ANCS 2017
https://netlab.dcs.gla.ac.uk
30 stars 13 forks source link

Error when adding a new HASH map #3

Closed lnx1288 closed 5 years ago

lnx1288 commented 5 years ago

Hi, I'm using the ewma.c example file as baseline and just added a few lines of code creating a map with the src_ip as key and a struct with a counter (I'm doing it like these because I intend to add more data into this struct in the future, is not gonna be a counter forever). However, when I run the code (it compiles without error) the Mininet switch running it gets disconnected from the controller and no IP count is made. Could you please help me with this? BTW, excellent work on BPFabric, quite an interesting platform!

Thanks in advance (code below)

include <linux/if_ether.h>

include "ebpf_switch.h"

include <netinet/ip.h>

define EWMA_DELTA 5 // in seconds

struct ewma_stats { uint64_t volume; uint64_t packets; uint64_t prediction; uint32_t lasttime; uint32_t count; };

struct sips { uint64_t sip_count; };

struct bpf_map_def SEC("maps") ewma = { .type = BPF_MAP_TYPE_ARRAY, .key_size = sizeof(unsigned int), .value_size = sizeof(struct ewma_stats), .max_entries = 24 };

struct bpf_map_def SEC("maps") srcips = { .type = BPF_MAP_TYPE_HASH, .key_size = sizeof(uint32_t), // src_ip is the key .value_size = sizeof(struct sips), .max_entries = 256, };

struct bpf_map_def SEC("maps") inports = { .type = BPF_MAP_TYPE_HASH, .key_size = 6, // MAC address is the key .value_size = sizeof(uint32_t), .max_entries = 256, };

uint64_t prog(struct packet pkt) { struct ewma_stats ewma_stat;

bpf_map_lookup_elem(&ewma, &pkt->metadata.in_port, &ewma_stat);

ewma_stat->volume += pkt->metadata.length;
ewma_stat->packets++;

if (pkt->metadata.sec - ewma_stat->lasttime > EWMA_DELTA) { // could use nsec to be more accurate
    // compute the new prediction, prediction = alpha * volume + (1.0-alpha)*prediction
    ewma_stat->prediction = (ewma_stat->volume + (ewma_stat->prediction << 3) - ewma_stat->prediction) >> 3;
    //bpf_notify(pkt->metadata.in_port, ewma_stat, sizeof(struct ewma_stats));

    ewma_stat->lasttime = pkt->metadata.sec;
    ewma_stat->packets = 0;
    ewma_stat->volume = 0;
    ewma_stat->count++;
}

// Check if the ethernet frame contains an ipv4 payload
if (pkt->eth.h_proto == 0x0008) {
    struct ip *ipv4 = (struct ip *)(((uint8_t *)&pkt->eth) + ETH_HLEN);    
    struct sips *sip;
    uint32_t *src_ip = &ipv4->ip_src.s_addr;    // source IP

    if (bpf_map_lookup_elem(&srcips, &src_ip, &sip) == 0) {
       sip->sip_count++;
    } else {
       sip->sip_count = 1;
       bpf_map_update_elem(&srcips, &src_ip, &sip, 0);
    }
    bpf_notify(13, sip, sizeof(struct sips));

}

// Learning switch behaviour
uint32_t *out_port;

// if the source is not a broadcast or multicast
if ((pkt->eth.h_source[0] & 1) == 0) {
    // Update the port associated with the packet
    bpf_map_update_elem(&inports, pkt->eth.h_source, &pkt->metadata.in_port, 0);
}

// Flood of the destination is broadcast or multicast
if ((pkt->eth.h_dest[0] & 1) == 1) {
    return FLOOD;
}

// Lookup the output port
if (bpf_map_lookup_elem(&inports, pkt->eth.h_dest, &out_port) == -1) {
    // If no entry was found flood
    return FLOOD;
}

return *out_port;

} char _license[] SEC("license") = "GPL";

simon-jouet commented 5 years ago

Hi @lnx1288

BTW, excellent work on BPFabric, quite an interesting platform!

Thanks for that, glad people are using it :)

Are you able to run the out-of-the-box EWMA example or it's crashing too ?

I might have missed something but just looking at the code it seems like you define sip as a struct sips * but then in the else branch you do sip->sip_count = 1; although sip was never initialized. If you do a dmesg do you get any segmentation error by any chance?

lnx1288 commented 5 years ago

Thanks for your quick reply! I've run the out-of-the-box EWMA example without issues, yes. As of your question, sip is created in the line struct sips *sip.

simon-jouet commented 5 years ago

sip is created in the line struct sips *sip.

but this is not creating sips it's just defining a new variable that's a pointer to a struct sips.

    if (bpf_map_lookup_elem(&srcips, &src_ip, &sip) == 0) {
       sip->sip_count++;
    } else {
       sip->sip_count = 1;
       bpf_map_update_elem(&srcips, &src_ip, &sip, 0);
    }
    bpf_notify(13, sip, sizeof(struct sips));

in here when you do sip->sip_count =1 sip is not set to anything so you will get a null pointer error.

in your else do something like below and it should work.

else {
  struct sips nsip;
  sip = &nsip;
  sip->sip_count = 1;
   bpf_map_update_elem(&srcips, &src_ip, &sip, 0);
lnx1288 commented 5 years ago

What you suggested worked perfectly, but the count isn't correct. I'm getting weird numbers and they repeat over time (e.g., 140724999679856, 140724999679881).

lnx1288 commented 5 years ago

I was mistakenly passing the address to the pointer instead of the pointer to the value when using bpf_map_update_elem.

simon-jouet commented 5 years ago

Hi @lnx1288

Great you got that working. All good now you are getting the notifications as you should?

lnx1288 commented 5 years ago

So far so good. Thanks @simon-jouet!

lnx1288 commented 5 years ago

@simon-jouet, I´m having a different issue now. Seems to be something trivial but I haven't been able to find the error. I'm still doing the counting but just want to know the IP. So I modified the struct accordingly and of course the cli.py script (the notify_event function). Despite having uint64_t and uint32_t inside the struct, for a total of 12 bytes to be sent in the notify message, the controller is giving me an error when unpacking saying that it has received 16 bytes.

Code added on the cli.py function notify_event :

if str(pkt.id) == '1': self.ewmaStruct = struct.Struct('QI') sip_count, src_ip = self.ewmaStruct.unpack(pkt.data) print ' --> sip_count: {}\n --> src_ip: {}\n'.format(sip_count, src_ip)

Script:

include <linux/if_ether.h>

include <netinet/ip.h>

include <netinet/tcp.h>

include "ebpf_switch.h"

struct sips { uint64_t sip_count; uint32_t src_ip; };

struct bpf_map_def SEC("maps") sipcount = { .type = BPF_MAP_TYPE_HASH, .key_size = sizeof(uint32_t), // src_ip is the key .value_size = sizeof(struct sips), .max_entries = 256, };

// learning switch map struct bpf_map_def SEC("maps") inports = { .type = BPF_MAP_TYPE_HASH, .key_size = 6, // MAC address is the key .value_size = sizeof(uint32_t), .max_entries = 256, };

uint64_t prog(struct packet pkt) {
// Check if the ethernet frame contains an ipv4 payload if (pkt->eth.h_proto == 0x0008) { struct ip
ipv4 = (struct ip )(((uint8_t )&pkt->eth) + ETH_HLEN);

    // Check if the ip packet contains a TCP payload
    if (ipv4->ip_p == 6) {
        struct tcphdr *tcp = (struct tcphdr *)(((uint32_t *)ipv4) + ipv4->ip_hl);

        if (ipv4->ip_src.s_addr < ipv4->ip_dst.s_addr) {                
            struct sips *sip;

            if (bpf_map_lookup_elem(&sipcount, &ipv4->ip_src.s_addr, &sip) == 0) {
                sip->sip_count++;
                sip->src_ip = ipv4->ip_src.s_addr;
            } else {
                struct sips nsip;
                sip = &nsip;
                sip->sip_count = 1;
                sip->src_ip = ipv4->ip_src.s_addr;
                bpf_map_update_elem(&sipcount, &ipv4->ip_src.s_addr, sip, 0);
            }

            bpf_notify(12, sip, sizeof(struct sips));
        }
    }    
}

// Learning switch behaviour
uint32_t *out_port;

// if the source is not a broadcast or multicast
if ((pkt->eth.h_source[0] & 1) == 0) {
    // Update the port associated with the packet
    bpf_map_update_elem(&inports, pkt->eth.h_source, &pkt->metadata.in_port, 0);
}

// Flood of the destination is broadcast or multicast
if ((pkt->eth.h_dest[0] & 1) == 1) {
    return FLOOD;
}

// Lookup the output port
if (bpf_map_lookup_elem(&inports, pkt->eth.h_dest, &out_port) == -1) {
    // If no entry was found flood
    return FLOOD;
}

return *out_port;

} char _license[] SEC("license") = "GPL";

simon-jouet commented 5 years ago

Hey,

My guess is the struct is getting aligned on 64 bits (because it's more efficient to have word aligned data).

So mark your struct as packed (so it won't be word aligned) or transform your uint32_t into a uint64_t or modify you python code to deal with the four extra bytes using padding x(https://docs.python.org/2/library/struct.html)

lnx1288 commented 5 years ago

@simon-jouet the explanation is useful, thanks. I had it already working using uint64_t but I had no idea why.

lnx1288 commented 5 years ago

Hey @simon-jouet, Do you know if there is a way to implement a timer in eBPF? I'm trying not to use the timestamps on the pakects to check if a time interval has finished.

simon-jouet commented 5 years ago

Hey,

No you can't, the ebpf program that you run is just a "pipeline" to process the packet it's not a process so you can't have anything that's not clocked by a incoming packet.

If you want to get some data from the maps at regular interval then don't use notify and just fetch the value from the controller at the interval you want. Check https://github.com/UofG-netlab/BPFabric/blob/master/controller/traffichist.py

lnx1288 commented 5 years ago

Thanks @simon-jouet!