Date: Wed, 27 Dec 2017 14:10:39 -0800 From: Navdeep Parhar <np@FreeBSD.org> To: "Andrey V. Elsukov" <bu7cher@yandex.ru>, Harsh Jain <harsh@chelsio.com>, freebsd-net@freebsd.org Subject: Re: [freebsd-current]Who should reset M_PKTHDR flag in m_buf when IP packets are fragmented. m_unshare panic throw when IPSec is enabled Message-ID: <2887a9d7-6b72-25a4-84da-74578b54d103@FreeBSD.org> In-Reply-To: <edc841d1-d895-f834-1462-1fb454dd8304@yandex.ru> References: <73302ead-b2e9-c25b-4d11-475f38dec1a1@chelsio.com> <993c58bb-3bf2-d6a3-9a05-13e1631aec87@yandex.ru> <fdb72f54-efdd-c54b-c8f7-c53057d24adf@chelsio.com> <c7513431-202e-55e4-e8be-2e3dffb897e9@yandex.ru> <c70e3596-89c2-67e8-e635-06789c2697be@FreeBSD.org> <edc841d1-d895-f834-1462-1fb454dd8304@yandex.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
On 12/27/2017 12:59, Andrey V. Elsukov wrote: > On 27.12.2017 23:09, Navdeep Parhar wrote: >>> It is not clear to me why it helps. The panic happens on outbound path, >>> where mbuf should be allocated by network stack and should be writeable. >>> ip_reass() usually used on inbound path. I think the patch just hides >>> the problem in another place. >>> Do you mean that cxgbe can produce !WRITEABLE mbuf for received packet >>> and then pass it to the network stack? >> >> Yes, cxgbe does that. But I think the real bug here is in ip_reass >> because it doesn't properly get rid of the pkthdr of the fragments while >> creating the reassembled datagram. cxgbe happens to trip on this easily >> because it often creates !WRITEABLE mbufs. > > From the quick look, I don't see the code in netipsec and in crypto, > that does check mbuf is WRITEABLE. It is expected that in most cases for > received mbuf the data will be decrypted and copied back into the given > buffer. Can this lead to memory corruption? > >> This should fix it: >> https://people.freebsd.org/~np/ip_reass_demotehdr.diff >> >> It will also fix leaks in configurations where mbuf tags are in use by >> default (for example with MAC), ip_reass is involved during rx, and the >> mbuf chain never gets m_demote'd elsewhere (meaning ip_reass should have >> freed the tags itself). > > I think such chain with several mbufs with M_PKTHDR flag is created with > m_cat() due to !WRITEABLE mbufs. And when mbuf chain will be freed, the > tags chain will be also destroyed by mbuf zone destructor. I see m_freem/m_free will do the right thing but such a chain isn't legal. m_unshare is complaining about it here. m_sanity on the chain will fail too. m_cat says it will leave the pkthdr alone so it is working as advertised. It's the caller's job to clean up headers etc. to keep the mbuf chain valid. > > If you think it solves the problem, the IPv6 fragment reassembly > probably needs the same code. But I think that M_WRITEABLE flag is not > properly handled is the problem too. > I think M_WRITEABLE is being handled properly here. m_unshare deals with the chain just fine apart from this assert about multiple M_PKTHDR. I'll fix IP6 reassembly too and post to phabricator if the change looks ok? Regards, Navdeep
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2887a9d7-6b72-25a4-84da-74578b54d103>