Date: Tue, 28 Jun 2016 11:58:56 +0200 From: Julien Charbon <jch@freebsd.org> To: Randall Stewart <rrs@netflix.com>, current@freebsd.org Cc: hselasky@FreeBSD.org, net@FreeBSD.org Subject: Re: panic with tcp timers Message-ID: <ae21858a-e162-6aad-1597-eeff614624c9@freebsd.org> In-Reply-To: <6E52CA6A-2153-4EF9-A3E1-97CB0D07EB28@freebsd.org> References: <20160617045319.GE1076@FreeBSD.org> <1f28844b-b4ea-b544-3892-811f2be327b9@freebsd.org> <20160620073917.GI1076@FreeBSD.org> <1d18d0e2-3e42-cb26-928c-2989d0751884@freebsd.org> <20160620095822.GJ1076@FreeBSD.org> <74bb31b7-a9f5-3d0c-eea0-681872e6f09b@freebsd.org> <18D94615-810E-4E79-A889-4B0CC70F9E45@netflix.com> <6E52CA6A-2153-4EF9-A3E1-97CB0D07EB28@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)
--V2vxqg2bse8BDAiFdbkRFrXdAgQxIDLnB
Content-Type: multipart/mixed; boundary="VSScAOvNUr1ikVRhBTpmu42CTd9EplESr"
From: Julien Charbon <jch@freebsd.org>
To: Randall Stewart <rrs@netflix.com>, current@freebsd.org
Cc: hselasky@FreeBSD.org, net@FreeBSD.org
Message-ID: <ae21858a-e162-6aad-1597-eeff614624c9@freebsd.org>
Subject: Re: panic with tcp timers
References: <20160617045319.GE1076@FreeBSD.org>
<1f28844b-b4ea-b544-3892-811f2be327b9@freebsd.org>
<20160620073917.GI1076@FreeBSD.org>
<1d18d0e2-3e42-cb26-928c-2989d0751884@freebsd.org>
<20160620095822.GJ1076@FreeBSD.org>
<74bb31b7-a9f5-3d0c-eea0-681872e6f09b@freebsd.org>
<18D94615-810E-4E79-A889-4B0CC70F9E45@netflix.com>
<6E52CA6A-2153-4EF9-A3E1-97CB0D07EB28@freebsd.org>
In-Reply-To: <6E52CA6A-2153-4EF9-A3E1-97CB0D07EB28@freebsd.org>
--VSScAOvNUr1ikVRhBTpmu42CTd9EplESr
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable
Hi Randall,
On 6/25/16 4:41 PM, Randall Stewart via freebsd-net wrote:
> Ok
>=20
> Lets try this again with my source changed to my @freebsd.net :-)
>=20
> Now I am also attaching a patch for you Gleb, this will take some pokin=
g to
> get in to your NF-head since it incorporates some changes we made earli=
er.
>=20
> I think this will fix the problem.. i.e. dealing with two locks in the =
callout system (which it was
> never meant to have done)..
>=20
> Note we probably can move the code to use the callout lock init now.. b=
ut lets see if this works
> on your setup on c096 and if so we can think about doing that.
Thanks for proposing a patch. I believe your patch will work with
callout lock init, but not without: You still have a use-after-free
issue on the tcpcb without callout lock init.
The case being subtle as usual, let me try to describe that could happen=
:
With your patch we have:
void
tcp_timer_keep(void *xtp)
{
struct tcpcb *tp =3D xtp;
struct tcptemp *t_template;
struct inpcb *inp;
CURVNET_SET(tp->t_vnet);
#ifdef TCPDEBUG
int ostate;
ostate =3D tp->t_state;
#endif
inp =3D tp->t_inpcb;
KASSERT(inp !=3D NULL, ("%s: tp %p tp->t_inpcb =3D=3D NULL", __fu=
nc__,
tp));
INP_WLOCK(inp);
if (callout_pending(&tp->t_timers->tt_keep) ### Use after free
of tp here
!callout_active(&tp->t_timers->tt_keep)) {
INP_WUNLOCK(inp);
CURVNET_RESTORE();
return;
}
...
The use-after-free scenario:
[CPU 1] the callout fires, tcp_timer_keep entered
[CPU 1] blocks on INP_WLOCK(inp);
[CPU 2] schedules tcp_timer_keep with callout_reset()
[CPU 2] tcp_discardcb called
[CPU 2] tcp_timer_keep callout successfully canceled
[CPU 2] tcpcb freed
[CPU 1] unblocks, the tcpcb is used
Then the tcpcb will used just after being freed... Might also crash or
not depending in the case.
Extra notes:
o The invariant I see here is: The "callout successfully canceled"
step should never happen when "the callout is currently being executed".
o Solutions I see to enforce this invariant:
- First solution: Use callout lock init with inp lock, your patch
seems to permit that now.
- Second solution: Change callout_async_drain() behavior: It can
return 0 (fail) when the callout is currently being executed (no matter
what).
- Third solution: Don't trust callout_async_drain(callout) return
value of 1 (success) if the previous call of callout_reset(callout)
returned 0 (fail). That was the exact purpose of r284261 change, but
this solution is also step backward in modernization of TCP
timers/callout...
https://svnweb.freebsd.org/base/stable/10/sys/netinet/tcp_timer.c?r1=3D28=
4261&r2=3D284260&pathrev=3D284261
Hopefully my description is clear enough...
--
Julien
--VSScAOvNUr1ikVRhBTpmu42CTd9EplESr--
--V2vxqg2bse8BDAiFdbkRFrXdAgQxIDLnB
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - https://gpgtools.org
iQEcBAEBCgAGBQJXckpqAAoJEKVlQ5Je6dhxbn8IAK+iSIVBRUUwNNolJFmSx47O
Y1qySRcEb04Q8qxTZrT72/0FDRyFdMydhTTKh3yifXnTEGeg+wWKomkDNP8RDhgX
96xwXhzos+Y50PEbeKy78/kAZG8UmFaSGRMCDyvUHeTBI9TIRdDjZzJzCgrVqLot
a54sCW/+Ud1tXYUO0HEdJqaMWMdAre4Xsn7QNGFF7eY0ewmFj6vbA7VST35SbRnw
vP+Oy2VBPb2otqKY+FYYHeUi6gRMs+Nsen0K+hegbokWxBRXWPhft9WuSiz3heTI
juMOCALkH/D2lrHTxVkoR3+4+1fTZ9LkmBIaoA8mgH6UbeCvLJLcaODIpvwsVYA=
=j5ic
-----END PGP SIGNATURE-----
--V2vxqg2bse8BDAiFdbkRFrXdAgQxIDLnB--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?ae21858a-e162-6aad-1597-eeff614624c9>