ioctl arg passing
Quoted 7 lines · Matt
struct instruction_t {
__s16 code;
__s16 rxlen;
__s16 *rxbuf;
__s16 txlen;
__s16 *txbuf;
};You should reorder fields, starting with largest fields and going down to smaller ones. That ways you'll not have troubles with alignment when someone decides to play with alignment rules...
Quoted 9 lines
struct
instruction_tlocal;__s16*temp;
copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) ); temp =kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );
local.rxbuf = temp;
temp =kmalloc( sizeof( __s16 ) * local.txlen, GFP_KERNEL );
...
As you are using signed value for rxlen/txlen, you should check for value < 0 ... And there is very low chance that kmalloc() for anything bigger than 4KB will succeed. You should either use vmalloc unconditionally, or at least as fallback. And some error checking (copy_from_user returns 0 if everything went OK) also makes driver safer.
Best regards,
Petr Vandrovec [email protected]
Quoted 3 lines
And there is very low chance that
kmalloc()for anything bigger than 4KB will succeed. You should either use vmalloc unconditionally, or at least as fallback.
The phrase 'very low chance' is inaccurate.
How do you think NFS works with -rsize, -wsize > 4096? kmalloc() uses get_free_pages() to allocate more than one physically contiguous memory page, which in turn uses a sort of modified buddy system to distribute them. And if you specify the right GFP_ flags, will page out, if necessary, to do so. (I know the hard way as this is the one substantial thing I fixed in the linux kernel just after 1.0 in about 95).
If you need physically (as opposed to virtially) contiguous memory, unless lots has changed since then, kmalloc() is the right call. However, you are correct that it draws on scarce resources.
--
Alex Bligh