linux-kernel - Re: [PATCH]KGTP (Linux Kernel debugger and tracer) lite patch for review

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120509140538.GZ27374@one.firstfloor.org>
Date:	Wed, 9 May 2012 16:05:39 +0200
From:	Andi Kleen <andi@...stfloor.org>
To:	Hui Zhu <teawater@...il.com>
Cc:	linux-kernel@...r.kernel.org, Andi Kleen <andi@...stfloor.org>,
	Christoph Hellwig <hch@...radead.org>,
	Geoff Levand <geoff@...radead.org>, jason.wessel@...driver.com
Subject: Re: [PATCH]KGTP (Linux Kernel debugger and tracer) lite patch for review

Please provide better explanation of the use case for this module.
One paragraph why someone would want it in their kernel.

> +++ b/arch/arm/include/asm/gtp.h
> @@ -0,0 +1,34 @@
> +#ifndef _ASM_ARM_GTP_H_
> +#define _ASM_ARM_GTP_H_
> +
> +#define ULONGEST		uint64_t

So u64 in kernel. Just use that.

> +#define CORE_ADDR		unsigned long

In linux kernel CORE_ADDR is always unsigned long. Use that.

> +
> +#define GTP_REG_ASCII_SIZE	336
> +
> +static inline void
> +gtp_regs2ascii(struct pt_regs *regs, char *buf)
> +{
> +#ifdef __LITTLE_ENDIAN
> +#define SWAB(a)		swab32(a)
> +#else
> +#define SWAB(a)		(a)
> +#endif

That's just ntohl()? Just use that

Linux already has macros for this: 
> +	int	i;
> +
> +	for (i = 0; i < 16; i++) {
> +		sprintf(buf, "%08lx", (unsigned long) SWAB(regs->uregs[i]));

Is the gdb protocol really big endian in ASCII?

Also could you share code on this with the in kernel gdbstub?

> +#include <linux/slab.h>
> +#include <asm/gtp.h>
> +
> +#define GTP_DEBUG		KERN_WARNING

You should use the pr_* macros now

> +static int			gtp_disconnected_tracing;
> +static int			gtp_circular;
> +
> +static DEFINE_SPINLOCK(gtp_frame_lock);

Every spinlock needs a comment that describes what it protects.
I believe checkpatch warns about that. Did you run it?

> +
> +	tmp = gtp_frame_alloc(GTP_FRAME_REG_SIZE);
> +	if (!tmp)
> +		return NULL;
> +
> +	*next = tmp;
> +	tmp[0] = 'r';
> +	freg = (struct gtp_frame_reg *) (tmp + 1);
> +	memcpy(&freg->regs, regs, sizeof(struct pt_regs));
> +#if !defined CONFIG_X86_32 && !defined CONFIG_X86_64
> +	freg->regs.sp = (unsigned long)&regs->sp;
> +#endif	/* CONFIG_X86_32 CONFIG_X86_64 */

That looks weird. What does that do?

> +gtp_action_alloc(char type)
> +{
> +	struct action	*ret;
> +
> +	ret = kmalloc(sizeof(struct action), GFP_KERNEL);

kzalloc

Same problem in others.

> +static int
> +hex2int(char hex, int *i)

I'm sure we have code for this. strtoul et.al.?

Didn't read further so far.


-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/