C implementation of linked lists with unique list id's

Question

I need to implement a linked list in kernel code (not sure if it matters too much). the code should identify each linked list with a number between 0 and 255. and each node also has an id inside the list and some other data fields. I'm having some trouble making it work and not sure what has gone wrong I've written it very clearly but it seems to have segmentation issues. I would be happy to understand what went wrong here.

Note: I'm quite sure the problems aren't regarding the fact it is using kernel functions.

#undef __KERNEL__
#define __KERNEL__
#undef MODULE
#define MODULE
#include <linux/kernel.h>  
#include <linux/module.h>  
#include <linux/fs.h>       
#include <linux/uaccess.h>  
#include <linux/string.h>   

MODULE_LICENSE("GPL");

typedef struct Node { 
    long node_id;
    char data[256];
    int data_size;
    struct Node next;
};

typedef struct List { 
    int list_id;
    struct Node head;
};

/* array holds the Lists by their list_id number */
static struct List* lists = (List*) kmalloc(256 * sizeof(List), GFP_KERNEL);

static struct List* get_list(int list_id) {
    return lists[list_id];
}

static struct Node* get_node(struct List* list, long node_id) {
    struct Node* node = list->head;
    while (node != NULL) {
        if (node->node_id == node_id) {
            return node;
        }
        node = node->next;
    }
    return NULL;
}

static void add_node(struct List* list, long node_id) {
    struct Node* node;
    node = kmalloc(sizeof(Node), GFP_KERNEL);
    node->next = list->head;
    list->head = node;
}

static void add_list(int list_id) {
    struct List* list;
    list = kmalloc(sizeof(List), GFP_KERNEL);
    list->list_id = list_id;
    lists[list->list_id] = list;
}

static void free_list(struct List* list) {
    if (list == NULL) {
        return;
    }
    while (list->head != NULL) {
        struct Node* node = list->head;
        list->head = node->next;
        kfree(node);
    }
    kfree(list);
}

static void free_lists(void) {
    int list_id;
    for (list_id = 0 ; list_id < 256; list_id++) {
        free_list(lists[list_id]);
    }
}

Answer 1

struct Node next;

This line in your struct Node definition particularly creates non-terminating recursion in which struct Node requires another defined struct Node in order to be correctly defined, which requires another struct Node, and so on:

We use pointers because we can terminate the linked list with NULL, which means there are no more nodes to connect to:

I assume you were trying to implement your linked lists and nodes this way as you act on them like they are pointers in your functions, most notable with get_list.

Another thing worth mentioning about kmalloc:

kmalloc is the normal method of allocating memory for objects smaller than page size in the kernel.

Although I've read that kmalloc can allocate way more memory than this default page size in modern kernels, I don't know specifically what Linux version or environment you're working with. Making the assumptions below, you might want to reconsider your overall implementation of these data structures or at least your use of kmalloc:

1. Linux is using the default page size of 4kb
2. Your Node struct, using 8 byte pointers, 4 byte integers, 1 byte chars, and 8 byte long integers, takes up 276 bytes.
3. Your List struct, which adds another integer, takes up 280 bytes.
4. static struct List* lists allocates 256 times the size of your List struct, which rounds up to 72kb, which is WAY more than a page size.

For one, I'd suggest you have a list use a pointer to the head node instead of having the list store the head itself. I'd also suggest you make char data[256] a dynamic array with a max capacity of 256 so you can save some space as well. I'd also suggest you use an array of pointers to your lists (static struct List* lists[256] instead of static struct List* lists or static struct List lists[256]).