Re: [apparmor] [PATCH v2 08/92] fs: new helper: simple_rename_timestamp

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2023-07-06 at 21:02 +0000, Seth Arnold wrote:
> On Wed, Jul 05, 2023 at 08:04:41PM -0400, Jeff Layton wrote:
> > 
> > I don't believe it's an issue. I've seen nothing in the POSIX spec that
> > mandates that timestamp updates to different inodes involved in an
> > operation be set to the _same_ value. It just says they must be updated.
> > 
> > It's also hard to believe that any software would depend on this either,
> > given that it's very inconsistent across filesystems today. AFAICT, this
> > was mostly done in the past just as a matter of convenience.
> 
> I've seen this assumption in several programs:
> 

Thanks for looking into this!

To be clear, POSIX doesn't require that _different_ inodes ever be set
to the same timestamp value. IOW, it certainly doesn't require that the
source and target directories on a rename() end up with the exact same
timestamp value.

Granted, POSIX is rather vague on timestamps in general, but most of the
examples below involve comparing different timestamps on the _same_
inode.


> mutt buffy.c
> https://sources.debian.org/src/mutt/2.2.9-1/buffy.c/?hl=625#L625
> 
>   if (mailbox->newly_created &&
>       (sb->st_ctime != sb->st_mtime || sb->st_ctime != sb->st_atime))
>     mailbox->newly_created = 0;
> 

This should be fine with this patchset. Note that this is comparing
a/c/mtime on the same inode, and our usual pattern on inode
instantiation is:

    inode->i_atime = inode->i_mtime = inode_set_ctime_current(inode);

...which should result in all of inode's timestamps being synchronized.

> 
> neomutt mbox/mbox.c
> https://sources.debian.org/src/neomutt/20220429+dfsg1-4.1/mbox/mbox.c/?hl=1820#L1820
> 
>   if (m->newly_created && ((st.st_ctime != st.st_mtime) || (st.st_ctime != st.st_atime)))
>     m->newly_created = false;
> 

Ditto here.

> 
> screen logfile.c
> https://sources.debian.org/src/screen/4.9.0-4/logfile.c/?hl=130#L130
> 
>   if ((!s->st_dev && !s->st_ino) ||             /* stat failed, that's new! */
>       !s->st_nlink ||                           /* red alert: file unlinked */
>       (s->st_size < o.st_size) ||               /*           file truncated */
>       (s->st_mtime != o.st_mtime) ||            /*            file modified */
>       ((s->st_ctime != o.st_ctime) &&           /*     file changed (moved) */
>        !(s->st_mtime == s->st_ctime &&          /*  and it was not a change */
>          o.st_ctime < s->st_ctime)))            /* due to delayed nfs write */
>   {
> 

This one is really weird. You have two different struct stat's, "o" and
"s". I assume though that these should be stat values from the same
inode, because otherwise this comparison would make no sense:

      ((s->st_ctime != o.st_ctime) &&           /*     file changed (moved) */

In general, we can never contrive to ensure that the ctime of two
different inodes are the same, since that is always set by the kernel to
the current time, and you'd have to ensure that they were created within
the same jiffy (at least with today's code).

> nemo libnemo-private/nemo-vfs-file.c
> https://sources.debian.org/src/nemo/5.6.5-1/libnemo-private/nemo-vfs-file.c/?hl=344#L344
> 
> 		/* mtime is when the contents changed; ctime is when the
> 		 * contents or the permissions (inc. owner/group) changed.
> 		 * So we can only know when the permissions changed if mtime
> 		 * and ctime are different.
> 		 */
> 		if (file->details->mtime == file->details->ctime) {
> 			return FALSE;
> 		}
> 

Ditto here with the first examples. This involves comparing timestamps
on the same inode, which should be fine.

> 
> While looking for more examples, I found a perl test that seems to suggest
> that at least Solaris, AFS, AmigaOS, DragonFly BSD do as you suggest:
> https://sources.debian.org/src/perl/5.36.0-7/t/op/stat.t/?hl=158#L140
> 

(I kinda miss Perl. I wrote a bunch of stuff in it in the 90's and early
naughties)

I think this test is supposed to be testing whether the mtime changes on
link() ?

-----------------8<----------------
    my($nlink, $mtime, $ctime) = (stat($tmpfile))[$NLINK, $MTIME, $CTIME];

[...]


        skip "Solaris tmpfs has different mtime/ctime link semantics", 2
                                     if $Is_Solaris and $cwd =~ m#^/tmp# and
                                        $mtime && $mtime == $ctime;
-----------------8<----------------

...again, I think this would be ok too since it's just comparing the
mtime and ctime of the same inode. Granted this is a Solaris-specific
test, but Linux would be fine here too.

So in conclusion, I don't think this patchset will cause problems with
any of the above code.
-- 
Jeff Layton <jlayton@kernel.org>