Messages in this thread | | | Subject | Re: [PATCH] LinuxPPS (with new syscalls API) | From | David Woodhouse <> | Date | Tue, 26 Jun 2007 18:38:40 +0100 |
| |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |