[freebsd-current]Who should reset M_PKTHDR flag in m_buf when IP packets are fragmented. m_unshare panic throw when IPSec is enabled

[Navdeep Parhar]

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