Date: Tue, 21 Mar 2017 00:18:18 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: Dimitry Andric <dim@FreeBSD.org>, Ian Lepore <ian@FreeBSD.org>, Gergely Czuczy <gergely.czuczy@harmless.hu>, FreeBSD Current <freebsd-current@freebsd.org> Subject: Re: process killed: text file modification Message-ID: <20170320221818.GM43712@kib.kiev.ua> In-Reply-To: <YTXPR01MB0189F47B6A23C10BFE8A85E6DD3B0@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM> References: <FF55DB37-4A6B-4D88-B201-B3BCA1A11E87@FreeBSD.org> <YTXPR01MB01898D49933A82170FCAB7A0DD390@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM> <YTXPR01MB018944EF4248402AD421D825DD390@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM> <20170317083605.GQ16105@kib.kiev.ua> <YTXPR01MB0189F7147A7C5C5F8C56B2F1DD390@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM> <20170317141917.GS16105@kib.kiev.ua> <D0770019-3EEA-45D2-A751-18DF1B274F90@FreeBSD.org> <YTXPR01MB0189FBB6CF664653C1162936DD390@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM> <20170318032150.GW16105@kib.kiev.ua> <YTXPR01MB0189F47B6A23C10BFE8A85E6DD3B0@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Mar 19, 2017 at 08:52:50PM +0000, 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?
Yes, I have to somewhat retract my claims, but then I have another set
of surprises.
I realized (remembered) that nfs has its own VOP_PUTPAGES() method.
Implementation seems to directly initiate write RPC request using the
pages as the source buffer. I do not see anything in the code which
would mark the buffers, which possibly contain the pages, as clean,
or mark the buffer range as undirty.
At very least, this might cause unnecessary double-write of the same
data. I am not sure if it could cause coherency issues between data
written using mappings and write(2). Also, both vm_object_page_clean()
and vfs_busy_pages() only ensure the shared-busy state on the pages,
so write(2) can occur while pageout sends data to server, causing
'impossible' content transmitted over the wire.
Could you, please, explain the reasons for such implementation of
ncl_putpage() ?
> --> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170320221818.GM43712>