| 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
| ||
|
Message-ID: <20100702161422.GA31733@redhat.com>
Date: Fri, 2 Jul 2010 18:14:22 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Jiri Olsa <jolsa@...hat.com>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.de,
eric.dumazet@...il.com, Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] time/fs - file's time race with vgettimeofday
In no way I can review this patch, but I am curious and have the questions.
Also, I think it makes sense to cc the fs/ developers, I've added Al.
On 07/02, Jiri Olsa wrote:
>
> 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;
> }
Interesting. To the point, I actually compiled this code and yes,
it triggers the problem on ext3 ;)
> 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.
What about other filesystems? Perhaps it makes sense to change
CURRENT_TIME_SEC instead of adding CURRENT_TIME_SEC_REAL?
Once again, I am asking. It is not that I suggest to change your patch.
But there is something I can't understand.
We have
#define CURRENT_TIME_SEC ((struct timespec) { get_seconds(), 0 })
and your patch adds
#define CURRENT_TIME_SEC_REAL ((struct timespec) { get_seconds_real(), 0 })
which fixes the problem for ext4.
But, get_seconds() is also used by sys_time(), and we should have the
same problem with another trivial test-case:
#include <stdio.h>
#include <sys/time.h>
#include <time.h>
int main(void)
{
struct timeval tv;
int sec;
do {
gettimeofday(&tv, NULL);
sec = time(NULL);
} while (tv.tv_sec <= sec);
printf("gtod: %ld.%06ld\n", tv.tv_sec, tv.tv_usec);
printf("time: %d.000000\n", sec);
return 0;
}
However, this test-case can't trigger the problem. Confused.
Oleg.
>
> 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.
>
>
> thanks for any ideas,
> jirka
>
>
>
> Signed-off-by: Jiri Olsa <jolsa@...hat.com>
> ---
> diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
> index 938dbc7..7a0a2fc 100644
> --- a/fs/ext2/ialloc.c
> +++ b/fs/ext2/ialloc.c
> @@ -558,7 +558,7 @@ got:
>
> inode->i_ino = ino;
> inode->i_blocks = 0;
> - inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC;
> + inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC_REAL;
> memset(ei->i_data, 0, sizeof(ei->i_data));
> ei->i_flags =
> ext2_mask_flags(mode, EXT2_I(dir)->i_flags & EXT2_FL_INHERITED);
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 19a4de5..2c2925c 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1157,7 +1157,7 @@ static inline struct ext4_inode_info *EXT4_I(struct inode *inode)
> static inline struct timespec ext4_current_time(struct inode *inode)
> {
> return (inode->i_sb->s_time_gran < NSEC_PER_SEC) ?
> - current_fs_time(inode->i_sb) : CURRENT_TIME_SEC;
> + current_fs_time(inode->i_sb) : CURRENT_TIME_SEC_REAL;
> }
>
> static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
> diff --git a/include/linux/time.h b/include/linux/time.h
> index ea3559f..f390687 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -109,12 +109,14 @@ void timekeeping_init(void);
> extern int timekeeping_suspended;
>
> unsigned long get_seconds(void);
> +unsigned long get_seconds_real(void);
> struct timespec current_kernel_time(void);
> struct timespec __current_kernel_time(void); /* does not hold xtime_lock */
> struct timespec get_monotonic_coarse(void);
>
> #define CURRENT_TIME (current_kernel_time())
> #define CURRENT_TIME_SEC ((struct timespec) { get_seconds(), 0 })
> +#define CURRENT_TIME_SEC_REAL ((struct timespec) { get_seconds_real(), 0 })
>
> /* Some architectures do not supply their own clocksource.
> * This is mainly the case in architectures that get their
> diff --git a/kernel/time.c b/kernel/time.c
> index 848b1c2..ce10dae 100644
> --- a/kernel/time.c
> +++ b/kernel/time.c
> @@ -227,7 +227,8 @@ SYSCALL_DEFINE1(adjtimex, struct timex __user *, txc_p)
> */
> struct timespec current_fs_time(struct super_block *sb)
> {
> - struct timespec now = current_kernel_time();
> + struct timespec now;
> + getnstimeofday(&now);
> return timespec_trunc(now, sb->s_time_gran);
> }
> EXPORT_SYMBOL(current_fs_time);
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index caf8d4d..7ebfe23 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -897,6 +897,14 @@ unsigned long get_seconds(void)
> }
> EXPORT_SYMBOL(get_seconds);
>
> +unsigned long get_seconds_real(void)
> +{
> + struct timespec ts;
> + getnstimeofday(&ts);
> + return ts.tv_sec;
> +}
> +EXPORT_SYMBOL(get_seconds_real);
> +
> struct timespec __current_kernel_time(void)
> {
> return xtime;
--
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/
Powered by blists - more mailing lists