linux-kernel - Re: [PATCH] v2 Re: Bug: Status/Summary of slashdot leap-second crash on new years 2008-2009

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: <e9e943910901031104n36bb216al9b81304eefde4085@mail.gmail.com>
Date:	Sat, 3 Jan 2009 19:04:24 +0000
From:	"Duane Griffin" <duaneg@...da.com>
To:	"Chris Adams" <cmadams@...aay.net>
Cc:	"Linas Vepstas" <linasvepstas@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] v2 Re: Bug: Status/Summary of slashdot leap-second crash on new years 2008-2009

2009/1/3 Chris Adams <cmadams@...aay.net>:
> Once upon a time, Duane Griffin <duaneg@...da.com> said:
>> How about instead of a switch statement, assigning the message to a
>> variable and printing that. I.e. something like:
>
> Good point.  Here's an updated version that also adds a comment to the
> xtime_lock definition about not using printk.

Good idea.

> --
> Chris Adams <cmadams@...aay.net>
> Systems and Network Administrator - HiWAAY Internet Services
> I don't speak for anybody but myself - that's enough trouble.
>
>
> From: Chris Adams <cmadams@...aay.net>
>
> The code to handle leap seconds printks an information message when the
> second is inserted or deleted.  It does this while holding xtime_lock.
> However, printk wakes up klogd, and in some cases, the scheduler tries
> to get the current kernel time, trying to get xtime_lock (which results
> in a deadlock).  This moved the printks outside of the lock.  It also
> adds a comment to not use printk while holding xtime_lock.
>
> Signed-off-by: Chris Adams <cmadams@...aay.net>
> ---
> diff -urpN linux-2.6.28-git5-vanilla/include/linux/time.h linux-2.6.28-git5/include/linux/time.h
> --- linux-2.6.28-git5-vanilla/include/linux/time.h      2009-01-02 22:09:10.000000000 -0600
> +++ linux-2.6.28-git5/include/linux/time.h      2009-01-03 11:57:27.000000000 -0600
> @@ -99,6 +99,12 @@ static inline struct timespec timespec_s
>
>  extern struct timespec xtime;
>  extern struct timespec wall_to_monotonic;
> +
> +/*
> + * Do not call printk while holding this lock; it wakes klogd and the
> + * scheduler may try to get the current kernel time, which will try to get
> + * this lock.
> + */
>  extern seqlock_t xtime_lock;
>
>  extern unsigned long read_persistent_clock(void);
> diff -urpN linux-2.6.28-git5-vanilla/kernel/time/ntp.c linux-2.6.28-git5/kernel/time/ntp.c
> --- linux-2.6.28-git5-vanilla/kernel/time/ntp.c 2009-01-02 22:09:34.000000000 -0600
> +++ linux-2.6.28-git5/kernel/time/ntp.c 2009-01-03 11:57:46.000000000 -0600
> @@ -130,6 +130,7 @@ void ntp_clear(void)
>  static enum hrtimer_restart ntp_leap_second(struct hrtimer *timer)
>  {
>        enum hrtimer_restart res = HRTIMER_NORESTART;
> +       const char *msg = NULL;
>
>        write_seqlock(&xtime_lock);
>
> @@ -140,8 +141,7 @@ static enum hrtimer_restart ntp_leap_sec
>                xtime.tv_sec--;
>                wall_to_monotonic.tv_sec++;
>                time_state = TIME_OOP;
> -               printk(KERN_NOTICE "Clock: "
> -                      "inserting leap second 23:59:60 UTC\n");
> +               msg = "Clock: inserting leap second 23:59:60 UTC";
>                hrtimer_add_expires_ns(&leap_timer, NSEC_PER_SEC);
>                res = HRTIMER_RESTART;
>                break;
> @@ -150,8 +150,7 @@ static enum hrtimer_restart ntp_leap_sec
>                time_tai--;
>                wall_to_monotonic.tv_sec--;
>                time_state = TIME_WAIT;
> -               printk(KERN_NOTICE "Clock: "
> -                      "deleting leap second 23:59:59 UTC\n");
> +               msg = "Clock: deleting leap second 23:59:59 UTC";
>                break;
>        case TIME_OOP:
>                time_tai++;
> @@ -166,6 +165,9 @@ static enum hrtimer_restart ntp_leap_sec
>
>        write_sequnlock(&xtime_lock);
>
> +       if (msg)
> +               printk(KERN_NOTICE "%s\n", msg);
> +
>        return res;
>  }

Looks good to me!

Cheers,
Duane.

-- 
"I never could learn to drink that blood and call it wine" - Bob Dylan
--
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/