Unable to create directories in /proc from a custom kernel module

Question

In the /proc directory a custom module should create two directories lkm and mem to get a hierarchy like /proc/lkm/mem.

A simple refactoring now broke what was working without obvious reason.

After the refactoring the module is not creating the hierarchy /proc/lkm/mem anymore. It only creates lkm and then stops. There're no messages in the Kernel Ring Buffer. I was wondering what did I break with that refactoring? I hope you can help me finding the issue.

Manual tests have been run with a fresh started VM and with usual insmod/rmmod and following versions:

• Kernel: 5.3.0 (x86_64)
• GCC: 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2)
• Distribution: Ubuntu 19.10, eoan

In the following you can see the original and the refactored code. At the bottom I put the a minimum source file and the Makefile. There is also a repository, an issue and the breaking commit. Of course I could revert that commit, but other refactorings are coming and I want to know the reason, before I break something similar.

Thanks in advance :)

Original code prior to refactoring

static int __init lkm_mem_init(void)
{

    lkm_proc_parent = proc_mkdir(LKM_PROC_PARENT, NULL);

    if (lkm_proc_parent == NULL) {
        printk(KERN_ERR "lkm_mem: Failed to create parent /proc/%s for lkm.\n",
        LKM_PROC_PARENT);

        return 1;
    }

    mem_proc_parent = proc_mkdir(LKM_MEM_PROC_PARENT, lkm_proc_parent);

    if (mem_proc_parent == NULL) {
        printk(KERN_ERR
        "lkm_mem: Failed to create parent /proc/%s/%s for mem.\n",
        LKM_PROC_PARENT, LKM_MEM_PROC_PARENT);

        return 1;
    }

    // ...
}

Refactored code

static int __init lkm_mem_init(void)
{

    lkm_proc_mkdir(lkm_proc_parent, LKM_PROC_PARENT, NULL);
    lkm_proc_mkdir(mem_proc_parent, LKM_MEM_PROC_PARENT, lkm_proc_parent);

    // ...
}

void lkm_proc_mkdir(struct proc_dir_entry *entry, const char *name,
            struct proc_dir_entry *parent)
{
    entry = proc_mkdir(name, parent);

    if (entry == NULL) {
        printk(KERN_ERR
               "lkm_mem: Failed to create parent %s in proc.\n",
               name);

        // todo: How to abort loading module in gentle way?
    }
}

// ...

Minimum source code

#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/module.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Thomas Piekarski");
MODULE_DESCRIPTION("Exposing separated memory and swap statistics to /proc");
MODULE_VERSION("0.1");

#define LKM_PROC_PERMISSION 0444
#define LKM_PROC_PARENT "lkm"
#define LKM_MEM_PROC_PARENT "mem"
#define LKM_MEM_PROC_TOTAL_ENTRY "total"
#define LKM_MEM_PROC_FREE_ENTRY "free"

struct sysinfo si;
struct proc_dir_entry *lkm_proc_parent;
struct proc_dir_entry *mem_proc_parent;
struct proc_dir_entry *mem_proc_total_entry;
struct proc_dir_entry *mem_proc_free_entry;

static int lkm_value_show(struct seq_file *seq, void *v)
{
    seq_put_decimal_ull(seq, "", *(unsigned long *)seq->private);
    seq_putc(seq, '\n');

    return 0;
}

void lkm_proc_create_single_data(struct proc_dir_entry *entry,
                 unsigned long *value, const char *name)
{
    entry = proc_create_single_data(name, LKM_PROC_PERMISSION,
                    mem_proc_parent, lkm_value_show, value);

    if (entry == NULL) {
        printk(KERN_ERR "lkm_mem: Failed to create /proc/%s/%s/%s.\n",
               LKM_PROC_PARENT, LKM_MEM_PROC_PARENT, name);
    }
}

void lkm_proc_mkdir(struct proc_dir_entry *entry, const char *name,
            struct proc_dir_entry *parent)
{
    entry = proc_mkdir(name, parent);

    if (entry == NULL) {
        printk(KERN_ERR
               "lkm_mem: Failed to create parent %s in proc.\n",
               name);
    }
}

void lkm_remove_proc_entry(struct proc_dir_entry *entry, const char *name,
               struct proc_dir_entry *parent)
{
    if (entry != NULL) {
        remove_proc_entry(name, parent);
    }
}

static int __init lkm_mem_init(void)
{
    lkm_proc_mkdir(lkm_proc_parent, LKM_PROC_PARENT, NULL);
    lkm_proc_mkdir(mem_proc_parent, LKM_MEM_PROC_PARENT, lkm_proc_parent);

    si_meminfo(&si);

    lkm_proc_create_single_data(mem_proc_total_entry, &si.totalram,
                    LKM_MEM_PROC_TOTAL_ENTRY);

    lkm_proc_create_single_data(mem_proc_free_entry, &si.freeram,
                    LKM_MEM_PROC_FREE_ENTRY);

    return 0;
}

static void __exit lkm_mem_exit(void)
{
    lkm_remove_proc_entry(mem_proc_total_entry, LKM_MEM_PROC_TOTAL_ENTRY,
                  mem_proc_parent);

    lkm_remove_proc_entry(mem_proc_free_entry, LKM_MEM_PROC_FREE_ENTRY,
                  mem_proc_parent);

    lkm_remove_proc_entry(mem_proc_parent, LKM_MEM_PROC_PARENT,
                  lkm_proc_parent);

    lkm_remove_proc_entry(lkm_proc_parent, LKM_PROC_PARENT, NULL);
}

module_init(lkm_mem_init);
module_exit(lkm_mem_exit);

Makefile

ccflags-y := -Wall

obj-m += lkm_device.o lkm_mem.o lkm_parameters.o lkm_proc.o lkm_sandbox.o lkm_skeleton.o

all:
    $(MAKE) -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules

clean:
    $(MAKE) -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean

Edits - Added URLs to repository, issue and commit

Answer 1

struct proc_dir_entry *lkm_proc_parent;
struct proc_dir_entry *mem_proc_parent;

static int __init lkm_mem_init(void)
{

lkm_proc_parent is NULL here...

    lkm_proc_mkdir(lkm_proc_parent, LKM_PROC_PARENT, NULL);

and it's still NULL here, since of course, function arguments in C are passed by value.

    lkm_proc_mkdir(mem_proc_parent, LKM_MEM_PROC_PARENT, lkm_proc_parent);

So this is calling proc_mkdir("mem", NULL). You'll probably notice that a /proc/mem directory has been created.

    // ...
}

Meanwhile:

void lkm_proc_mkdir(struct proc_dir_entry *entry, const char *name,
            struct proc_dir_entry *parent)
{
    entry = proc_mkdir(name, parent);

    if (entry == NULL) {
        printk(KERN_ERR
               "lkm_mem: Failed to create parent %s in proc.\n",
               name);
    }
}

The value returned by proc_mkdir is lost when this function returns, since you only assigned it to one of the function arguments, which of course is local to the function. Again, arguments are passed by value!

It seems like you probably want lkm_proc_mkdir to return a struct proc_dir *, rather than taking one as an argument. Or possibly you wanted the first argument to be a struct proc_dir **, but that seems to me like worse style.