Re: [PATCH] init - fix building bug and potential buffer overflow

From: Linus Torvalds
Date: Thu May 15 2008 - 17:16:10 EST




On Thu, 15 May 2008, Andrew Morton wrote:
>
> (yeah, I normally parenthesise sizeof too, but this provided 80-col
> salvation)

Why?

The nice way to provide 80-col salvation is to just fix the code. Instead
of having complex nested loops, have a really trivial outer loop like

for (call = __initcall_start; call < __initcall_end; call++)
do_one_initcall(*call);

and then the code to actually do one of the initcalls is much simpler to
read, and because it's not as indented, the 80-column issues just go away.

IOW, something like this (the diff looks ugly, but the end result is
better).

Talking about cleanups, why the *hell* does print_fn_descriptor_symbol()
take an "unsigned long", when every single user would want to give it a
pointer?

Would somebody please want to move that cast into the macro (or better
yet, make it an inline function that takes a 'void *'), and remove all the
casts from the callers?

Linus

---
init/main.c | 78 +++++++++++++++++++++++++++++++----------------------------
1 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/init/main.c b/init/main.c
index f406fef..0a1d65e 100644
--- a/init/main.c
+++ b/init/main.c
@@ -695,53 +695,57 @@ __setup("initcall_debug", initcall_debug_setup);

extern initcall_t __initcall_start[], __initcall_end[];

-static void __init do_initcalls(void)
+static void __init do_one_initcall(initcall_t call)
{
- initcall_t *call;
int count = preempt_count();
+ ktime_t t0, t1, delta;
+ char msgbuf[64];
+ int result;
+
+ if (initcall_debug) {
+ print_fn_descriptor_symbol("calling %s()\n",
+ (unsigned long) call);
+ t0 = ktime_get();
+ }

- for (call = __initcall_start; call < __initcall_end; call++) {
- ktime_t t0, t1, delta;
- char msgbuf[40];
- int result;
+ result = call();

- if (initcall_debug) {
- print_fn_descriptor_symbol("calling %s()\n",
- (unsigned long) *call);
- t0 = ktime_get();
- }
+ if (initcall_debug) {
+ t1 = ktime_get();
+ delta = ktime_sub(t1, t0);

- result = (*call)();
+ print_fn_descriptor_symbol("initcall %s()",
+ (unsigned long) call);
+ printk(" returned %d after %Ld msecs\n", result,
+ (unsigned long long) delta.tv64 >> 20);
+ }

- if (initcall_debug) {
- t1 = ktime_get();
- delta = ktime_sub(t1, t0);
+ msgbuf[0] = 0;

- print_fn_descriptor_symbol("initcall %s()",
- (unsigned long) *call);
- printk(" returned %d after %Ld msecs\n", result,
- (unsigned long long) delta.tv64 >> 20);
- }
+ if (result && result != -ENODEV && initcall_debug)
+ sprintf(msgbuf, "error code %d ", result);

- msgbuf[0] = 0;
+ if (preempt_count() != count) {
+ strlcat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
+ preempt_count() = count;
+ }
+ if (irqs_disabled()) {
+ strlcat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
+ local_irq_enable();
+ }
+ if (msgbuf[0]) {
+ print_fn_descriptor_symbol(KERN_WARNING "initcall %s()",
+ (unsigned long) call);
+ printk(" returned with %s\n", msgbuf);
+ }
+}

- if (result && result != -ENODEV && initcall_debug)
- sprintf(msgbuf, "error code %d ", result);
+static void __init do_initcalls(void)
+{
+ initcall_t *call;

- if (preempt_count() != count) {
- strncat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
- preempt_count() = count;
- }
- if (irqs_disabled()) {
- strncat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
- local_irq_enable();
- }
- if (msgbuf[0]) {
- print_fn_descriptor_symbol(KERN_WARNING "initcall %s()",
- (unsigned long) *call);
- printk(" returned with %s\n", msgbuf);
- }
- }
+ for (call = __initcall_start; call < __initcall_end; call++)
+ do_one_initcall(*call);

/* Make sure there is no pending stuff from the initcall sequence */
flush_scheduled_work();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/