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.

collect $reg
end

Review line 4

Quoted 7 lines

+++ b/arch/arm/include/asm/gtp.h

(quoted diff) +4 -0

@@ -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.

Review line 5

Quoted 1 line

(quoted diff) +1 -0

+#define CORE_ADDR		unsigned long

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

Quoted 5 lines

(quoted diff) +5 -0

+
+#define GTP_REG_ASCII_SIZE	336
+
+static inline void
+gtp_regs2ascii(struct pt_regs *regs, char *buf)

Review line 16

Quoted 6 lines

(quoted diff) +6 -0

+{
+#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:

Review line 20

Quoted 4 lines

(quoted diff) +4 -0

+	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?

Review line 24

Quoted 5 lines

(quoted diff) +4 -0

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

You should use the pr_* macros now

Review line 28

Quoted 5 lines

(quoted diff) +4 -0

+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?

Review

Quoted 13 lines

(quoted diff) +12 -0

+
+	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?

Review

Quoted 6 lines

(quoted diff) +5 -0

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

kzalloc

Same problem in others.

Review

Quoted 2 lines

(quoted diff) +2 -0

+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

Quoted 2 lines · Andi Kleen

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

sudo insmod kernel/gtp.ko

Quoted 7 lines

arch/arm/include/asm/gtp.h line 4

Quoted 4 lines

arch/arm/include/asm/gtp.h new file +4 -0

+#ifndef _ASM_ARM_GTP_H_
+#define _ASM_ARM_GTP_H_
+
+#define ULONGEST		uint64_t

So u64 in kernel. Just use that.

Quoted 4 lines

Quoted 1 line

+#define CORE_ADDR unsigned long

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

ULONGEST and CORE_ADDR is the type from GDB rsp packet. I use it to parse the package from GDB. Then want to add add a new arch to KGTP, just need set this two type same with GDB. So do you mind I keep it?

OK. I changed hex2ulongest to simple_strtoull in new patch. But I meet a issue is checkpatch told me that "simple_strtoull is obsolete, use kstrtoull instead". But kstrtoull cannot get the address that where this convert stop at. Do you have some comments on it?

kernel/gtp.c new file +4 -0

+	if (!gtp_frame) {
+		gtp_frame = vmalloc(GTP_FRAME_SIZE);
+		if (!gtp_frame)
+			return -ENOMEM;

kernel/gtp.c new file +8 -0

+static int __init gtp_init(void)
+{
+	atomic_set(&gtp_count, 0);
+	gtp_read_ack = 0;
+	gtp_rw_buf = NULL;
+	gtp_rw_bufp = NULL;
+	gtp_rw_size = 0;
+	gtp_start = 0;

Quoted 5 lines · Hui Zhu

Quoted 2 lines · Andi Kleen

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

sudo insmod kernel/gtp.ko

Thanks. Please put that into the change log for the next iteration.

Quoted 13 lines

Quoted 12 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+#ifndef _ASM_ARM_GTP_H_
+#define _ASM_ARM_GTP_H_
+
+#define ULONGEST           uint64_t

So u64 in kernel. Just use that.

Quoted 1 line

+#define CORE_ADDR unsigned long

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

Quoted 3 lines

ULONGEST and CORE_ADDR is the type from GDB rsp packet. I use it to parse the package from GDB. Then want to add add a new arch to KGTP, just need set this two type same with GDB. So do you mind I keep it?

I was just aiming to eliminate the asm files. Is that possible? If you can't please reuse stuff from kgdb.

Quoted 5 lines

OK. I changed hex2ulongest to simple_strtoull in new patch. But I meet a issue is checkpatch told me that "simple_strtoull is obsolete, use kstrtoull instead". But kstrtoull cannot get the address that where this convert stop at. Do you have some comments on it?

Keep using simple_strtoull. Ignore checkpatch when it's wrong.

Review

Quoted 4 lines

(quoted diff) +4 -0

+static inline void
+gtp_regs2ascii(struct pt_regs *regs, char *buf)
+{
+#ifdef CONFIG_32BIT

BITS_PER_LONG == 32 ? Then it would work on other archs and you could move it out from asm/

Review

Quoted 12 lines

(quoted diff) +11 -0

+#define OUTFORMAT    "%08lx"
+#define REGSIZE              8
+#define SWAB(a)              ntohl(a)
+#else
+#define OUTFORMAT    "%016lx"
+#define REGSIZE              16
+#ifdef __LITTLE_ENDIAN
+#define SWAB(a)              swab64(a)
+#else
+#define SWAB(a)              (a)
+#endif

be64_to_cpu()

Review line 2

Quoted 4 lines

(quoted diff) +2 -0

@@ -0,0 +1,914 @@
+/*
+ * Kernel GDB tracepoint module.

add one sentence here what this file does.

Review line 9

Quoted 8 lines

(quoted diff) +7 -0

+     memcpy(&freg->regs, regs, sizeof(struct pt_regs));
+#ifdef CONFIG_X86_32
+     freg->regs.sp = (unsigned long)&regs->sp;
+#endif       /* CONFIG_X86_32 */
+#ifdef CONFIG_X86
+     freg->regs.ip -= 1;
+#endif       /* CONFIG_X86 */

What is that for? - 1?

Review line 12

Quoted 4 lines

(quoted diff) +3 -0

+
+static void
+gtp_action_release(struct action *ae)

Standard style is type on the same line.

Review line 21

Quoted 10 lines

(quoted diff) +9 -0

+{
+     struct action   *ae2;
+
+     while (ae) {
+             ae2 = ae;
+             ae = ae->next;
+             /* Release ae2.  */
+             kfree(ae2);
+     }

Better use a list_head from list.h and list_for_each_entry_safe() etc. This will simplify some of the list code.

Review line 28

Quoted 8 lines

(quoted diff) +7 -0

+static int
+gtp_gdbrsp_qtstart(void)
+{
+     struct gtp_entry        *tpe;
+
+     if (gtp_start)
+             return -EBUSY;

This is in a lot of places. Can you put it somewhere more central?

Quoted 8 lines

(quoted diff) +8 -0

+     if (!gtp_frame) {
+             gtp_frame = vmalloc(GTP_FRAME_SIZE);
+             if (!gtp_frame)
+                     return -ENOMEM;
+
+             gtp_frame_reset();
+     }
+

Review line 42

Quoted 6 lines

(quoted diff) +6 -0

+     for (tpe = gtp_list; tpe; tpe = tpe->next) {
+             if (tpe->action_list) {
+                     int     ret = register_kprobe(&tpe->kp);
+                     if (ret < 0) {
+                             gtp_gdbrsp_qtstop();
+                             return ret;

memory leak with frame?

Review line 46

Quoted 5 lines

(quoted diff) +4 -0

+     pr_devel("gtp_gdbrsp_QT: %s\n", pkg);
+
+     if (strcmp("init", pkg) == 0)
+             ret = gtp_gdbrsp_qtinit();

This if cascade should be probably a table walk

Review line 53

Quoted 8 lines

(quoted diff) +7 -0

+     else if (strcmp("Stop", pkg) == 0)
+             ret = gtp_gdbrsp_qtstop();
+
+     return ret;
+}
+
+static unsigned char gtp_m_buffer[0xffff];

Ok so what lock protects this buffer?

Review line 57

Quoted 5 lines

(quoted diff) +4 -0

+     struct gtp_frame_reg    *fr = NULL;
+
+     if (GTP_RW_MAX - 4 - gtp_rw_size < GTP_REG_ASCII_SIZE)
+             return -E2BIG;

The magic 4 should be a define

Review line 64

Quoted 8 lines

(quoted diff) +7 -0

+     next = gtp_frame_current->next;
+     while (next) {
+             switch (next[0]) {
+             case 'r':
+                     fr = (struct gtp_frame_reg *) (next + 1);
+                     goto check;
+                     break;

either check or break, but not both

Review line 66

Quoted 2 lines

(quoted diff) +2 -0

+             default:
+                     next = NULL;

Not needed

Quoted 3 lines

(quoted diff) +3 -0

+                     break;
+             }
+     }

Review line 74

Quoted 5 lines

(quoted diff) +5 -0

+check:
+     if (fr)
+             gtp_regs2ascii(&fr->regs, gtp_rw_bufp);
+     else
+             memset(gtp_rw_bufp, '0', GTP_REG_ASCII_SIZE);

No 0 termination?

Review line 82

Quoted 9 lines

(quoted diff) +8 -0

+
+static int
+gtp_open(struct inode *inode, struct file *file)
+{
+     if (atomic_inc_return(&gtp_count) > 1) {
+             atomic_dec(&gtp_count);
+             return -EBUSY;
+     }

This looks racy. Better use a try mutex lock

Quoted 13 lines

(quoted diff) +12 -0

+static int __init gtp_init(void)
+{
+     atomic_set(&gtp_count, 0);
+     gtp_read_ack = 0;
+     gtp_rw_buf = NULL;
+     gtp_rw_bufp = NULL;
+     gtp_rw_size = 0;
+     gtp_start = 0;
+     gtp_disconnected_tracing = 0;
+     gtp_circular = 0;
+     gtp_frame_num = 0;
+     gtp_frame = NULL;

Review line 100

Quoted 6 lines

(quoted diff) +6 -0

+     gtp_frame_r_start = NULL;
+     gtp_frame_w_start = NULL;
+     gtp_frame_w_end = NULL;
+     gtp_frame_r_cache = NULL;
+     gtp_frame_is_circular = 0;
+     gtp_frame_current = NULL;

Globals don't need to be initialized with 0

Review line 107

Quoted 9 lines

(quoted diff) +7 -0

+config GTP
+     tristate "GDB tracepoint support"
+     depends on X86 || ARM || MIPS
+     select KPROBES
+     select DEBUG_FS
+     ---help---
+       Supply GDB tracepoint interface in /sys/kernel/debug/gtp.

Need far more description here. Write 1-2 paragraphs why a user wants this. checkpatch should have warned.

-Andi
--
[email protected] -- Speaking for myself only.

I remove them from asm files, and add:
#ifndef ULONGEST
#define ULONGEST uint64_t
#endif
#ifndef CORE_ADDR
#define CORE_ADDR unsigned long
#endif
in gtp.c
Then if not need, the arch didn't define ULONGEST and CORE_ADDR for itself.

For example ARM and MIPS have a lot of special reg that cannot be converted by a for loop.

After check the code the kgdb, I thought that good ways is use dbg_reg_def of kgdb to convert the reg to gdb rsp format. But use it directly will make kgtp depend on kgdb.

What I thought is move this part of code out from kgdb, when kgdb or kgtp need, select it too. But I am not sure Jason is OK with that.

Quoted 10 lines

kernel/gtp.c line 171

Quoted 8 lines

kernel/gtp.c new file +7 -0

+     memcpy(&freg->regs, regs, sizeof(struct pt_regs));
+#ifdef CONFIG_X86_32
+     freg->regs.sp = (unsigned long)&regs->sp;
+#endif       /* CONFIG_X86_32 */
+#ifdef CONFIG_X86
+     freg->regs.ip -= 1;
+#endif       /* CONFIG_X86 */

What is that for? - 1?

That is because when kprobe, the ip of X86 point will have a offset with current ip.

Quoted 5 lines

kernel/gtp.c line 306

Quoted 3 lines

kernel/gtp.c new file +3 -0

+
+     if (gtp_start)
+             return -EBUSY;

This is in a lot of places. Can you put it somewhere more central?

Sorry I don't understand this part. Do you want I change all "struct
gtp_entry *tpe;" to "struct gtp_entry *tpe;"?

Quoted 7 lines

kernel/gtp.c line 598

Quoted 5 lines

kernel/gtp.c new file +4 -0

+     pr_devel("gtp_gdbrsp_QT: %s\n", pkg);
+
+     if (strcmp("init", pkg) == 0)
+             ret = gtp_gdbrsp_qtinit();

This if cascade should be probably a table walk

I cannot find the examle about it in the Kernel, could you give me a example?

Quoted 1 line

Ok so what lock protects this buffer?

No. The gtp_frame_lock protects the vars that follow it:
static int gtp_frame_num;
static char *gtp_frame;
static char *gtp_frame_r_start;
static char *gtp_frame_w_start;
static char *gtp_frame_w_end;
static char *gtp_frame_r_cache;
static int gtp_frame_is_circular;
It protects them all.

gtp_m_buffer is protected by gtp_rw_lock that I just add in new patch.

OK. Add some introduce. checkpatch is OK with it.

Quoted 9 lines

I remove them from asm files, and add:
#ifndef ULONGEST
#define ULONGEST uint64_t
#endif
#ifndef CORE_ADDR
#define CORE_ADDR unsigned long
#endif
in gtp.c
Then if not need, the arch didn't define ULONGEST and CORE_ADDR for itself.

Which architecture needs it? Linux prefers to use "native" types.

Quoted 7 lines

For example ARM and MIPS have a lot of special reg that cannot be converted by a for loop.

After check the code the kgdb, I thought that good ways is use dbg_reg_def of kgdb to convert the reg to gdb rsp format. But use it directly will make kgtp depend on kgdb.

That's fine. Code reuse is good. Code duplication is bad.

Quoted 2 lines

What I thought is move this part of code out from kgdb, when kgdb or kgtp need, select it too. But I am not sure Jason is OK with that.

Just send a patch.

Quoted 12 lines

Quoted 11 lines

Review

Quoted 9 lines

(quoted diff) +7 -0

+     memcpy(&freg->regs, regs, sizeof(struct pt_regs));
+#ifdef CONFIG_X86_32
+     freg->regs.sp = (unsigned long)&regs->sp;
+#endif       /* CONFIG_X86_32 */
+#ifdef CONFIG_X86
+     freg->regs.ip -= 1;
+#endif       /* CONFIG_X86 */

What is that for? - 1?

Quoted 2 lines

That is because when kprobe, the ip of X86 point will have a offset with current ip.

But a kprobe is not necessarily one byte. e.g. a jumper probe is longer.

Anyways if that is really needed there should be some macros provided by kprobes to adjust IP. If it's not there it needs to be added.

Quoted 8 lines

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+     if (gtp_start)
+             return -EBUSY;

This is in a lot of places. Can you put it somewhere more central?

Sorry I don't understand this part. Do you want I change all "struct
gtp_entry *tpe;" to "struct gtp_entry *tpe;"?

I mean the if (gtp_start) return -EBUSY check.

Quoted 10 lines

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+     pr_devel("gtp_gdbrsp_QT: %s\n", pkg);
+
+     if (strcmp("init", pkg) == 0)
+             ret = gtp_gdbrsp_qtinit();

This if cascade should be probably a table walk

I cannot find the examle about it in the Kernel, could you give me a example?

struct cmd {
        char *name;
        int (*init)(void);
};

struct cmd cmds[] = {
        { "init", gtp_xyz_init }
        ...
        {}
};

int i;
for (i = 0; cmds[i].name; i++)

        if (!strcmp(cmds[i].name, pkg))
                ret = cmds->init();

Quoted 13 lines

Quoted 2 lines

Ok so what lock protects this buffer?

No. The gtp_frame_lock protects the vars that follow it:

static int                    gtp_frame_num;
static char                   *gtp_frame;
static char                   *gtp_frame_r_start;
static char                   *gtp_frame_w_start;
static char                   *gtp_frame_w_end;
static char                   *gtp_frame_r_cache;
static int                    gtp_frame_is_circular;

It protects them all.

Quoted 1 line

gtp_m_buffer is protected by gtp_rw_lock that I just add in new patch.

Then your comment was wrong/unclear?

Quoted 4 lines

OK. Add some introduce. checkpatch is OK with it.

You must be using an old version?

# check for Kconfig help text having a real description
# Only applies when adding the entry originally, after that we do not have
# sufficient context to determine whether it is indeed long enough.
...
        WARN("CONFIG_DESCRIPTION",
                             "please write a paragraph that describes the config symbol fully\n" . $herecurr) if ($is_start && $is_end && $length < 4);

I wonder how you avoided that.