kern/123095 kern/131602 sendfile

[Andre Oppermann]

On 08.07.2010 13:47, Kostik Belousov wrote:
> On Thu, Jul 08, 2010 at 01:34:28PM +0200, Andre Oppermann wrote:
>> On 08.07.2010 11:42, Kostik Belousov wrote:
>>> On Thu, Jul 08, 2010 at 11:40:05AM +0300, Andriy Gapon wrote:
>>>> on 08/07/2010 11:29 Kostik Belousov said the following:
>>>>> Right, the patch maps the page in sf buffer read-only (on i386 only).
>>>>> But note the parallel posting with m_cat() change. It is still not
>>>>> enough,
>>>>> and I am not set up for the real network testing ATM.
>>>>
>>>> Could you also try to experiment with mb_dupcl?
>>>> Namely transfer M_RDONLY from source mbuf.
>>>
>>> Right, it is it.
>>>
>>> Below is my current patch including debugging facilities that seems to
>>> work.
>>> Real changes that needed are in m_cat and mb_dupcl.
>>>
>> ...
>>> diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
>>> index f41eb03..1701ef2 100644
>>> --- a/sys/kern/uipc_mbuf.c
>>> +++ b/sys/kern/uipc_mbuf.c
>>> @@ -301,7 +301,7 @@ mb_dupcl(struct mbuf *n, struct mbuf *m)
>>>   	n->m_ext.ext_size = m->m_ext.ext_size;
>>>   	n->m_ext.ref_cnt = m->m_ext.ref_cnt;
>>>   	n->m_ext.ext_type = m->m_ext.ext_type;
>>> -	n->m_flags |= M_EXT;
>>> +	n->m_flags |= M_EXT | (M_RDONLY&  m->m_flags);
>>>   }
>>
>> Having the M_EXT flag always implies readonly and M_WRITABLE gets this
>> right.  Not inheriting all the flags from the source seems questionable.
>> So IMHO this should be done here:
>>
>>   n->m_flags |= (M_EXT | m->m_flags)
>>
>>>   /*
>>> @@ -911,7 +911,8 @@ m_cat(struct mbuf *m, struct mbuf *n)
>>>   		m = m->m_next;
>>>   	while (n) {
>>>   		if (m->m_flags&   M_EXT ||
>>> -		    m->m_data + m->m_len + n->m_len>=&m->m_dat[MLEN]) {
>>> +		    m->m_data + m->m_len + n->m_len>=&m->m_dat[MLEN] ||
>>> +		    !M_WRITABLE(m)) {
>>
>> Here you can fully replace the (m->m_flags&  M_EXT) test with
>> M_WRITABLE().  The M_EXT test is included in it.
>>
>>>   			/* just join the two chains */
>>>   			m->m_next = n;
>>>   			return;
>
> The patch is below. Works as well for me, thank you for the feedback.

You may want to run the change to m_dupcl() by rwatson just to be sure.
It should be the right thing to do but better have another mbuf expert
look at it.

Does this fix the sendfile corruption problem for all cases or just the
panic for the loopback case?

> diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
> index f41eb03..58567a4 100644
> --- a/sys/kern/uipc_mbuf.c
> +++ b/sys/kern/uipc_mbuf.c
> @@ -301,7 +301,7 @@ mb_dupcl(struct mbuf *n, struct mbuf *m)
>   	n->m_ext.ext_size = m->m_ext.ext_size;
>   	n->m_ext.ref_cnt = m->m_ext.ref_cnt;
>   	n->m_ext.ext_type = m->m_ext.ext_type;
> -	n->m_flags |= M_EXT;
> +	n->m_flags |= M_EXT | m->m_flags;
>   }
>
>   /*
> @@ -910,7 +910,7 @@ m_cat(struct mbuf *m, struct mbuf *n)
>   	while (m->m_next)
>   		m = m->m_next;
>   	while (n) {
> -		if (m->m_flags&  M_EXT ||
> +		if (!M_WRITABLE(m) ||
>   		    m->m_data + m->m_len + n->m_len>=&m->m_dat[MLEN]) {
>   			/* just join the two chains */
>   			m->m_next = n;

-- 
Andre