From owner-svn-src-head@freebsd.org Sat Jul 14 16:57:34 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id AAD521032D89; Sat, 14 Jul 2018 16:57:34 +0000 (UTC) (envelope-from sbruno@freebsd.org) Received: from mail.ignoranthack.me (ignoranthack.me [199.102.79.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3AA7180AAF; Sat, 14 Jul 2018 16:57:34 +0000 (UTC) (envelope-from sbruno@freebsd.org) Received: from [192.168.0.6] (67-0-234-146.albq.qwest.net [67.0.234.146]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: sbruno@ignoranthack.me) by mail.ignoranthack.me (Postfix) with ESMTPSA id CE6421AF636; Sat, 14 Jul 2018 09:12:03 +0000 (UTC) Subject: Re: svn commit: r336282 - head/sys/netinet To: John Baldwin , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201807141619.w6EGJk2Y016469@repo.freebsd.org> <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org> From: Sean Bruno Openpgp: preference=signencrypt Autocrypt: addr=sbruno@freebsd.org; prefer-encrypt=mutual; keydata= xsBNBFk+0UEBCADaf4bgxxKvMOhRV5NPoGWRCCGm49d6+1VFNlQ77WsY/+Zvf95TPULdRlnG w648KfxWt7+O3kdKhdRwnqlXWC7zA2Qt0dRE1yIqOGJ4jp4INvp/bcxWzgr0aoKOjrlnfxRV bh+s0rzdZt6TsNL3cVYxkC8oezjaUkHdW4mFJU249U1QJogkF8g0FeKNfEcjEkwJNX6lQJH+ EzCWT0NCk6J+Xyo+zOOljxPp1OUfdvZi3ulkU/qTZstGVWxFVsP8xQklV/y3AFcbIYx6iGJ4 5L7WuB0IWhO7Z4yHENr8wFaNYwpod9i4egX2BugbrM8pOfhN2/qqdeG1L5LMtXw3yyAhABEB AAHNN1NlYW4gQnJ1bm8gKEZyZWVCU0QgRGV2ZWxvcGVyIEtleSkgPHNicnVub0BmcmVlYnNk Lm9yZz7CwJQEEwEKAD4WIQToxOn4gDUE4eP0ujS95PX+ibX8tgUCWT7RQQIbAwUJBaOagAUL CQgHAwUVCgkICwUWAwIBAAIeAQIXgAAKCRC95PX+ibX8ttKTCACFKzRc56EBAlVotq02EjZP SfX+unlk6AuPBzShxqRxeK+bGYVCigrYd1M8nnskv0dEiZ5iYeND9HIxbpEyopqgpVTibA7w gBXaZ7SOEhNX1wXwg14JrralfSmPFMYni+sWegPMX/zwfAsn1z4mG1Nn44Xqo3o7CfpkMPy6 M5Bow2IDzIhEYISLR+urxs74/aHU35PLtBSDtu18914SEMDdva27MARN8mbeCDbuJVfGCPWy YHuy2t+9u2Zn5Dd+t3sBXLM9gpeaMm+4x6TNPpESygbVdh4tDdjVZ9DK/bWFg0kMgfZoaq6J l0jNsQXrZV3bzYNFbVw04pFcvA2GIJ7xzsBNBFk+0UEBCADIXBmQOaKMHGbc9vwjhV4Oj5aZ DdhNedn12FVeTdOXJvuTOusgxS29lla0RenHGDsgD08UiFpasBXWq/E+BhQ19d+iRbLLR17O KKc1ZGefoVbLARLXD68J5j4XAyK+6k2KqBLlqzAEpHTzsksM9naARkVXiEVcrt6ciw0FSm8n kuK3gDKKe93XfzfP+TQdbvvzJc7Fa+appLbXz61TM1aikaQlda8bWubDegwXbuoJdB34xU1m yjr/N4o+raL0x7QrzdH+wwgrTTo+H4S2c1972Skt5K5tbxLowfHicRl23V8itVQr3sBtlX4+ 66q+Apm7+R36bUS/k+G45Sp6iPpxABEBAAHCwHwEGAEKACYWIQToxOn4gDUE4eP0ujS95PX+ ibX8tgUCWT7RQQIbDAUJBaOagAAKCRC95PX+ibX8trrIB/9Pljqt/JGamD9tx4dOVmxSyFg9 z2xzgklTLuDgS73MM120mM7ao9AQUeWiSle/H0UCK7xPOzC/aeUC4oygDQKAfkkNbCNTo3+A qDjBRA8qx0e9a/QjDL+RFgD4L5kLT4tToY8T8HaBp8h03LBfk510IaI8oL/Jg7vpM3PDtJMW tUi2H+yNFmL3NfM2oBToWKLFsoP54f/eeeImrNnrlLjLHPzqS+/9apgYqX2Jwiv3tHBc4FTO GuY8VvF7BpixJs8Pc2RUuCfSyodrp1YG1kRGlXAH0cqwwr0Zmk4+7dZvtVQMCl6kS6q1+84q JwtItxS2eXSEA4NO0sQ3BXUywANh Message-ID: Date: Sat, 14 Jul 2018 10:57:32 -0600 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="3Gncr6iJ5BhrSfBA5oFx32w6nrm7oAy7g" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 14 Jul 2018 16:57:34 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --3Gncr6iJ5BhrSfBA5oFx32w6nrm7oAy7g Content-Type: multipart/mixed; boundary="mQotTGm8t5PiUhpisxN0wwEcLVIFPAJdw"; protected-headers="v1" From: Sean Bruno To: John Baldwin , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Message-ID: Subject: Re: svn commit: r336282 - head/sys/netinet References: <201807141619.w6EGJk2Y016469@repo.freebsd.org> <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org> In-Reply-To: <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org> --mQotTGm8t5PiUhpisxN0wwEcLVIFPAJdw Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07/14/18 10:37, John Baldwin wrote: > 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() >> =20 >> Submitted by: Jason Eggleston >> 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 >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D >> --- 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 =3D ulmin(inp->inp_options->m_len, sopt->sopt_v= alsize); >> + struct mbuf *options =3D malloc(len, M_TEMP, M_WAITOK); >> + INP_RLOCK(inp); >> + bcopy(inp->inp_options, options, len); >> + INP_RUNLOCK(inp); >> error =3D sooptcopyout(sopt, >> - mtod(inp->inp_options, >> + mtod(options, >> char *), >> - inp->inp_options->m_len); >> - else >> + len); >> + free(options, M_TEMP); >> + } else >> sopt->sopt_valsize =3D 0; >> break; >=20 > You can't malloc an mbuf, and you don't really want an mbuf here anyway= =2E > Also, style(9) doesn't normally assign values when a variable is create= d. > I would perhaps have done something like this: >=20 > if (inp->inp_options) { > void *options; > u_long len; >=20 > INP_RLOCK(inp); > len =3D ulmin(inp->inp_options->m_len, sopt->sopt_valsize); > INP_RUNLOCK(inp); > options =3D malloc(len, M_TEMP, M_WAITOK); > INP_RLOCK(inp); > len =3D ulmin(inp->inp_options->m_len, len); > m_copydata(inp->inp_options, 0, len, options); > INP_RUNLOCK(inp); > error =3D sooptcopyout(sopt, options, len); > free(options, M_TEMP); > } >=20 > 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= =2E > 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). >=20 > 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 architectur= es > rather than panicking since we don't tend to invalidate KVA if you lose= > that race). >=20 Shall I revert and rethink? sean --mQotTGm8t5PiUhpisxN0wwEcLVIFPAJdw-- --3Gncr6iJ5BhrSfBA5oFx32w6nrm7oAy7g Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQGTBAEBCgB9FiEE6MTp+IA1BOHj9Lo0veT1/om1/LYFAltKK3xfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldEU4 QzRFOUY4ODAzNTA0RTFFM0Y0QkEzNEJERTRGNUZFODlCNUZDQjYACgkQveT1/om1 /LYJgQgAn+hdOLTO4jdx7idzur4MvqgLBRd5kM0OkQUX571d0eIWkOLohsAu8E8e XwKegzBFGZvITzQV0ZWXLjzWB2RvGwcgtNdQWD2NBeoRYENH4cFjHf1T0cuTqO5t h9JbRNjQFJiNTG8eQh+au/BtInNZ8lLtBWQlIbVkrHQmwo7Yxn52qkHDeZ4mMVOx 72+uD6uxuI5G2pSEeQwrev/wxx+B7Y72J/HC97eAZYwH86t16DFbyH8OTlCAwXKh t4y4AKQZUoeWWrbeGErdyb3OsHP76ivJPWtLsZqWPaWBREk5uNh/8PjC0C2mgoAM eKOGzb2v5XSCtq5IZNXUAiLKfJ03kA== =+yx6 -----END PGP SIGNATURE----- --3Gncr6iJ5BhrSfBA5oFx32w6nrm7oAy7g--