Re: [9fans] Race condition in /sys/src/9/pc/trap.c?

[cinap_lenrek@gmx.de]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

very interesting. thanks for the sum up.

but my testcase crashes a uniprocessor system, so here is no
waiting for mmuflushes on other processors going on.

any other process that shares the segment and was suspended
in the kernel may potentialy hold a pointer to that freed memory
area of the segment and may cause a fault in the kernel on resume.

process1:
	in kernel:
		read(buf)
			validaddr(buf)
			...
			*context switch*

process2:
	in kernel:
		...
			ibrk()
				mfreeseg()
					procflushseg()
				flushmmu()
		...
		*context switch*

process1:
	still in kernel:
		memmove(buf, ...)
		*fault*
		trap()
			fault386()
				fault() => -1
				if(!user){
					panic()
					*panic*
				}
				...
				postnote()


we cant really wait for all processes sharing the segment to be
in userspace as they may already be waiting in a long blocking
syscall.

how do we fix this?

--
cinap

[-- Attachment #2: Type: message/rfc822, Size: 6164 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 2077 bytes --]

this resulted in a little side discussion.
to save someone else from having to break a strong oath about 9fans,
i'll sum it up. the existing code handles this situation.

when several processes share a segment, and any one of them
decides to shrink the segment, all the processes must see
the change before the pages are made available for re-use.
any one of them could have any of those pages currently in their mmu state.
and any  must be removed from the mmu state local to the process,
to ensure that no further access to the pages is possible before they are freed.
the critical point is that it's irrelevant whether traps or syscalls are
involved: ordinary store instructions are clearly bad enough.

thus /sys/src/9/port/segment.c:/^mfreeseg contains (with s locked)
	/* flush this seg in all other processes */
	if(s->ref > 1)
		procflushseg(s);
and procflushseg finds all processes that share s, sets them all up
to flush their mmu states, and also sets any processor running such a process
to flush its state (that's picked up by a clock interrupt).

procflushseg will not proceed until all processes and processors that
might need to flush state have done so. (s remains locked throughout.)

after the flush, no process or processor can have a reference
to any of the pages in its mmu state.  it is safe to free them,
which mfreeseg does.

now, a process might still be executing a system call or some other trap
that might refer to that segment, to an address that's now been removed
by another process. to access the memory, it must ultimately issue
a load or a store instruction (even for syscalls, such as read or write).
that instruction will trap, because as described above there is no longer
a valid mmu mapping for that address within the process. normally, the
trap will find the right page in the software mmu structures and install the map.
in this case, however, it can't find the address, so it will raise an exception
in the process (ie, send a note). (all such searches are done with the
segment properly locked.)

[-- Attachment #2.1.2: Type: message/rfc822, Size: 1746 bytes --]

From: Charles Forsyth <forsyth@terzarima.net>
To: 9fans@9fans.net
Subject: Re: [9fans] Race condition in /sys/src/9/pc/trap.c?
Date: Thu, 30 Jul 2009 16:54:16 +0100
Message-ID: <1a242b28f6d149aec599a1175df52718@terzarima.net>

just stop processes with s->ref > 1 from freeing parts of s with ibrk.
it's not as if anything ever does in practice.