Re: [PATCH 07/28] ktap: add runtime/ktap.[c|h]

[Andi Kleen <andi@firstfloor.org>]

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