4
\$\begingroup\$

I wrote a small LKM that opens the /proc/cpuinfo file, reads it line by line, and attempts to the cpu model name. If the function fails to extract the cpu model name, or for some reason it can't read the cpuinfo file, it should return "N/A".

While I am aware that I can also achieve this with a user-space application, I would like to do it within the LKM. I find string processing inside a LKM quiet challenging.

I appreciate any feedback. I am especially interested in the following aspects:

  • String-Processing: Can the extraction of the cpu model name string be simplified?
  • Edge cases: Are there any edge cases that aren't covered, that could lead to memory leaks or crashes?

Below is the LKM code, and a Makefile.

test.c

#include <linux/init.h>
#include <linux/module.h>
#include <linux/printk.h>
#include <linux/types.h>
#include <linux/path.h>
#include <linux/fs.h>

MODULE_LICENSE("GPL");
MODULE_AUTHOR("test");


static int lkm_init(void);
char *get_cpu_model_name(void);
static void lkm_exit(void);

static int lkm_init(void) {
    pr_info("Initializing\n");
    pr_info("cpu: %s\n", get_cpu_model_name());
    return 0;
}

char *get_cpu_model_name(void) {
    const char *filename = "/proc/cpuinfo";
    struct file *file = filp_open(filename, O_RDONLY, 0);
    if (IS_ERR(file)) {
        return "N/A";
    }

    // Alloc buffer
    char *buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
    if (!buffer) {
        filp_close(file, NULL);
        return "N/A";
    }

    ssize_t bytes_read;
    char *newline_pos;
    while ((bytes_read = kernel_read(file, buffer, PAGE_SIZE - 1, &file->f_pos)) > 0) {
        // Ensure buffer has null terminator at the end.
        buffer[bytes_read] = '\0';
        // Process each line
        char *line = buffer;
        while ((newline_pos = strchr(line, '\n')) != NULL) {
            *newline_pos = '\0';
            // Check if the line starts with "model name"
            if (strncmp(line, "model name", 10) == 0) {
                // Skip until ":"
                char *colon_pos = strchr(line, ':');
                if (colon_pos != NULL) {
                    // Skip ":" and ""
                    colon_pos += 2;
                    return colon_pos;
                }
            }
            line = newline_pos + 1;
        }
        memset(buffer, 0, PAGE_SIZE);
    }
    kfree(buffer);
    filp_close(file, NULL);
    return "N/A";
}

static void lkm_exit(void) {
    pr_info("Unloading\n");
}

module_init(lkm_init);
module_exit(lkm_exit);

Makefile

obj-m := test.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

Compile the LKM using make, and load it with sudo insmod test.ko. Observe the output in dmesg: sudo dmesg. Afterwards, unload the LKM with sudo rmmod test.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Everybody manually saving a makefile probably knows this, but: those indentations in the maefile must be tabs, not spaces. \$\endgroup\$
    – Davislor
    Commented Jun 18, 2023 at 3:46
  • \$\begingroup\$ @Davislor Yes, I am aware - I have the Makefile indentation with tabs on my end. I assume while formatting the Makefile it for this post, that got lost :) \$\endgroup\$
    – 766F6964
    Commented Jun 18, 2023 at 14:32
  • 1
    \$\begingroup\$ You did, of course. Or it would never have worked!That warning was for anyone else who tried to copy-and-paste. \$\endgroup\$
    – Davislor
    Commented Jun 18, 2023 at 16:30

2 Answers 2

4
\$\begingroup\$

get_cpu_model_name() should define a string constant for "N/A" which can be used by both it and its consumers.


There are multiple calls to filp_close() which should be handled using the goto out resource cleanup idiom.


It's not obvious to me that, on a multi-core machine, the text will always fit within PAGE_SIZE. Put another way, you could consider allocating a buffer that you're confident can hold more than a line of text, and then stream your way through the file with multiple reads. We might declare support for lines up to length 127, allocate three 128-byte buffers, and use the first two as read() bounce buffers while get_line() copies each line out to the third. This would force us to skip most of the rather long "flags:" line, or we could choose bigger constants if parsing that was of interest.

