Date: Tue, 04 Oct 2016 14:27:19 -0700 From: Lohith Bellad <lohith.bellad@me.com> To: Hiren Panchasara <hiren@FreeBSD.org>, Gleb Smirnoff <glebius@FreeBSD.org>, svn-src-head@freebsd.org Cc: lohithbsd@gmail.com Subject: Re: svn commit: r306337 - head/sys/kern Message-ID: <E437A1A5-5BDB-4CA4-B98E-51BAF353A760@me.com> In-Reply-To: <20161004205945.GA50669@strugglingcoder.info> References: <201609261013.u8QADwrV002892@repo.freebsd.org> <20161004205352.GM23123@FreeBSD.org> <20161004205945.GA50669@strugglingcoder.info>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Gleb and Hiren, > On Oct 4, 2016, at 1:59 PM, Hiren Panchasara <hiren@FreeBSD.org> = wrote: >=20 > + Lohith >=20 > On 10/04/16 at 01:53P, Gleb Smirnoff wrote: >> Hiren, >>=20 >> On Mon, Sep 26, 2016 at 10:13:58AM +0000, Hiren Panchasara wrote: >> H> Author: hiren >> H> Date: Mon Sep 26 10:13:58 2016 >> H> New Revision: 306337 >> H> URL: https://svnweb.freebsd.org/changeset/base/306337 >> H>=20 >> H> Log: >> H> In sendit(), if mp->msg_control is present, then in sockargs() = we are allocating >> H> mbuf to store mp->msg_control. Later in kern_sendit(), call to = getsock_cap(), >> H> will check validity of file pointer passed, if this fails EBADF = is returned but >> H> mbuf allocated in sockargs() is not freed. Fix this possible = leak. >> H> =20 >> H> Submitted by: Lohith Bellad <lohith.bellad@me.com> >> H> Reviewed by: adrian >> H> MFC after: 3 weeks >> H> Differential Revision: https://reviews.freebsd.org/D7910 >>=20 >> The commit appeared to be incorrect, but a problem exists. I'd like = to look at it. >> Is there any reproduce recipe for the leak or bug filed? >>=20 I figured out a way to prevent this leak. I completed running stress = test on the image built with this patch and it looks fine. I will send = out for review once I reach my FreeBSD system tonight. This is regarding the following commit, which led to kernel panic!!! https://svnweb.freebsd.org/base?view=3Drevision&revision=3D306337 = <https://svnweb.freebsd.org/base?view=3Drevision&revision=3D306337>; Discussion thread regarding the kernel panic, = https://lists.freebsd.org/pipermail/svn-src-head/2016-September/092110.htm= l = <https://lists.freebsd.org/pipermail/svn-src-head/2016-September/092110.ht= ml> Thanks a lot for the input and sorry for the trouble created. Modified diff: Since its not possible to check and free the control mbuf correclty in = sendit() routine. We can clear the control mbuf in kern_sendit() routine after checking = correctly. Here is the diff, Index: sys/kern/uipc_syscalls.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 --- sys/kern/uipc_syscalls.c (revision 305955) +++ sys/kern/uipc_syscalls.c (working copy) @@ -809,6 +809,9 @@ } if (error =3D=3D 0) td->td_retval[0] =3D len - auio.uio_resid; +=20 + /* call to sosend would have cleared control */ + control =3D NULL; #ifdef KTRACE if (ktruio !=3D NULL) { ktruio->uio_resid =3D td->td_retval[0]; @@ -816,6 +819,8 @@ } #endif bad: + if (control !=3D NULL) + m_freem(control); fdrop(fp, td); return (error); } Since, we know for sure sosend() routine will consume the control mbuf = if its present else it will clear the mbuf. So, making control =3D NULL, = after the call to sosend() will prevent double freeing of control mbuf.=20= If there are any errors before call to sosend() in kern_sendit(), for = example EBADF (Bad File Descriptor) then we will fall to "bad:" and if = control !=3D NULL, we will clear the mbuf. This way mbuf leak for EBADF = is also prevented. Cheers, Lohith
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?E437A1A5-5BDB-4CA4-B98E-51BAF353A760>