Conversation
Implemented code for task10, where was realized characted device. Signen-off-by: Anton Kotsiubailo <antohakotsubailo@gmail.com>
| static void cleanup_buffer(char *buff) | ||
| { | ||
| kfree(buff); | ||
| buff = NULL; |
There was a problem hiding this comment.
This line have no effect, it just clears local pointer buff.
If you want to clear user passed pointer - then you need to pass pointer of the pointer to this function.
| if (volume_buffer == NULL) | ||
| return -ENOMEM; |
There was a problem hiding this comment.
Add proper error handling!
In case of error - need to free previously allocated resources in order to prevent potential memory leak.
| proc_file = proc_create(PROC_FILENAME_1, S_IFREG | 0444, proc_dir, | ||
| &volume_fops); | ||
| if (proc_file == NULL) | ||
| return -EFAULT; |
There was a problem hiding this comment.
Add proper error handling! In case of error - need to free previously allocated resources in order to prevent potential memory leak and system crash.
| proc_file2 = proc_create(PROC_FILENAME_2, S_IFREG | 0444, proc_dir, | ||
| &size_fops); | ||
| if (proc_file == NULL) | ||
| return -EFAULT; |
There was a problem hiding this comment.
Add proper error handling! In case of error - need to free previously allocated resources in order to prevent potential memory leak and system crash.
| { | ||
| if (proc_file2) { | ||
| remove_proc_entry(PROC_FILENAME_2, proc_dir); | ||
| proc_file = NULL; |
There was a problem hiding this comment.
Typo? Did you mean to clear proc_file2 variable here?
| static struct file_operations volume_fops = { | ||
| .read = volume_read, | ||
| }; | ||
|
|
||
| static struct file_operations size_fops = { | ||
| .read = size_read, | ||
| }; | ||
|
|
||
| static struct file_operations fops = { | ||
| .open = dev_open, | ||
| .read = dev_read, | ||
| .write = dev_write, | ||
| .release = dev_release, | ||
| }; |
There was a problem hiding this comment.
Checkpatch warning: these structs should be const!
| int err; | ||
| if (length > LENGTH_DEF) { |
|
|
||
| message = kmalloc(length * sizeof(char), GFP_KERNEL); | ||
| if (message == NULL) | ||
| return -ENOMEM; |
| return 0; | ||
| } | ||
|
|
||
| left = copy_to_user(buffer, volume_buffer, len); |
There was a problem hiding this comment.
This line contains mistake that lead to garbage in function output.
Please read documentation on sprintf and copy_to_user functions and then fix the mistake.
| return 0; | ||
| } | ||
|
|
||
| left = copy_to_user(buffer, size_buffer, len); |
There was a problem hiding this comment.
The same mistake as in volume_read.
| if (len > sizeof(message) - 1) | ||
| size_of_message = sizeof(message) - 1; |
There was a problem hiding this comment.
According to your code, message is a pointer.
What would be the value of sizeof(message) ?
| if (len > sizeof(message) - 1) | ||
| size_of_message = sizeof(message) - 1; | ||
|
|
||
| ret = copy_from_user(message, buffer, len); |
There was a problem hiding this comment.
As you use "len" here, it means that previous length-check has no effect.
Out of bound read is a security issue and potential system crash.
|
|
||
| proc_file2 = proc_create(PROC_FILENAME_2, S_IFREG | 0444, proc_dir, | ||
| &size_fops); | ||
| if (proc_file == NULL) |
There was a problem hiding this comment.
Typo? Did you mean to check proc_file2 here?
| class_destroy(charClass); | ||
| unregister_chrdev(majorNumber, DEVICE_NAME); | ||
| kfree(message); | ||
| cleanup_proc(); |
There was a problem hiding this comment.
Check this function, because it is the reason for error message in system log:
Anton-Soroka
added
Change requested
3 participants
Done task10