Building a Simple character device but device driver file will not write or read

Question

I am trying to write a simple character device/LKM that reads, writes, and seeks. I have been having a lot of issues with this, but have been working on it/troubleshooting for weeks and have been unable to get it to work properly. Currently, my module makes properly and mounts and unmounts properly, but if I try to echo to the device driver file the terminal crashes, and when i try to read from it using cat it returns killed.

Steps for this module:

First, I make the module by running make -C /lib/modules/$(uname -r)/build M=$PWD modules

For my kernel, uname -r is 4.10.17newkernel

I mount the module using sudo insmod simple_char_driver.ko

If I run lsmod, the module is listed

If I run dmesg, the KERN_ALERT in my init function "This device is now open" triggers correctly.

Additionally, if I run sudo rmmod, that functions "This device is now closed" KERN_ALERT also triggers correctly.

The module also shows up correctly in cat /proc/devices

I created the device driver file in /dev using sudo mknod -m 777 /dev/simple_char_driver c 240 0

Before making this file, I made sure that the 240 major number was not already in use.

My device driver c file has the following code:

#include<linux/init.h>
#include<linux/module.h>
#include<linux/fs.h>
#include<linux/slab.h>
#include<asm/uaccess.h>

#define BUFFER_SIZE 1024

MODULE_LICENSE("GPL");
//minor nunmber 0;
static int place_in_buffer = 0;
static int end_of_buffer = 1024;
static int MAJOR_NUMBER = 240;
char* DEVICE_NAME = "simple_char_driver";
typedef struct{
    char* buf;
}buffer;

char *device_buffer;
static int closeCounter=0;
static int openCounter=0;

ssize_t simple_char_driver_read (struct file *pfile, char __user *buffer, size_t length, loff_t *offset){
    int bytesRead = 0;
    if (*offset >=BUFFER_SIZE){
        bytesRead = 0;
    }
    if (*offset + length > BUFFER_SIZE){
        length = BUFFER_SIZE - *offset;
    }
    printk(KERN_INFO "Reading from device\n");
    if (copy_to_user(buffer, device_buffer + *offset, length) != 0){
        return -EFAULT;
    }
    copy_to_user(buffer, device_buffer + *offset, length);
    *offset += length;
    printk(KERN_ALERT "Read: %s", buffer);
    printk(KERN_ALERT "%d bytes read\n", bytesRead);
    return 0;
}

ssize_t simple_char_driver_write (struct file *pfile, const char __user *buffer, size_t length, loff_t *offset){
    int nb_bytes_to_copy;
if (BUFFER_SIZE - 1 -*offset <= length)
{
    nb_bytes_to_copy= BUFFER_SIZE - 1 -*offset;
    printk("BUFFER_SIZE - 1 -*offset <= length");
}
else if (BUFFER_SIZE - 1 - *offset > length)
{
    nb_bytes_to_copy = length;
    printk("BUFFER_SIZE - 1 -*offset > length");
}
printk(KERN_INFO "Writing to device\n");
if (*offset + length > BUFFER_SIZE)
{
    printk("sorry, can't do that. ");
    return -1;
}
printk("about to copy from device");
copy_from_user(device_buffer + *offset, buffer, nb_bytes_to_copy);
device_buffer[*offset + nb_bytes_to_copy] = '\0';
*offset += nb_bytes_to_copy;
return nb_bytes_to_copy;
}


int simple_char_driver_open (struct inode *pinode, struct file *pfile)
{
    printk(KERN_ALERT"This device is now open");    
    openCounter++;
    printk(KERN_ALERT "This device has been opened this many times: %d\n", openCounter);    
    return 0;
}

int simple_char_driver_close (struct inode *pinode, struct file *pfile)
{
    printk(KERN_ALERT"This device is now closed");  
    closeCounter++;
    printk(KERN_ALERT "This device has been closed this many times: %d\n", closeCounter);   
    return 0;
}

loff_t simple_char_driver_seek (struct file *pfile, loff_t offset, int whence)
{
    printk(KERN_ALERT"We are now seeking!");
    switch(whence){
        case 0:{
            if(offset<= end_of_buffer && offset >0){
                place_in_buffer = offset;
                printk(KERN_ALERT" this is where we are in the buffer: %d\n", place_in_buffer);
                }
            else{
                printk(KERN_ALERT"ERROR you are attempting to go ouside the Buffer");
                }       
            break;//THIS IS SEEK_SET
        }

        case 1:{
            if(((place_in_buffer+offset)<= end_of_buffer)&&((place_in_buffer+offset)>0)){
                place_in_buffer = place_in_buffer+offset;
                printk(KERN_ALERT" this is where we are in the buffer: %d\n", place_in_buffer);
                }
                else{
                    printk(KERN_ALERT"ERROR you are attempting to go ouside the Buffer");
                    }   
                break;          

        }
        case 2:{//THIS IS SEEK END
            if((end_of_buffer-offset)>=0&& offset>0){
                place_in_buffer = end_of_buffer-offset;
                printk(KERN_ALERT" this is where we are in the buffer: %d\n", place_in_buffer);
                }
                else{
                    printk(KERN_ALERT"ERROR you are attempting to go ouside the Buffer");
                    }   
                break;

        }
        default:{

        }
    }
    printk(KERN_ALERT"I sought %d\n", whence);
    return place_in_buffer;
}

struct file_operations simple_char_driver_file_operations = {

    .owner   = THIS_MODULE,
        .read = simple_char_driver_read,
        .write = simple_char_driver_write,
        .open = simple_char_driver_open,
        .llseek = &simple_char_driver_seek,
        .release = simple_char_driver_close,
};

static int simple_char_driver_init(void)
{   
    printk(KERN_ALERT "inside %s function\n",__FUNCTION__);
    register_chrdev(MAJOR_NUMBER,DEVICE_NAME, &simple_char_driver_file_operations);
    device_buffer = kmalloc(BUFFER_SIZE, GFP_KERNEL);
    return 0;
}

static void simple_char_driver_exit(void)
{
    printk(KERN_ALERT "inside %s function\n",__FUNCTION__);
    unregister_chrdev(MAJOR_NUMBER, DEVICE_NAME);
    kfree(device_buffer);
}

module_init(simple_char_driver_init);
module_exit(simple_char_driver_exit);

As I said before, this file makes properly with no errors or warnings. However, currently if I try to echo to the device file

using: echo "hello world" >> /dev/simple_char_driver

The terminal I am using crashes

If I then reopen a terminal, and use: cat /dev/simple_char_driver

then the terminal returns killed.

I am completely lost as to what is going wrong, and I have been searching for a solution for a very long time without success. If anyone has any insight into what is going wrong, please let me know.

Edit: As a user below suggested, I removed all code from my read and write methods except for the printk and the return, to make sure the functions were being triggered. When I then used echo, dmesg showed that the write printk was triggered, and the device(which I had had open) closed. When I then tried to cat the device file, dmesg showed that the device reopened, the "ready from device" printk showed up succesfully, and then the device closed again. However, echo did not actually find anything to read from the device file, despite my having echoed "Hello world" into it immediately before.

edit

Final functioning read and write functions are as follows:

ssize_t simple_char_driver_read (struct file *pfile, char __user *buffer, size_t length, loff_t *offset)
{
     if (*offset > BUFFER_SIZE)
     {
        printk("offset is greater than buffer size");
        return 0;
     }
     if (*offset + length > BUFFER_SIZE)
     {
        length = BUFFER_SIZE - *offset;
     }
     if (copy_to_user(buffer, device_buffer + *offset, length) != 0)
    {
        return -EFAULT;
    }
    *offset += length;
    return length;


}
ssize_t simple_char_driver_write (struct file *pfile, const char __user *buffer, size_t length, loff_t *offset){
    /* *buffer is the userspace buffer where you are writing the data you want to be written in the device file*/
    /* length is the length of the userspace buffer*/
    /* current position of the opened file*/
    /* copy_from_user function: destination is device_buffer and source is the userspace buffer *buffer */
    int nb_bytes_to_copy;
    if (BUFFER_SIZE - 1 -*offset <= length)
    {
        nb_bytes_to_copy= BUFFER_SIZE - 1 -*offset;
        printk("BUFFER_SIZE - 1 -*offset <= length");
    }
    else if (BUFFER_SIZE - 1 - *offset > length)
    {
        nb_bytes_to_copy = length;
        printk("BUFFER_SIZE - 1 -*offset > length");
    }
    printk(KERN_INFO "Writing to device\n");
    if (*offset + length > BUFFER_SIZE)
    {
        printk("sorry, can't do that. ");
        return -1;
    }
    printk("about to copy from device");
    copy_from_user(device_buffer + *offset, buffer, nb_bytes_to_copy);
    device_buffer[*offset + nb_bytes_to_copy] = '\0';
    *offset += nb_bytes_to_copy;
    return nb_bytes_to_copy;
}

Answer 1

Your code in general leaves much to be desired, but what I can see at the moment is that your .write implementation might be dubious. There are two possible mistakes - the absence of buffer boundaries check and disregard of null-termination which may lead to undefined behaviour of strlen().

First of all, you know the size of your buffer - BUFFER_SIZE. Therefore, you should carry out a check that *offset + length < BUFFER_SIZE. It should be < and not <= because anyhow the last byte shall be reserved for null-termination. So, such a check shall make the method return immediately if no space is available (else branch or >=). I can't say for sure whether you should return 0 to report that nothing has been written or use a negative value to return an error code, say, -ENOBUFS or -ENOSPC. Anyhow, the return value of the method is ssize_t meaning that negative value may be returned.

Secondly, if your first check succeeds, your method shall calculate actual space available for writing. I.e., you can make use of MIN(A, B) macro to do this. In other words, you'd better create a variable, say, nb_bytes_to_copy and initialise it like nb_bytes_to_copy = MIN(BUFFER_SIZE - 1 - *offset, length) so that you can use it later in copy_from_user() call. If the user, say, requests to write 5 bytes of data starting at the offset of 1021 bytes, then your driver will allow to write only 2 bytes of the data - say, he instead of hello. Also, the return value shall be set to nb_bytes_to_copy so that the caller will be able to detect the buffer space shortage.

Finally, don't forget about null termination. As soon as you've done with

copy_from_user(device_buffer + *offset, buffer, nb_bytes_to_copy);

you shall pay attention to do something like

device_buffer[*offset + nb_bytes_copy] = '\0';

Alternatively, if I recall correctly, you may use a special function like strncopy_from_user() to make sure that the data is copied with an implicit null termination.

Also, although a null-terminated write shall not cause problems with subsequent strlen(), I doubt that you ever need it. You can simply do *offset += nb_bytes_to_copy.

By the way, I'd recommend to name the arguments/variables in a more descriptive way. *offset is an eyesore. It would look better if named *offsetp. If your method becomes huge, an average reader will unlikely remember that offset is a pointer and not a value. offsetp where p stands for "pointer" will ease the job of anyone who will support your code in future.

To put it together, I doubt your .write implementation and suggest that you rework it. If some other mistakes persist, you will need to debug them further. Adding debug printouts may come in handy, but please revisit the basic points first, such as null-termination and buffer boundary protection. To make my answer a little bit more useful for you, I furnish it with the link to the section 3.7 of "Linux Device Drivers 3" book which will shed light on the topic under discussion.