Alternatively, perhaps there is a code assumption that should be explicitly documented: "All occurrences of 'model name' are specified to have identical content, and the first occurrence is guaranteed to appear within PAGE_SIZE of the beginning."


            if (strncmp(line, "model name", 10) == 0) {

This is clear enough. But still, 10 is a magic number. A manifest string constant or even a simple macro would be an improvement and would DRY this up a bit.

Maybe the toolchain permits compiler const expressions?


                    // Skip ":" and ""

Code defect: some comments lie, including this one. Author's intent was " " rather than "".

It's not terrible that we don't verify a SPACE character at that position. But I wouldn't mind seeing that sanity check, for example by using strstr(). Also, we might verify the return value points at a non-SPACE character. Returning a truncated CPU name seems kind of OK, but returning a zero-length name would be bad. Come to think of it, maybe by bad luck the : colon appears at end of buffer, and you wrote a '\0' zero on top of the subsequent SPACE, leading to invalid return address.

                    return colon_pos;

This is correct code, but it's not terrific. That is, sometimes identifiers lie, as we see here. This was a beautiful, informative, accurate identifier when we assigned strchr() to it. Here, I recommend omitting the increment and simply doing

                    return colon_pos + 2;

which is perfectly accurate.


        memset(buffer, 0, PAGE_SIZE);

Beautiful, it resembles the 0xDEADBEEF convention. This is a nice touch, I like it. Maybe there is some style guide or another advice URL you'd like to cite for this in a // comment? It just seems like a kindness to other folks debugging their modules.

I do have a concern about it, though. The crypto community has a whole DSE (dead store elimination) literature about those mean old compilers optimizing out a buffer clear like this one, see e.g. sodium_memzero(). At least in user space, it is often the case that a compiler can prove that clearing a secret password buffer has "no effect", or that code trying to read the buffer would have "undefined behavior". Changing the compiler flags can mean the difference between zeroing or not zeroing. So we maybe want a second comment here about recommended flag settings where we observed via https://godbolt.org or similar that the zeroing actually happened.


In the Makefile I confess I don't know why M=$(PWD) is necessary, but I imagine it truly is.


Looks good, very clear. This code achieves its design goals.

I would be happy to delegate or accept maintenance tasks on this codebase.


EDIT

"on a multi-core machine, the text [might not] fit within PAGE_SIZE"

There is a spec of sorts for cpuinfo output text, such as being comprised of "x : y\n" lines.

There's no real size limit on how much output it could produce. When going from a uniprocessor to dual-, quad-, 8-core machines we see that its length increases, without any fixed bound.

It seems plausible to me, though not guaranteed, that uniprocessor output (first core output) would usually fit within PAGE_SIZE. I was looking to make that explicit, and also to make it clear that once we see "model name : y" we require that any subsequent mention must also say "y". If we saw e.g. "...i3-3220 CPU @ 3.30GHz" and subsequently saw "...i3-3220 CPU @ 1.00GHz" we would be sad.

bounce buffers

Arbitrary limits can often be designed out of an algorithm. So for example we might malloc(n) rather than declare char buf[80]. Or we might do stream processing, which I briefly described above.

Arbitrarily define MAX_LINE to be 127, large enough for consumer to parse any line it is interested in. Define BUFSIZE to be some larger power of 2 such as 128. Allocate three such buffers and ask that they be aligned on cache lines, such as kmalloc() offers. (Allocate a fourth buffer, and ignore part of it, if requesting aligned allocations is inconvenient.)

Consumer reads from result buffer and never from either private bounce buffer.

Define get_line() which issues reads, finds next newline, and strcpy's next line into the result buffer. It's a fixed-size result buffer so long lines get truncated.

Init with a BUFSIZE-byte read to fill the first bounce buffer, and set an end index to the amount read. Set current index to 0. Define a buf_num() helper that maps an index to 0 or 1.

Each request to get another line will use memchr to find next \n newline in the valid portion of current buffer. If not found, issue a BUFSIZE-byte read for the alternate buffer, updating end, and scan for newline again. Possibly it's not found and we set a NO_NEWLINE flag and set newline index to end of that buffer, corresponding to a very long line.

Now we perform two copies to the result buffer:

  • from current to either newline or end-of-current-buffer
  • if needed, from start-of-alternate-buffer to newline

Write a \0 at end of result buffer. Now a (possibly truncated) line is ready for the consumer.

If NO_NEWLINE is set, continue to read buffers and scan for newlines. Stop when you hit newline or EOF. That way we can maintain the invariant that upon entry current always points to the start of line that will be returned to consumer, as long as valid characters remain. On many processors it will take multiple reads to skip past the somewhat verbose "flags:" lines.

Update current to point just past the newline. Return the result buffer.

Can you elaborate a bit on the sanity check you mentioned?

I was just advocating that we replace strchr(line, ':') with the slightly more picky strstr(line, ": "). Do I believe the colon will always be followed by SPACE? Yes, I do, it's part of the cpuinfo spec. But I'd like to express that in the code.

I feel like I could mock this edge-case by manually creating a file to test this behavior?

Yes, you could definitely arrange for the OP target code to see a buffer which ends with ": " or with just ":", and arrange for a Red test to turn Green.

how do you feel about the function returning a const char* instead of a char*?

I missed that. Good catch!

Yes, definitely mark it const.

\$\endgroup\$
3
  • \$\begingroup\$ (1/2) Thanks for the feedback! Regarding your point "on a multi-core machine, the text [might not] fit within PAGE_SIZE" - can you elaborate a bit on that, maybe with an example? I don't fully understand your proposed solution with bounce buffers. Ideally I would like this function to be as resilient as possible, so it works on "most" systems. Also you mention: "... by bad luck the : colon appears at end of buffer ...". Good catch, that could happen. Can you elaborate a bit the sanity check you mentioned? \$\endgroup\$
    – 766F6964
    Commented Jun 18, 2023 at 15:08
  • \$\begingroup\$ (2/2) I feel like I could mock this edge-case by manually creating a file to test this behavior? Also, how do you feel about the function returning a const char* instead of a char*? \$\endgroup\$
    – 766F6964
    Commented Jun 18, 2023 at 15:08
  • 1
    \$\begingroup\$ I fleshed it out a bit more. \$\endgroup\$
    – J_H
    Commented Jun 18, 2023 at 16:44
0
\$\begingroup\$

Micro optimization

char *colon_pos = strchr(line, ':'); replaceable with char *colon_pos = strchr(line+10, ':'); as per prior code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.