Re: [PATCH 01/10] fs: turn inode ctime fields into a single ktime_t

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2024-07-02 at 05:15 -0700, Christoph Hellwig wrote:
> On Tue, Jul 02, 2024 at 08:09:46AM -0400, Jeff Layton wrote:
> > > > corrupt timestamps like this?
> > > 
> > > inode_set_ctime_to_ts should return an error if things are out of
> > > range.
> > 
> > Currently it just returns the timespec64 we're setting it to (which
> > makes it easy to do several assignments), so we'd need to change
> > its
> > prototype to handle this case, and fix up the callers to recognize
> > the
> > error.
> > 
> > Alternately it may be easier to just add in a test for when
> > __i_ctime == KTIME_MAX in the appropriate callers and have them
> > error
> > out. I'll have a look and see what makes sense.
> 
> The seems like a more awkward interface vs one that explicitly
> checks.
> 

Many of the existing callers of inode_ctime_to_ts are in void return
functions. They're just copying data from an internal representation to
struct inode and assume it always succeeds. For those we'll probably
have to catch bad ctime values earlier.

So, I think I'll probably have to roll bespoke error handling in all of
the relevant filesystems if we go this route. There are also
differences between filesystems -- does it make sense to refuse to load
an inode with a bogus ctime on NFS or AFS? Probably not.

Hell, it may be simpler to just ditch this patch and reimplement
mgtimes using the nanosecond fields like the earlier versions did.

I'll need to study this a bit and figure out what's best.

> > 
> > > How do we currently catch this when it comes from userland?
> > > 
> > 
> > Not sure I understand this question. ctime values should never come
> > from userland. They should only ever come from the system clock.
> 
> Ah, yes, utimes only changes mtime.

Yep. That's the main reason I see the ctime as very different from the
atime or mtime.
-- 
Jeff Layton <jlayton@kernel.org>