soclose() & so->so_upcall() = race?

[Andre Oppermann]

Robert Watson wrote:
> 
> On Fri, 7 Mar 2008, Alexander Motin wrote:
> 
>> As I can see so_upcall() callback is called with SOCKBUF_MTX unlocked. 
>> It means that SB_UPCALL flag can be removed during call and socket can 
>> be closed and deallocated with soclose() while callback is running. Am 
>> I right or I have missed something? How in that situation socket 
>> pointer protected from being used after free?
> 
> 
> There are known problems with so_upcall and locking, including this 
> one.  The other problems include:
> 
> - The locking condition on entering the upcall depends on the invocation 
> point
>   and is inconsistent.
> 
> - The protection of the upcall field and flag are inconsistent.
> 
> - Consumers of so_upcall, such as socket accept filters, don't properly
>   respect the locking environment they run in.
> 
> - Some (all) accept filters produce nastily convoluted stack traces 
> involving
>   recursion across really odd combinations of sockets and protocols.  For
>   example, you can see soisdisconnected() calling soisconnected().
> 
> Some of this is inherent to the design of accept filters and so_upcall 
> and follows from using a single function pointer for many different 
> cases.  I have an 8.x todo list item to try and figure out how to make 
> so_upcall and accept filters rational in the context of fine-grained 
> locking, but I've not yet reached that point on my todo list.  I think 
> we can continue to incrementally hack so_upcall and accept filters to 
> fix many races, but the reality is that we need a more coherent model 
> for dealing with accept filters.  One idea that Colin Percival (I think) 
> suggested is that we separate socket upcalls from accept filters, and 
> that accept filters consistent of a predicate for completion rather than 
> directly invoking socket state transitions.  I've not explored the 
> implications, but think it might well be a good idea to avoid the weird 
> stack traces.

I've experimented with some changes to sowakeup() to better formalize it.
Code diff below.  Observations noted in the comments.

-- 
Andre


Index: uipc_sockbuf.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/uipc_sockbuf.c,v
retrieving revision 1.165
diff -u -p -r1.165 uipc_sockbuf.c
--- uipc_sockbuf.c	6 Sep 2006 21:59:36 -0000	1.165
+++ uipc_sockbuf.c	25 Feb 2007 15:22:32 -0000
@@ -173,11 +173,12 @@ sowakeup(struct socket *so, struct sockb

  	SOCKBUF_LOCK_ASSERT(sb);

-	selwakeuppri(&sb->sb_sel, PSOCK);
+#if 1
+	selwakeuppri(&sb->sb_sel, PSOCK);	/* removes thread from sleepq */
  	sb->sb_flags &= ~SB_SEL;
  	if (sb->sb_flags & SB_WAIT) {
  		sb->sb_flags &= ~SB_WAIT;
-		wakeup(&sb->sb_cc);
+		wakeup(&sb->sb_cc);		/* removes thread from sleepq too!? */
  	}
  	KNOTE_LOCKED(&sb->sb_sel.si_note, 0);
  	SOCKBUF_UNLOCK(sb);
@@ -188,6 +189,46 @@ sowakeup(struct socket *so, struct sockb
  	if (sb->sb_flags & SB_AIO)
  		aio_swake(so, sb);
  	mtx_assert(SOCKBUF_MTX(sb), MA_NOTOWNED);
+#else
+	/* Only wakeup if above low water mark. */
+	if (so->so_error == 0 && sb->sb_lowat < sbspace(sb))
+		return;
+	/* First run any upcalls which may process or modify the data. */
+	if (sb->sb_flags & SB_UPCALL) {
+		/*
+		 * Upcall tells us whether to do a wakeup or not.
+		 * Need a flag on the socket to tell select/poll and kqueue
+		 * to ignore socket buffer until cleared.
+		 * Maybe we should defer the upcall to a worker thread using
+		 * task queue?
+		 */
+		if ((*so->so_upcall)(so, sb, so->so_upcallarg, M_DONTWAIT)) {
+			sb->sb_flags |= SB_IGNORE;
+			SOCKBUF_UNLOCK(sb);
+			return;			/* don't wakeup, more work! */
+		}
+	}
+	/* KQueue notifications. */
+	KNOTE_LOCKED(&sb->sb_sel.si_note, 0);	/* issues wakeup */
+	/* Someone doing select/poll? */
+	if (sb->sb_flags & SB_SEL) {
+		selwakeuppri(&sb->sb_sel, PSOCK); /* removes thread from sleepq */
+		sb->sb_flags &= ~SB_SEL;
+	}
+	/* Someone waiting blocked on socket buffer. */
+	if (sb->sb_flags & SB_WAIT) {
+		wakeup(&sb->sb_cc);		/* removes thread from sleepq */
+		sb->sb_flags &= ~SB_WAIT;
+	}
+	/* Async IO notificatins. */
+	if (sb->sb_flags & SB_AIO)
+		aio_swake(so, sb);
+	/* Socket buffer won't be accessed anymore. */
+	SOCKBUF_UNLOCK(sb);
+	/* Send SIGIO or SIGURG to thread.  XXX: Is this still needed? */
+	if ((so->so_state & SS_ASYNC) && so->so_sigio != NULL)
+		pgsigio(&so->so_sigio, SIGIO, 0);
+#endif
  }

  /*