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
}
/*
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
