lkml.org 
[lkml]   [2014]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 07/28] ktap: add runtime/ktap.[c|h]
Date
Jovi Zhangwei <jovi.zhangwei@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
--
ak@linux.intel.com -- Speaking for myself only


\
 
 \ /
  Last update: 2014-03-28 20:01    [W:0.256 / U:0.844 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site