linux-kernel - Re: [PATCH] time/fs - file's time race with vgettimeofday

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: <AANLkTilE3sd7SSLVCN87rpJ5iSm-sYd-n6nJnzlByfRK@mail.gmail.com>
Date:	Tue, 6 Jul 2010 16:03:29 -0700
From:	john stultz <johnstul@...ibm.com>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	linux-kernel@...r.kernel.org, tglx@...utronix.de,
	eric.dumazet@...il.com, oleg@...hat.com
Subject: Re: [PATCH] time/fs - file's time race with vgettimeofday

On Fri, Jul 2, 2010 at 12:41 AM, Jiri Olsa <jolsa@...hat.com> wrote:
> hi,
>
> there's a race among calling gettimeofday(2) and a file's time
> updates.  Following test program expose the race.
>
> run it in the while loop
>        while [ 1 ]; do ./test1 || break; done
>
> --- SNIP ---
> #include <stdio.h>
> #include <stdlib.h>
> #include <fcntl.h>
>
> int main (void)
> {
>        struct stat st;
>        struct timeval tv;
>
>        unlink("./file");
>
>        gettimeofday(&tv, NULL);
>
>        if (-1 == creat("./file", O_RDWR)) {
>                perror("creat");
>                return -1;
>        }
>
>        if (stat("./file", &st) != 0) {
>                perror("stat");
>                return -1;
>        }
>
>        printf("USER gtod: %ld\n", (long)tv.tv_sec);
>        printf("USER file: %ld.%09u\n",
>                        (long) st.st_mtime,
>                        (unsigned) st.st_mtim.tv_nsec);
>
>        return tv.tv_sec <= st.st_mtime ? 0 : -1;
> }
> --- SNIP ---
>
>
> The point is that the stat call returns time that
> sometime precedes time from gettimeofday.
>
> The reason follows.
>
>        - inode's time is initialized by CURRENT_TIME_SEC macro,
>          which returns tv_sec portion of xtime variable
>        - the xtime is updated by update_wall_time being called
>          each tick (not that often for NO_HZ)
>        - vgettimeofday reads the actuall clocksource tick
>          and computes the time
>
> Thus while the inode's time is based on the xtime update
> by the update_wall_time function, the vgettimeofday computes
> the time correctly each time it's called.
>
> Thus the race is triggered within 2 update_wall_time updates,
> when in between the gettimeofday and creat calls happened.
>
>
>
> ticks                   CPU                   update_wall_time executed
> -------------------------------------------------------------------------------
>  t1
>                                                        update 1
>                                           (xtime is computed based on tick t1)
>
>
>  t2
>
>       |         gettimeofday           |
>       | (returns time based on tick 2) |
>       |                                |
>       |            creat               |
>       |   (set time based on tick 1)   |
>
>
>                                                        update 2
>                                           (xtime is computed based on tick t2)
>  t3
> -------------------------------------------------------------------------------
>
>
>
> This issue was described in the BZ 244697
>
>        Time goes backward - gettimeofday() vs. rename()
>        https://bugzilla.redhat.com/show_bug.cgi?id=244697
>
>
> It's not just issue of the creat but few others like rename.
>
>
> The following patch will prevent the race by adding the
> CURRENT_TIME_SEC_REAL macro, which will return seconds from
> the getnstimeofday call, ensuring it's computed on current tick.
> It fixes the 'creat' case for ext4.
>
>
> I'm not sure how much trouble is having this race unfixed compared
> to the performance impact the fix might have. Maybe there're
> better ways to fix this.

I do worry that your patch will have too much of a performance hit. I
think the right fix would be in vtime().

Test patch to follow shortly.

thanks
-john
--
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/