linux-kernel - Re: [GIT PULL] timestamp fixes

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: <CAHk-=wgxpneOTcf_05rXMMc-djV44HD-Sx6RdM9dnfvL3m10EA@mail.gmail.com>
Date:   Mon, 18 Sep 2023 13:18:07 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] timestamp fixes

On Mon, 18 Sept 2023 at 12:39, Jeff Layton <jlayton@...nel.org> wrote:
>
> In general, we always update the atime with a coarse-grained timestamp,
> since atime and ctime updates are never done together during normal read
> and write operations. As you note, things are a little more murky with
> utimes() updates but I think we should be safe to overwrite the atime
> with a coarse-grained timestamp unconditionally.

I do think utimes() ends up always overwriting, but that's a different
code-path entirely (ie it goes through the ->setattr() logic, not this
inode_update_timestamps() code).

So I *think* that even with your patch, doing a "touch" would end up
doing the right thing - it would update atime even if it was in the
future before.

But doing a plain "read()" would break, and not update atime.

That said, I didn't actually ever *test* any of this, so this is
purely from reading the patch, and I can easily have missed something.

Anyway, I do think that the timespec64_equal() tests are a bit iffy in
fs/inode.c now, since the timespecs that are being tested might be of
different precision.

So I do think there's a *problem* here, I just do not believe that
doing that timespec64_equal() -> timespec64_compare() is at all the
right thing to do.

My *gut* feel is that in both cases, we have this

        if (timespec64_equal(&inode->i_atime, &now))

and the problem is that *sometimes* 'now' is the coarse time, but
sometimes it's the fine-grained one, and so checking for equality is
simply nonsensical.

I get the feeling that that timespec64_equal() logic for those atime
updates should be something like

 - if 'now' is in the future, we always considering it different, and
update the time

 - if 'now' is further in the past than the coarse granularity, we
also update the time ("clearly not equal")

 - but if 'now' is in the past, but within the coarse time
granularity, we consider it equal and do not update anything

but it's not like I have really given this a huge amount of thought.
It's just that "don't update if in the past" that I am pretty sure can
*not* be right.

                  Linus