Re: process killed: text file modification

On Fri, Mar 17, 2017 at 03:10:57AM +0000, Rick Macklem wrote:
> Hope you don't mind a top post...
> Attached is a little patch you could test maybe?
> 
> rick
> ________________________________________
> From: owner-freebsd-current_at_freebsd.org <owner-freebsd-current_at_freebsd.org> on behalf of Rick Macklem <rmacklem_at_uoguelph.ca>
> Sent: Thursday, March 16, 2017 9:57:23 PM
> To: Dimitry Andric; Ian Lepore
> Cc: Gergely Czuczy; FreeBSD Current
> Subject: Re: process killed: text file modification
> 
> Dimitry Andric wrote:
> [lots of stuff snipped]
> > I'm also running into this problem, but while using lld.  I must set
> > vfs.timestamp_precision to 1 (e.g. sec + ns accurate to 1/HZ) on both
> > the client and the server, to make it work.
> >
> > Instead of GNU ld, lld uses mmap to write to the output executable.  If
> > this executable is more than one page, and resides on an NFS share,
> > running it will almost always result in "text file modification", if
> > vfs_timestamp_precision >= 2.
> >
> > A small test case: http://www.andric.com/freebsd/test-mmap-write.c,
> > which writes a simple "hello world" i386-freebsd executable file, using
> > the sequence: open() -> ftruncate() -> mmap() -> memcpy() -> munmap() ->
> > close().
> Hopefully Kostik will correct me if I have this wrong, but I don't believe any
> of the above syscalls guarantee that dirty pages have been flushed.
> At least for cases without munmap(), the writes of dirty pages can occur after
> the file descriptor is closed. I run into this in NFSv4, where there is a Close (NFSv4 one)
> that can't be done until VOP_INACTIVE().
> If you look in the NFS VOP_INACTIVE() { called ncl_inactive() } you'll see:
> if (NFS_ISV4(vp) && vp->v_type == VREG) {
> 237                     /*
> 238                      * Since mmap()'d files do I/O after VOP_CLOSE(), the NFSv4
> 239                      * Close operations are delayed until now. Any dirty
> 240                      * buffers/pages must be flushed before the close, so that the
> 241                      * stateid is available for the writes.
> 242                      */
> 243                     if (vp->v_object != NULL) {
> 244                             VM_OBJECT_WLOCK(vp->v_object);
> 245                             retv = vm_object_page_clean(vp->v_object, 0, 0,
> 246                                 OBJPC_SYNC);
> 247                             VM_OBJECT_WUNLOCK(vp->v_object);
> 248                     } else
> 249                             retv = TRUE;
> 250                     if (retv == TRUE) {
> 251                             (void)ncl_flush(vp, MNT_WAIT, NULL, ap->a_td, 1, 0);
> 252                             (void)nfsrpc_close(vp, 1, ap->a_td);
> 253                     }
> 254             }
> Note that nothing like this is done for NFSv3.
> What might work is implementing a VOP_SET_TEXT() vnode op for the NFS
> client that does most of the above (except for nfsrpc_close()) and then sets
> VV_TEXT.
> --> That way, all the dirty pages will be flushed to the server before the executable
>      starts executing.
> 
> > Running this on an NFS share, and then attempting to run the resulting
> > 'helloworld' executable will result in the "text file modification"
> > error, and it will be killed.  But if you simply copy the executable to
> > something else, then it runs fine, even if you use -p to retain the
> > properties!
> >
> > IMHO this is a rather surprising problem with the NFS code, and Kostik
> > remarked that the problem seems to be that the VV_TEXT flag is set too
> > early, before the nfs cache is invalidated.  Rick, do you have any ideas
> > about this?
> I don't think it is the "nfs cache" that needs invalidation, but the dirty
> pages written by the mmap'd file need to be flushed, before the VV_TEXT
> is set, I think?
> 
> If Kostik meant "attribute cache" when he said "nfs cache", I'll note that the
> cached attributes (including mtime) are updated by the reply to every write.
> (As such, I think it is the writes that must be done before setting VV_TEXT
>   that is needed.)
> 
> It is a fairly simple patch to create. I'll post one to this thread in a day or so
> unless Kostik thinks this isn't correct and not worth trying.
> 

I think that the patch is in right direction, but I am not convinced
that it is enough. What we need is to ensure that mtime on server does
not change after VV_TEXT is set. Dirty pages indeed would cause the
mtime update on flush, but wouldn't dirty buffers writes cause the same
issue ? In other words, I think that enchanced VOP_SET_TEXT() for nfs
must flush everything to ensure that mtime on server would not change
due to further actions by this machine' nfs client.