Date: Sat, 14 Jul 2018 10:57:32 -0600 From: Sean Bruno <sbruno@freebsd.org> To: John Baldwin <jhb@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: <e3dbc55b-a085-ce92-ae89-4db0a7287113@freebsd.org> In-Reply-To: <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org> References: <201807141619.w6EGJk2Y016469@repo.freebsd.org> <7bbbb553-fca5-9801-49aa-caf6df929c2f@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --3Gncr6iJ5BhrSfBA5oFx32w6nrm7oAy7g Content-Type: multipart/mixed; boundary="mQotTGm8t5PiUhpisxN0wwEcLVIFPAJdw"; protected-headers="v1" From: Sean Bruno <sbruno@freebsd.org> To: John Baldwin <jhb@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Message-ID: <e3dbc55b-a085-ce92-ae89-4db0a7287113@freebsd.org> 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 <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 >> =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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?e3dbc55b-a085-ce92-ae89-4db0a7287113>