Date: Sat, 14 Jul 2018 09:37:09 -0700 From: John Baldwin <jhb@FreeBSD.org> To: Sean Bruno <sbruno@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r336282 - head/sys/netinet Message-ID: <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org> In-Reply-To: <201807141619.w6EGJk2Y016469@repo.freebsd.org> References: <201807141619.w6EGJk2Y016469@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 7/14/18 9:19 AM, Sean Bruno wrote: > Author: sbruno > Date: Sat Jul 14 16:19:46 2018 > New Revision: 336282 > URL: https://svnweb.freebsd.org/changeset/base/336282 > > Log: > Fixup memory management for fetching options in ip_ctloutput() > > Submitted by: Jason Eggleston <jason@eggnet.com> > Sponsored by: Limelight Networks > Differential Revision: https://reviews.freebsd.org/D14621 > > Modified: > head/sys/netinet/ip_output.c > > Modified: head/sys/netinet/ip_output.c > ============================================================================== > --- head/sys/netinet/ip_output.c Sat Jul 14 16:06:53 2018 (r336281) > +++ head/sys/netinet/ip_output.c Sat Jul 14 16:19:46 2018 (r336282) > @@ -1256,12 +1256,18 @@ ip_ctloutput(struct socket *so, struct sockopt *sopt) > switch (sopt->sopt_name) { > case IP_OPTIONS: > case IP_RETOPTS: > - if (inp->inp_options) > + if (inp->inp_options) { > + unsigned long len = ulmin(inp->inp_options->m_len, sopt->sopt_valsize); > + struct mbuf *options = malloc(len, M_TEMP, M_WAITOK); > + INP_RLOCK(inp); > + bcopy(inp->inp_options, options, len); > + INP_RUNLOCK(inp); > error = sooptcopyout(sopt, > - mtod(inp->inp_options, > + mtod(options, > char *), > - inp->inp_options->m_len); > - else > + len); > + free(options, M_TEMP); > + } else > sopt->sopt_valsize = 0; > break; You can't malloc an mbuf, and you don't really want an mbuf here anyway. Also, style(9) doesn't normally assign values when a variable is created. I would perhaps have done something like this: if (inp->inp_options) { void *options; u_long len; INP_RLOCK(inp); len = ulmin(inp->inp_options->m_len, sopt->sopt_valsize); INP_RUNLOCK(inp); options = malloc(len, M_TEMP, M_WAITOK); INP_RLOCK(inp); len = ulmin(inp->inp_options->m_len, len); m_copydata(inp->inp_options, 0, len, options); INP_RUNLOCK(inp); error = sooptcopyout(sopt, options, len); free(options, M_TEMP); } The current code isn't doing what you think it is doing since the bcopy() copies the m_data pointer from 'inp_options' into 'options' so that 'options->m_data' points at the m_data buffer in the 'inp_options' mbuf. Thus, the mtod() returns a pointer into 'inp_options' just like the old code, so this commit doesn't change anything in terms of the previous race (it is still just as broken). Also, while INP_RLOCK isn't helpful in my version above for reading the 'm_len' member (it's racy to read and then malloc no matter what), you still need the lock to ensure the inp_options pointer itself is valid when you dereference it. Without the lock another thread might have freed inp_options out from under you and the unlocked dereference could panic (though it probably just reads garbage on most of our architectures rather than panicking since we don't tend to invalidate KVA if you lose that race). -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7bbbb553-fca5-9801-49aa-caf6df929c2f>