Date: Sat, 23 Jan 2016 18:09:19 +1100 From: Lawrence Stewart <lstewart@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r294536 - head/sys/netinet Message-ID: <56A3271F.6010904@freebsd.org> In-Reply-To: <20160122180411.GA1004@FreeBSD.org> References: <201601212253.u0LMrC7B016136@repo.freebsd.org> <56A1C9F6.2010708@freebsd.org> <20160122180411.GA1004@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 01/23/16 05:04, Gleb Smirnoff wrote: > On Fri, Jan 22, 2016 at 05:19:34PM +1100, Lawrence Stewart wrote: > L> On 01/22/16 09:53, Gleb Smirnoff wrote: > L> > Author: glebius > L> > Date: Thu Jan 21 22:53:12 2016 > L> > New Revision: 294536 > L> > URL: https://svnweb.freebsd.org/changeset/base/294536 > L> > > L> > Log: > L> > Refactor TCP_CONGESTION setsockopt handling: > L> > - Use M_TEMP instead of stack variable. > L> > - Unroll error handling, removing several levels of indentation. > L> > L> As noted privately elsewhere, this change introduces races with respect > L> to CC algorithm module unloading which will need to be fixed. Just > L> mentioning it again publicly so others are aware of it. > > In the code checked in the check for (CC_ALGO(tp)->ctl_output being > not NULL and the actual call are under the same lock. > > Is that the race you refer to? No, the TCP_CONGESTION refactoring i.e. this commit, introduced races in the get and set cases. I guess I didn't provide enough context in Crucible, so here goes... The post refactoring get code is now: case TCP_CONGESTION: INP_WUNLOCK(inp); error = sooptcopyout(sopt, CC_ALGO(tp)->name, TCP_CA_NAME_MAX); break; Consider that tp is using cc_blah and that the cc_blah module is unloaded as the copy out is happening. By not holding the INP lock, the CC module unload code is able to walk the list of active connections, find this connection is using cc_blah, acquire the INP lock, switch this connection to cc_newreno, release the lock and finally unload the cc_blah module, rendering the pointer passed in to sooptcopyout garbage. See cc_deregister_algo() in cc.c and tcp_ccalgounload() in tcp_subr.c for details related to CC module unload. The post refactoring set code is now: case TCP_CONGESTION: INP_WUNLOCK(inp); buf = malloc(TCP_CA_NAME_MAX, M_TEMP, M_WAITOK|M_ZERO); error = sooptcopyin(sopt, buf, TCP_CA_NAME_MAX, 1); if (error) { free(buf, M_TEMP); break; } CC_LIST_RLOCK(); STAILQ_FOREACH(algo, &cc_list, entries) if (strncmp(buf, algo->name, TCP_CA_NAME_MAX) == 0) break; CC_LIST_RUNLOCK(); free(buf, M_TEMP); if (algo == NULL) { error = EINVAL; break; } INP_WLOCK_RECHECK(inp); /* * We hold a write lock over the tcb so it's safe to * do these things without ordering concerns. */ if (CC_ALGO(tp)->cb_destroy != NULL) CC_ALGO(tp)->cb_destroy(tp->ccv); CC_ALGO(tp) = algo; /* * If something goes pear shaped initialising the new * algo, fall back to newreno (which does not * require initialisation). */ if (algo->cb_init != NULL && algo->cb_init(tp->ccv) != 0) { CC_ALGO(tp) = &newreno_cc_algo; /* * The only reason init should fail is * because of malloc. */ error = ENOMEM; } INP_WUNLOCK(inp); break; Consider that the application has requested to switch to "blah" congestion control and that the cc_blah module is unloaded as the request is being processed. We hold no locks, copy in the string "blah", grab the CC list lock, find cc_blah's algo struct in the list leaving its pointer stored in the "algo" variable and drop the CC list lock. Crucially at this point, we don't hold the INP lock, so once we drop the CC list lock, the CC module unload code is free to pull cc_blah from the CC module list, walk the list of active connections and switch any using cc_blah to cc_newreno and to unload the cc_blah module. The tp we're modifying hasn't switched to cc_blah yet so it won't be reverted to cc_newreno by the CC module unload code, which means the "algo" variable will be a garbage pointer by the time we get around to grabbing the INP lock at the INP_WLOCK_RECHECK() line of code. You have to ensure that the algo pointer is valid and that the INP lock is held at the time of assigning to the tp, as that will ensure that if you're racing with the unload code, the unload code will correctly switch the connection's algo back to cc_newreno once it's able to acquire the INP lock. Cheers, Lawrence
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?56A3271F.6010904>