Re: process killed: text file modification

On 2017. 03. 19. 21:52, Rick Macklem wrote:
> Kostik wrote:
> [stuff snipped]
>>>>> Dirty pages are flushed by writes, so if we have a set of dirty pages and
>>>>> async vm_object_page_clean() is called on the vnode' vm_object, we get
>>>>> a bunch of delayed-write AKA dirty buffers.  This is possible even after
>>>>> VOP_CLOSE() was done, e.g. by syncer performing regular run involving
>>>>> vfs_msync().
>>> When I was talking about ncl_flush() above, I was referring to buffer cache
>>> buffers written by a write(2) syscall, not the case of mmap'd pages.
>> But dirty buffers can appear on the vnode queue due to dirty pages msyncing
>> by syncer, for instance.
> Ok, just to clarify this, in case I don't understand it...
> - You aren't saying that anything will be added to v_bufobj.bo_dirty.bv_hd by
>    vfs_msync() or similar, after VOP_CLOSE(), right?
> --> ncl_flush() { was called nfs_flush() in the old NFS client } only deals with
>       "struct buf's" hanging off v_bufobj.bo_dirty.bv_hd, so I don't see a use for
>       it in the patch.
>
> As for pages added to v_bufobj.bo_object...the patch assumes that the process
> that was writing the executable file mmap'd is done { normally exited } before
> the exec() syscall occurs. If it is still dirtying pages when the exec() occurs, then
> failing with "Text file modified" seems correct to me. As you mentioned, another
> client can do this to the file anyhow.
>
> My understanding is that vm_object_page_clean() will get all the dirty pages written
> back to the server at that point and if that is done in VOP_SET_TEXT() as this patch
> does, what more can the NFS client do?
>
> [more stuff snipped]
>> Syncer does not open the vnode inside the vfs_msync() operations.
> Ok, but this doesn't put "struct buf's" on v_bufobj.bo_dirty.bv_hd. Am I right?
> (When I said "buffers". I meant "struct buf's" under bo_dirty, not stuff under
>   v_bufobj.bo_object.)
>
>> We do track writeability to the file, and do not allow execution if there is
>> an active writer, be it a file descriptor opened for write, or a writeable
>> mapping.  And in reverse, if the file is executed (VV_TEXT is set), then
>> we disallow opening the file for write.
> Yes, and that was why I figured doing this in VOP_SET_TEXT(), just before
> setting VV_TEXT, was the right place to do it.
> [more stuff snipped]
>> Thanks for testing the patch. Now, if others can test it...rick
>>
> Again, hopefully others (especially the original reporter) will be able to
> test the patch, rick
Actually I want to test it, but you guys are so vehemently discussing 
it, I thought it would be better to do so, once you guys settled your 
analysis on the code. Also, me not having the problem occurring, I don't 
think would mean it's solved, since that would only mean, the codepath 
for my specific usecase works. There might be other things there as 
well, what I don't hit.

Let me know which patch should I test, and I will see to it in the next 
couple of days, when I get the time to do it.

Regards,
-czg