Date: Sat, 23 Jan 2016 23:56:40 -0800 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Lawrence Stewart <lstewart@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: <20160124075640.GD1004@FreeBSD.org> In-Reply-To: <56A3271F.6010904@freebsd.org> References: <201601212253.u0LMrC7B016136@repo.freebsd.org> <56A1C9F6.2010708@freebsd.org> <20160122180411.GA1004@FreeBSD.org> <56A3271F.6010904@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Lawrence,
On Sat, Jan 23, 2016 at 06:09:19PM +1100, Lawrence Stewart wrote:
L> > Is that the race you refer to?
L>
L> No, the TCP_CONGESTION refactoring i.e. this commit, introduced races in
L> the get and set cases. I guess I didn't provide enough context in
L> Crucible, so here goes...
L>
L> The post refactoring get code is now:
L>
L> case TCP_CONGESTION:
L> INP_WUNLOCK(inp);
L> error = sooptcopyout(sopt, CC_ALGO(tp)->name, TCP_CA_NAME_MAX);
L> break;
L>
L> Consider that tp is using cc_blah and that the cc_blah module is
L> unloaded as the copy out is happening. By not holding the INP lock, the
L> CC module unload code is able to walk the list of active connections,
L> find this connection is using cc_blah, acquire the INP lock, switch this
L> connection to cc_newreno, release the lock and finally unload the
L> cc_blah module, rendering the pointer passed in to sooptcopyout garbage.
L> See cc_deregister_algo() in cc.c and tcp_ccalgounload() in tcp_subr.c
L> for details related to CC module unload.
Understood. Can you please review this patch? It basicly shifts INP_WLOCK
earlier in the SOPT_SET case, and reverts to old code the SOPT_GET case.
It returns back the stack based string buffer.
--
Totus tuus, Glebius.
[-- Attachment #2 --]
Index: tcp_usrreq.c
===================================================================
--- tcp_usrreq.c (revision 294658)
+++ tcp_usrreq.c (working copy)
@@ -1479,7 +1479,7 @@ tcp_default_ctloutput(struct socket *so, struct so
u_int ui;
struct tcp_info ti;
struct cc_algo *algo;
- char *buf;
+ char *pbuf, buf[TCP_CA_NAME_MAX];
/*
* For TCP_CCALGOOPT forward the control to CC module, for both
@@ -1488,22 +1488,22 @@ tcp_default_ctloutput(struct socket *so, struct so
switch (sopt->sopt_name) {
case TCP_CCALGOOPT:
INP_WUNLOCK(inp);
- buf = malloc(sopt->sopt_valsize, M_TEMP, M_WAITOK | M_ZERO);
- error = sooptcopyin(sopt, buf, sopt->sopt_valsize,
+ pbuf = malloc(sopt->sopt_valsize, M_TEMP, M_WAITOK | M_ZERO);
+ error = sooptcopyin(sopt, pbuf, sopt->sopt_valsize,
sopt->sopt_valsize);
if (error) {
- free(buf, M_TEMP);
+ free(pbuf, M_TEMP);
return (error);
}
INP_WLOCK_RECHECK(inp);
if (CC_ALGO(tp)->ctl_output != NULL)
- error = CC_ALGO(tp)->ctl_output(tp->ccv, sopt, buf);
+ error = CC_ALGO(tp)->ctl_output(tp->ccv, sopt, pbuf);
else
error = ENOENT;
INP_WUNLOCK(inp);
if (error == 0 && sopt->sopt_dir == SOPT_GET)
- error = sooptcopyout(sopt, buf, sopt->sopt_valsize);
- free(buf, M_TEMP);
+ error = sooptcopyout(sopt, pbuf, sopt->sopt_valsize);
+ free(pbuf, M_TEMP);
return (error);
}
@@ -1600,12 +1600,10 @@ unlock_and_done:
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);
+ if (error)
break;
- }
+ INP_WLOCK_RECHECK(inp);
CC_LIST_RLOCK();
STAILQ_FOREACH(algo, &cc_list, entries)
if (strncmp(buf, algo->name,
@@ -1612,12 +1610,11 @@ unlock_and_done:
TCP_CA_NAME_MAX) == 0)
break;
CC_LIST_RUNLOCK();
- free(buf, M_TEMP);
if (algo == NULL) {
+ INP_WUNLOCK(inp);
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.
@@ -1786,9 +1783,10 @@ unlock_and_done:
error = sooptcopyout(sopt, &ti, sizeof ti);
break;
case TCP_CONGESTION:
+ bzero(buf, sizeof(buf));
+ strlcpy(buf, CC_ALGO(tp)->name, TCP_CA_NAME_MAX);
INP_WUNLOCK(inp);
- error = sooptcopyout(sopt, CC_ALGO(tp)->name,
- TCP_CA_NAME_MAX);
+ error = sooptcopyout(sopt, buf, TCP_CA_NAME_MAX);
break;
case TCP_KEEPIDLE:
case TCP_KEEPINTVL:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160124075640.GD1004>