Why this char driver always copies just one byte?

Question

I was fiddling around with a scull driver code, and in turn tried to make it print the whole characters entered, instead of one.That is:

# insmod memory.ko
# chmod 666 /dev/memory
$ echo -n abcdef >/dev/memory
$ cat /dev/memory

should print abcdef, but instead printing just f

For this i made modifications to the scull code as:

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

  /* Transfering data to user space */ 
  copy_to_user(buf,memory_buffer,5);

  /* Changing reading position as best suits */ 
  if (*f_pos == 0) { 
    *f_pos+=1; 
    return 1; 
  } else { 
    return 0; 
  }
}

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

  char *tmp;

  tmp=buf+count-1;
  copy_from_user(memory_buffer,tmp,5);
  return 1;
}

int memory_init(void) {
  int result;

  /* Registering device */
  result = register_chrdev(memory_major, "memory", &memory_fops);
  if (result < 0) {
    printk(
      "<1>memory: cannot obtain major number %d\n", memory_major);
    return result;
  }

  /* Allocating memory for the buffer */
  memory_buffer = kmalloc(5, GFP_KERNEL); 
  if (!memory_buffer) { 
    result = -ENOMEM;
    goto fail; 
  } 
  memset(memory_buffer, 0, 5);

  printk("<1>Inserting memory module\n"); 
  return 0;

  fail: 
    memory_exit(); 
    return result;
}

People who downvoted this, thinking it as too naive can infact point to the solution, and the problem i am facing — RajSanpui, Jun 11 '13 at 16:36
This very close to the question I answered yesterday: http://stackoverflow.com/questions/16983153/kernel-driver-reading-ok-from-user-space-but-writing-back-is-always-0 — Benjamin Leinweber, Jun 11 '13 at 18:40

score 6 · Accepted Answer · edited Jun 11 '13 at 21:14

6

The read is not correctly implemented. It should not write more then count bytes to user (he can have smaller buffer). More relevantly, it should return number of bytes written to user and should return 0 only in case of end of file. It can also return error code as negative number like -EIO in case of error.

You write 5 characters, but return 1, so cat thinks it is incomplete read, which read 1 byte. I tries to read again and gets 0, so it interprets it as end of file.

write has analogous problems, it should also check size of user buffer and return number of bytes.

This is more or less how I would implement read for fixed size buffer:

ssize_t memory_read(struct file *filp, char *buf, 
                    size_t count, loff_t *f_pos) { 
  ssize_t retval;

  if(down_interruptible(&sem)) // some mutex is probably necessary
    return -ERESTARTSYS;
  count = MIN(count, MEMORY_SIZE - *f_pos);
  if(count == 0) { // this if is probably not necessary
    retval = 0;
    goto out;
  }
  /* Transfering data to user space */ 
  if(copy_to_user(buf, memory_buffer + *f_pos, count)) {
    retval = -EFAULT;
    goto out;
  }
  *f_pos += count;
  retval = count;
out:
  up(&sem);
  return retval;
}

edited Jun 11 '13 at 21:14

This isn't my real name

4,869
3
17
30

answered Jun 11 '13 at 15:22

zch

14,931
2
41
49

I changed it to: `int bytes=copy_to_user(buf,memory_buffer,5); return bytes` in `memory_read` and same type of return in `memory_write` which hangs the program. – RajSanpui Jun 11 '13 at 15:34
Can you tell us, if you differ in the opinion of what i have stated. Do you mean some other way? – RajSanpui Jun 11 '13 at 15:45
@kingsmasher1 - it's not that simple. If `copy_to_user` return non-zero then you should return `-EFAULT`. In case of success you should return number of bytes transfered, which depends on your logic. If you will return 5 it can work in this case. – zch Jun 11 '13 at 15:48
Additionally, i guess `tmp=buf+count-1;` should be changed to `tmp=buf` right? – RajSanpui Jun 11 '13 at 15:53
No use of this: `n=copy_to_user(buf,memory_buffer,count);` `/* Changing reading position as best suits */` `if(n != 0)` `return -EFAULT;` `else` `return count;` – RajSanpui Jun 11 '13 at 16:01
And for write, i changed to (though no use): `tmp=buf;` `n=copy_from_user(memory_buffer,tmp,count);` `if(n != 0)` `return -EFAULT;` `else` `return count;` – RajSanpui Jun 11 '13 at 16:03
@kingsmasher1 Now you always return positive, so `cat` will read forever and forever, waiting for end of file. I added the example, maybe you will understand it better. – zch Jun 11 '13 at 16:09
Sorry, if i sound too dumb, but what is this `MEMORY_SIZE`? – RajSanpui Jun 11 '13 at 16:15
@kingsmasher1 - size of the buffer. – zch Jun 11 '13 at 16:17

Why this char driver always copies just one byte?

1 Answers1