Re: the printk problem

On Fri, 4 Jul 2008 13:02:05 -0700 (PDT) Linus Torvalds [off-list ref] wrote:

On Fri, 4 Jul 2008, Linus Torvalds wrote:

quoted

so I think we could easily just say that we extend %p in various ways:

 - %pS - print pointer as a symbol

and leave tons of room for future extensions for different kinds of 
pointers. 

So here's a totally untested example patch of this, which could probably 
easily be extended to to other things.

I actually made it '%pF' and '%pS' for a Function descriptor pointer and 
normal Symbolic pointer respectively, because of the stupid things ia64 
and PPC64 do with the pointer indirection through function descriptors. 

That function descriptor indirection is totally untested, and I did it 
with a

	pagefault_disable();
	__get_user(..)
	pagefault_enable();

thing because I thought it would be nice if printk() was always safe, and 
passing bogus function pointers to '%pF' should try to work, but quite 
frankly, I didn't even check that that part compiles, much less works.

probe_kernel_address() should be usable here.

hm, is that needed for other reasons than "it will fault"?

otherwise we could walk the whole string with probe_kernel_address()
before we do anything with it.

That's slightly racy against vunmap and CONFIG_DEBUG_PAGEALLOC and
stuff.  Fixable by *never* dereferencing that pointer except via
probe_kernel_address().

+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	char sym[KSYM_SYMBOL_LEN];
+	switch (*fmt) {
+	case 'F':	/* Function pointer */
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+		{ void *p;
+		pagefault_disable();
+		if (!__get_user((void **)ptr, &p))
+			ptr = p;
+		pagefault_enable();
+		}
+#endif
+	/* Fallthrough */
+	case 'S':	/* Other pointer */
+#if CONFIG_KALLSYMS
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}

If this takes off we might want a register-your-printk-handler
interface.  Maybe.

We also jump through hoops to print things like sector_t and
resource_size_t.  They always need to be cast to `unsiged long long',
which generates additional stack space and text in some setups.

And then there's the perennial "need to cast u64 to unsigned long long
to print it".  If we were to do

	printk("%SL", (void *)some_u64);

then that's still bloody ugly, but it'll save a little text-n-stack.