Jovi Zhangwei <jovi.zhang...@gmail.com> writes:
Quick review of the file.
> +#include <linux/version.h>
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 1, 0)
> +#error "Currently ktap don't support kernel older than 3.1"
> +#endif
> +
> +#if !CONFIG_EVENT_TRACING
> +#error "Please enable CONFIG_EVENT_TRACING before compile ktap"
> +#endif
> +
> +#if !CONFIG_PERF_EVENTS
> +#error "Please enable CONFIG_PERF_EVENTS before compile ktap"
> +#endif
This should be all in Kconfig
> +/* common helper function */
> +long gettimeofday_ns(void)
> +{
> + struct timespec now;
> +
> + getnstimeofday(&now);
> + return now.tv_sec * NSEC_PER_SEC + now.tv_nsec;
> +}
This is ktime_get()
Or more likely you want something like trace_clock(), otherwise
you may have problems on systems not using TSC.
> +
> +static int load_trunk(ktap_option_t *parm, unsigned long **buff)
> +{
> + int ret;
> + unsigned long *vmstart;
> +
> + vmstart = vmalloc(parm->trunk_len);
> + if (!vmstart)
> + return -ENOMEM;
> +
> + ret = copy_from_user(vmstart, (void __user *)parm->trunk,
> + parm->trunk_len);
Needs unsigned length check if supplied from user space.
> + if (ret < 0) {
> + vfree(vmstart);
> + return -EFAULT;
> + }
> +
> + *buff = vmstart;
> + return 0;
> +}
> +
> +static struct dentry *kp_dir_dentry;
What lock protects that?
> +static long ktap_ioctl(struct file *file, unsigned int cmd, unsigned long
> arg)
> +{
> + ktap_option_t parm;
> + int ret;
> +
> + switch (cmd) {
> + case KTAP_CMD_IOC_VERSION:
> + print_version();
This seems odd. Should return the version instead?
> + return 0;
> + case KTAP_CMD_IOC_RUN:
> + /*
> + * must be root to run ktap script (at least for now)
> + *
> + * TODO: check perf_paranoid sysctl and allow non-root user
> + * to use ktap for tracing process(like uprobe) ?
This would need ensuring first that there is no possible way to crash
the kernel. Probably very hard.
> + */
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> + ret = copy_from_user(&parm, (void __user *)arg,
> + sizeof(ktap_option_t));
> + if (ret < 0)
> + return -EFAULT;
The check is wrong, it should be != 0
> +struct syscall_metadata **syscalls_metadata;
> +
> +/*TODO: kill this function in future */
> +static int __init init_dummy_kernel_functions(void)
> +{
> + unsigned long *addr;
> +
> + /*
> + * ktap need symbol ftrace_profile_set_filter to set event filter,
> + * export it in future.
Obviously you need to fix that. Just export the symbols.
> +
> +extern struct syscall_metadata **syscalls_metadata;
> +
> +/* get from kernel/trace/trace.h */
Use that version instead.
> +static __always_inline int trace_get_context_bit(void)
> +{
> + int bit;
> +
> + if (in_interrupt()) {
> + if (in_nmi())
> + bit = 0;
> + else if (in_irq())
> + bit = 1;
> + else
> + bit = 2;
> + } else
> + bit = 3;
> +
> + return bit;
> +}
-Andi
--
a...@linux.intel.com -- Speaking for myself only
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
