CODEWITHSUNDEEP
clinux-kernellinux-device-driverredhat
Idan Banani
Idan Banani

Reputation: 141

Simple char device driver / module - Linux RedHat 8 (2.4.18) on VM segmentation fault after ./module_unload

edit: I fixed the code and turned it to a more compact code regarding memory allocations, everything works now . You might aware me if I'm doing something wrong

I'm not sure that the Write&Read implemantations are perfect....

#define ARRAY_LENGTH 128
#define MY_DEVICE "my_device" 
MODULE_LICENSE("GPL");  MODULE_AUTHOR("Anonymous");
/* globals */
  int my_major = 0; /* will hold the major # of my device driver */
  int g_index=0;    /*index of elements we will act on*/

typedef struct _my_array_elem{
    char* string;
    int size;
} my_array_elem;


    my_array_elem my_array [ARRAY_LENGTH] ;   //global array of strings 

int init_module(void)
{
    int i;
    //no need to malloc&free for this string?
    char* our_names = "333333333 \n222222222";

    my_major = register_chrdev(0,MY_DEVICE,&my_fops);
    if (my_major < 0) {
        printk(KERN_WARNING "can't get dynamic major\n");
        return my_major;
    }

    my_array[0].string=our_names;   
    my_array[0].size=strlen(our_names);

    for (i=1; i<ARRAY_LENGTH; i++)  {                       
        my_array[i].string=NULL;
        my_array[i].size=-1;
    }
   return 0;
}

void cleanup_module(void)
{
    int i;
    int ret = unregister_chrdev(my_major, MY_DEVICE);
    if (ret < 0){
        printk("Error in unregister_chrdev: %d\n", ret);
        }

        //CHECK!!: do I need to free the names string? (index 0)?
        for (i=1; i<ARRAY_LENGTH; i++){
            kfree(&my_array[i].string);
        }       
    return;
}



ssize_t my_read(struct file *filp,char *buf,size_t count,loff_t *f_pos)
   {    

    int bytes_read = count;
    if (g_index<0 || g_index>ARRAY_LENGTH-1) {
        return -EINVAL;   //illegal index   
        }

    if (my_array[g_index].size < count){
        bytes_read = my_array[g_index].size;
        }

    if (copy_to_user(buf, my_array[g_index].string, bytes_read)!=0){
        return -ENOMEM;     
        }

    return bytes_read;   
}

ssize_t my_write(struct file *filp, const char *buf, size_t count, loff_t *f_pos)
{

    if (g_index<1 || g_index>ARRAY_LENGTH-1){
        return -EINVAL;
        }   

    if ((my_array[g_index].size) != -1){
        kfree(&my_array[g_index].string); 
    }

    char* temp_string=kmalloc(count, GFP_KERNEL);
    if (temp_string == NULL){                         
            return -ENOMEM; //Out of memory
        }

    if (copy_from_user((void*)temp_string, buf, count)){                                                              
            kfree(temp_string);       
            return -ENOMEM; //Out of memory
        }

    my_array[g_index].string=temp_string;     
    my_array[g_index].size=count;       
    return count;
}

Upvotes: 2

Views: 585

Answers (1)

rodrigo
rodrigo

Reputation: 98338

On first read:

my_array_elem* temp_elem=kmalloc(sizeof(my_array_elem), GFP_KERNEL);         
....
copy_from_user((void*)temp_elem->string, buf, count)

You are copying the data from the user buf but the string address is not yet allocated.

You need something like:

temp_elem->string = kmalloc(count, GFP_KERNEL);

To be sincere, your handling of dynamic memory is a bit confusing... You should probably write a few functions that handle that instead of writing all the byte-mangling code in the read/write functions.

Upvotes: 1

Related Questions