sys/kern_descrip.c BUG fixes 4.[23]BSD VAXen and non-VAXens +FIX

[ryu at kiwi.UUCP]

4.2BSD, 4.3BSD, VAXen and non-VAXen fixes in				+FIX

	sys/sys/kern_descrip.c
	sys/sys/sys_inode.c
	sys/sys/sys_socket.c

by	Ryu Koriyama, Waseda Univ. Sci. & Eng.

	It is possible to close a file descriptor more than once,
	or otherwise use it after it has been closed, and
	possibly after another process has reallocated it or
	reallocated the in-core inode it points to.

	This has been first reported by
		Jeff Mogul <mogul at navajo> 13 Dec 83
	and temporally fix made by
		Brian Thomson decvax!uthub!tompson 13 Feb 84

REPEAT BY:
	First install Jeff Mogul's firewall panic in
	closef() [ref. unix-wizards <14552 at sri-arpa.UUCP> Dec 12 83]
	you may have already seen this. If not, then put it in first
	Like so:
		kern_descrip.c, closef(), before
			(*fp->f_ops->fo_close)(fp);
		insert
			if(fp->f_count < 1)
				panic("closef: f_count < 1");

	If you run the system heavily, you will soon meet a panic.

FIX:
	What is happening is,

		1.	Usr calls close() via syscall

		2.	In close() it call closef()

		3.	Closef() calls (*fp->f_ops->fo_close)() which
		  	may be ether ino_close() or soo_close()

		4.1.	In ino_close(), after iput(), ino_close() sets
			fp->f_count to 0, which means (struct file *)fp is
			no longer used and is now an open slot.

			This is because we no longer need fp.
			This is a bit strange since we are still passing
			fp to [cb]devsw[major(dev)].d_close(), but in
			ino_close(), a little after fp->f_count is set to 0,
			we are using fp as a variable to check that another
			inode for the same device isn't active.
			So what we are passing to [cb]devsw[].d_close() is
			garbage which is has fileNFILE in it (see the code).

		4.1.2.	Now suppose we block in [cb]devsw[].d_close().

		4.1.3.  Fp is now a free slot.
			Someone, via falloc, gets this file structure.
			This is because falloc assumes that a file structure
			which f_count field is 0 is a free slot.
			So now someone has this file structure.

		4.1.4.	Now the blocking [cb]devsw[].d_close() in 4.1.2 above
			has ended.
			We will now return to closef().

		4.1.5.	In closef(), as soon as we have returned, closef() will
			set the f_count field to 0 because he thinks that
			the file structure is now longer needed and he should
			free the slot.

			But, still, someone may be using that file structure!
			Sooner or later someone comes around and gets that
			file structure via falloc().

			Now we have a file structure which is shared by
			2 processes. We will soon get a mixed file.

		4.2.	Now then, in soo_close() we will use the f_data
			field in the file structure. So we can't give
			fp away.
			Soo_close() calls soclose() to do the works.

		4.2.1.	Soon after soclose() returns, soo_close()
			will set fp->f_data to 0.
			This is strange too.
				P.S.	I have to take a look at the code once
					more, but I am sure this is not needed,
					and is where the problem starts.
			Anyway soo_close() is not going to free the file
			structure slot.

		4.2.2.	Well as soon as we return to closef(), closef() will
			set the f_count to 0 and free the file structure slot.
			Here we are happily ever after.

	Now we know the scene.

	The problem is WHO is responsible to free the file structure slot.
	If closef() is responsible, ino_close() should not free it.
	If fileops.fo_close() is responsible, soo_close() should free it.
	Looking at the code, in ino_close(), there is the next line
		fp->f_count = 0;		/* XXX Should catch */
	So, I think, the author is asking fileops.fo_close() to be responsible
	in freeing the file structure slot.

	Here comes the bug.

	Now when we moved to 4.2BSD, we have got the socket.
	And we have got the ino_close() and soo_close() routine.
	But the soo_close() routine does not free the file structure slot.
	So we had to do it somewhere else.
	So the code went into closef(). The BUG is born.

	The FIX at last!

	Since the fileops.fo_close() routine should be responsible for
	freeing the file structure slot,
		1.	closef() should not set the f_count field to 0
		2.	all of the fileops.fo_close() routines should
			set the f_count field to 0 as soon as they have
			no need of the file structure slot.
	So the fix will be,
		1.	take out the fp->f_count=0; code out of closef();
		2.	put in the fp->f_count=0; code into the last line
			of soo_close() routine.

	This way we will be freeing the file structure slot as soon as
	we are finished with it, and we can have a free file structure as
	soon as possible.

	FIX:
		file:		sys/sys/kern_descrip.c
		routine:	closef()
		modify:
				}
				(*fp->f_ops->fo_close)(fp);
				fp->f_count = 0;
			}
		to:
				}
				(*fp->f_ops->fo_close)(fp);
			}
		file:		sys/sys_socket.c
		routine:	soo_close()
		modify:
				error = soclose((struct socket *)fp->f_data);
			fp->f_data = 0;
			return (error);
		}
		to:
				error = soclose((struct socket *)fp->f_data);
			fp->f_data = 0;		/* XXX Just in case... */
			fp->f_count = 0;
			return (error);
		}

	END OF FIX

		Ryu Koriyama
		Waseda Univ. Sci. & Eng.

		To contact:
			Foretune Co.
			R&D Div.
			Towa Building Room 55
			3-5-1 Suido
			Bunkyo-ku, Tokyo, Japan
			ZIP 112
			Phone 03-811-7861 (day and night)
		Or on the net:
			via JUNET
				ryu at psycho.kiwi.juice
				{somewhere}!titcca!kwmux!kiwi!psycho!ryu

P.S.	This fix was posted via this channel since the VAX I am using is
	not connected to any net.
	But, you can still contact me via this address.

P.P.S.	This fix is still needed in 4.3BSD and other codes than VAXen ones.
	We have this code running on heavily loaded VAXs but have no
	problem. At least I am sure this is not an ENBUG.

P.P.P.S. Special thanks to
		Mr. Yoshihiro Magara yoshih-m at ascii.junet
		Mr. Masahiko Kiso masahi-k at ascii.junet
	 for the support and testing of the debug.