[ovs-dev] datapath: stt: linearize in SKIP_ZERO_COPY case

During the investigation of a kernel panic, we encountered a condition
that triggered a kernel panic due to a large skb with an unusual
geometry.  Inside of the STT codepath, an effort is made to linearize
such packets to avoid trouble during both fragment reassembly and
segmentation in the linux networking core.

As currently implemented, kernels with CONFIG_SLUB defined will skip
this process because it does not expect an skb with a frag_list to be
present.  This patch removes the assumption, and allows these skb to
be linearized as intended.  We confirmed this corrects the panic we
encountered.

Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html
Requested-by: Pravin Shelar <pshelar@ovn.org>
Signed-off-by: Neal Shrader <neal@digitalocean.com>
---
 datapath/linux/compat/stt.c | 7 -------
 1 file changed, 7 deletions(-)

On 6/22/2018 3:18 PM, Neal Shrader via dev wrote:
> During the investigation of a kernel panic, we encountered a condition
> that triggered a kernel panic due to a large skb with an unusual
> geometry.  Inside of the STT codepath, an effort is made to linearize
> such packets to avoid trouble during both fragment reassembly and
> segmentation in the linux networking core.
>
> As currently implemented, kernels with CONFIG_SLUB defined will skip
> this process because it does not expect an skb with a frag_list to be
> present.  This patch removes the assumption, and allows these skb to
> be linearized as intended.  We confirmed this corrects the panic we
> encountered.
>
> Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html
> Requested-by: Pravin Shelar <pshelar@ovn.org>
> Signed-off-by: Neal Shrader <neal@digitalocean.com>
> ---
>   datapath/linux/compat/stt.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c
> index ca9f03988..64fc550fe 100644
> --- a/datapath/linux/compat/stt.c
> +++ b/datapath/linux/compat/stt.c
> @@ -559,12 +559,6 @@ static int __try_to_segment(struct sk_buff *skb, bool csum_partial,
>   
>   static int try_to_segment(struct sk_buff *skb)
>   {
> -#ifdef SKIP_ZERO_COPY
> -	/* coalesce_skb() since does not generate frag-list no need to
> -	 * linearize it here.
> -	 */
> -	return 0;
> -#else
>   	struct stthdr *stth = stt_hdr(skb);
>   	bool csum_partial = !!(stth->flags & STT_CSUM_PARTIAL);
>   	bool ipv4 = !!(stth->flags & STT_PROTO_IPV4);
> @@ -572,7 +566,6 @@ static int try_to_segment(struct sk_buff *skb)
>   	int l4_offset = stth->l4_offset;
>   
>   	return __try_to_segment(skb, csum_partial, ipv4, tcp, l4_offset);
> -#endif
>   }
>   
>   static int segment_skb(struct sk_buff **headp, bool csum_partial,

This seems reasonable to me but it still does seem strange that an skb 
with a frag list occurs when it's not supposed to?

Adding Pravin.

- Greg

On Fri, Jun 22, 2018 at 5:20 PM, Gregory Rose <gvrose8192@gmail.com> wrote:
> On 6/22/2018 3:18 PM, Neal Shrader via dev wrote:
>>
>> During the investigation of a kernel panic, we encountered a condition
>> that triggered a kernel panic due to a large skb with an unusual
>> geometry.  Inside of the STT codepath, an effort is made to linearize
>> such packets to avoid trouble during both fragment reassembly and
>> segmentation in the linux networking core.
>>
>> As currently implemented, kernels with CONFIG_SLUB defined will skip
>> this process because it does not expect an skb with a frag_list to be
>> present.  This patch removes the assumption, and allows these skb to
>> be linearized as intended.  We confirmed this corrects the panic we
>> encountered.
>>
>> Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
>> Reported-at:
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html
>> Requested-by: Pravin Shelar <pshelar@ovn.org>
>> Signed-off-by: Neal Shrader <neal@digitalocean.com>
>> ---
>>   datapath/linux/compat/stt.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c
>> index ca9f03988..64fc550fe 100644
>> --- a/datapath/linux/compat/stt.c
>> +++ b/datapath/linux/compat/stt.c
>> @@ -559,12 +559,6 @@ static int __try_to_segment(struct sk_buff *skb, bool
>> csum_partial,
>>     static int try_to_segment(struct sk_buff *skb)
>>   {
>> -#ifdef SKIP_ZERO_COPY
>> -       /* coalesce_skb() since does not generate frag-list no need to
>> -        * linearize it here.
>> -        */
>> -       return 0;
>> -#else
>>         struct stthdr *stth = stt_hdr(skb);
>>         bool csum_partial = !!(stth->flags & STT_CSUM_PARTIAL);
>>         bool ipv4 = !!(stth->flags & STT_PROTO_IPV4);
>> @@ -572,7 +566,6 @@ static int try_to_segment(struct sk_buff *skb)
>>         int l4_offset = stth->l4_offset;
>>         return __try_to_segment(skb, csum_partial, ipv4, tcp, l4_offset);
>> -#endif
>>   }
>>     static int segment_skb(struct sk_buff **headp, bool csum_partial,
>
>
> This seems reasonable to me but it still does seem strange that an skb with
> a frag list occurs when it's not supposed to?
>

I think some driver started generating such skb, so we need to handle
it in STT.

I applied patch to master and to branch-2.[6-9].

Thanks.