On Tue, 2007-06-26 at 19:06 +0200, Rodolfo Giometti wrote:
> On Tue, Jun 26, 2007 at 11:57:07AM +0100, David Woodhouse wrote:
> >
> > Your syscalls blindly dereference userspace pointers instead of using
> > copy_{to,from} user.
>
> I use access_ok() to test userspace addresses. It should be ok,
> shouldn't it?
No; it's racy. You must use copy_from_user() and copy_to_user().
> > Why did you split all your syscalls into two functions?
> >
> > s/__FUNCTION__/__func__/
>
> Just for an easy management of mutex locking.
That sounds like you're scared of using goto. Don't be :)
> > s/antennas/antennae/
>
> Done. However I found other files in the kernel code with the same
> error... ;)
This is often true of anything which gets pointed out during review. :)
> > You seem to have added debugging messages mentioning 'serial8250' into
> > serial_core.h
>
> Yes! Fixed.
>
> > You added <linux/pps.h> with #ifdef __KERNEL__ in it, but didn't export
> > it to userspace. Why?
>
> This file is called by timepps.h who exports the userland data.
I don't see this timepps.h of which you speak. If it's a _userspace_
file, it cannot include <linux/pps.h> unless you actually add
<linux/pps.h> to the list of files which are exported.
Run 'make headers_install' and observe that there is no file
usr/include/linux/pps.h -- so there would be no /usr/include/linux/pps.h
generated from your kernel tree. You need to add 'unifdef-y += pps.h' to
include/linux/Kbuild for that to happen.
> > Your structures for userspace communication look OK -- I don't think you
> > need special 32/64 compatibility for them. You do need it for the
> > 'struct timespec' in sys_time_pps_fetch() though.
>
> Mmm... can you please explain a bit what do you mean? Maybe just a
> link...
64-bit kernels can run 32-bit userspace programs. But some structures
come out _differently_ between 32-bit and 64-bit compilation, so the
system call needs a special 'compat' handler instead of just running the
normal 64-bit system call.
The 'struct timespec' is one structure which is sometimes different for
32-bit vs. 64-bit, so any system call taking a 'struct timespec' must
have a separate compat_sys_xxxx() to handle that. See something like
compat_sys_clock_settime() in kernel/compat.c for an example (but don't
use set_fs() like it does; just see how it handles the compat_timespec).
> > Must we have the ioctl-like interface to sys_time_pps_cmd()? If the
>
> It seems to me stronger then other solutions...
>
> > second argument is always 'struct pps_source_data_s *', why does the
> > syscall pretend it's 'void *'?
>
> Just to keep sys_time_pps_cmd() generic for future new commands.
Hm. I'll let other people complain at you about that :)
--
dwmw2
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
