Subject: better diagnostics for get*name(2)....
To: NetBSD Kernel Technical Discussion List <tech-kern@NetBSD.ORG>
From: Greg A. Woods <woods@weird.com>
List: tech-kern
Date: 05/21/2002 01:40:21
Recent experiments with a daemon that calls getpeername() and
getsockname() for every connection revealed that short-lasting
connections which are reset before these calls can occur willk result in
ENOTCONN from the former but EINVAL from the latter.  While ENOTCONN is
usually sufficient for diagnosis, EINVAL is a very nasty overloading of
a far more critical error.

In this scenario FreeBSD-4.x appears to now return ECONNRESET for both
calls, and this seems to be far more appropriate.

The original 4.4BSD code marks this return code with an /* XXX */
comment.  Unfortunately Stevens (in TCP/IP Illust. Vol.2) said nothing
about this notation, nor the choice of return code.  This comment was
"silently" elided from the NetBSD source along with a bunch of other
not-so-minor changes that go by the log entry "Minor changes."

I'd like to propose a "fix" for this situation in NetBSD, but there are
now tremendous differences in the code between NetBSD and FreeBSD and
I'm unsure as to where exactly the test for "connectedness" should be
made in NetBSD (which in this area still matches the 4.4BSD code).

I think the following (as yet untested) change will at least make
NetBSD's get*name() calls behave more similarly to FreeBSD (perhaps even
"more correctly" in that ENOTCONN won't be returned if a connection had
been set up and is now fully shut down and the PCB is released).  If
someone can let me know whether I'm on the right track or not (and if
not give me a bit of a pointer in the right direction) then I'll build a
kernel and actually try testing this fix.

Ultimately I think it would be best to not free the PCB until the socket
is closed -- or at least do what the FreeBSD commit-log for their fix
suggests and squirrel away the local and peer addresses somewhere safe.
It's really frustrating that these values are lost forever on demand of
the remote peer.  I want to know what shy client I was about to talk to!
Unfortunately delaying the PCB release seems like it would be quite
intrusive, even in the admittedly cleaner FreeBSD code.  On the other
hand also copying the local and peer addresses into the struct socket
seems wasteful and hackish.

Index: uipc_syscalls.c
===================================================================
RCS file: /cvs/NetBSD/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.1.1.10
diff -c -c -r1.1.1.10 uipc_syscalls.c
*** uipc_syscalls.c	19 Jun 2001 19:39:28 -0000	1.1.1.10
--- uipc_syscalls.c	21 May 2002 03:17:01 -0000
***************
*** 1034,1043 ****
  	if ((error = getsock(p->p_fd, SCARG(uap, fdes), &fp)) != 0)
  		return (error);
  	so = (struct socket *)fp->f_data;
- 	if ((so->so_state & (SS_ISCONNECTED|SS_ISCONFIRMING)) == 0) {
- 		error = ENOTCONN;
- 		goto out;
- 	}
  	error = copyin((caddr_t)SCARG(uap, alen), (caddr_t)&len, sizeof(len));
  	if (error)
  		goto out;
--- 1034,1039 ----
Index: tcp_usrreq.c
===================================================================
RCS file: /cvs/NetBSD/src/sys/netinet/tcp_usrreq.c,v
retrieving revision 1.1.1.8
diff -c -c -r1.1.1.8 tcp_usrreq.c
*** tcp_usrreq.c	25 Mar 2001 08:09:03 -0000	1.1.1.8
--- tcp_usrreq.c	21 May 2002 03:16:10 -0000
***************
*** 262,274 ****
  	if ((inp == 0 && in6p == 0) && req != PRU_ATTACH)
  #endif
  	{
! 		error = EINVAL;
  		goto release;
  	}
  #ifdef INET
  	if (inp) {
  		tp = intotcpcb(inp);
  		/* WHAT IF TP IS 0? */
  #ifdef KPROF
  		tcp_acounts[tp->t_state][req]++;
  #endif
--- 262,290 ----
  	if ((inp == 0 && in6p == 0) && req != PRU_ATTACH)
  #endif
  	{
! 		/*
! 		 * FreeBSD-4.x unconditionally returns ECONNRESET if the PCB is
! 		 * gone, but that seems too cavalier.  If the reason the PCB is
! 		 * gone is because our peer reset the connection then so_error
! 		 * will have benen set to ECONNRESET.  Can it ever be set to
! 		 * anything else in this case?  Otherwise we've not connected
! 		 * yet so ENOTCONN seems to be the right thing to return (and
! 		 * this will mimic the check that was used previously in
! 		 * ../kern/uipc_syscalls.c:sys_getpeername().
! 		 */
! 		if (so->so_error != 0)
! 			error = so->so_error;
! 		else if ((so->so_state & (SS_ISCONNECTED|SS_ISCONFIRMING)) == 0)
! 			error = ENOTCONN;
  		goto release;
  	}
  #ifdef INET
  	if (inp) {
  		tp = intotcpcb(inp);
  		/* WHAT IF TP IS 0? */
+ # ifdef DIAGNOSTIC
+ 		panic("tcp_usrreq: intotcpcp() found NULL struct tcpcb in per-proto PCB");
+ # endif
  #ifdef KPROF
  		tcp_acounts[tp->t_state][req]++;
  #endif


-- 
								Greg A. Woods

+1 416 218-0098;  <gwoods@acm.org>;  <g.a.woods@ieee.org>;  <woods@robohack.ca>
Planix, Inc. <woods@planix.com>; VE3TCP; Secrets of the Weird <woods@weird.com>