Re: [PATCH] openvswitch: supply a dummy err_handler of gre_cisco_protocol to prevent kernel crash

Re: [PATCH] openvswitch: supply a dummy err_handler of gre_cisco_protocol to prevent kernel crash

[Jesse Gross]

On Thu, Mar 27, 2014 at 2:56 PM, Wei Zhang <asuka.com@163.com> wrote:
> diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> index a3d6951..d64c639 100644
> --- a/net/openvswitch/vport-gre.c
> +++ b/net/openvswitch/vport-gre.c
> @@ -110,6 +110,12 @@ static int gre_rcv(struct sk_buff *skb,
>         return PACKET_RCVD;
>  }
>
> +static int gre_dummy_err(struct sk_buff *skb, u32 info,
> +                        const struct tnl_ptk_info *tpi)
> +{
> +       return PACKET_REJECT;
> +}

I'm not sure that rejecting all ICMP packets is the correct thing do
here since it means that we could pass them onto a later caller even
though they are intended for us. We should probably use the same logic
as for receiving packets and just discard them here.

Re: [PATCH] openvswitch: supply a dummy err_handler of gre_cisco_protocol to prevent kernel crash

[wei zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1915 bytes --]

At 2014-03-29 06:02:25,"Jesse Gross" <jesse@nicira.com> wrote:

>I'm not sure that rejecting all ICMP packets is the correct thing do
>here since it means that we could pass them onto a later caller even
>though they are intended for us. We should probably use the same logic
>as for receiving packets and just discard them here.

Thank you very much for your advice, did you mean this logic?  

diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
index a3d6951..c183a56 100644
--- a/net/openvswitch/vport-gre.c
+++ b/net/openvswitch/vport-gre.c
@@ -110,6 +110,21 @@ static int gre_rcv(struct sk_buff *skb,
        return PACKET_RCVD;
 }
 
+/* Called with rcu_read_lock and BH disabled. */
+static int gre_err(struct sk_buff *skb, u32 info,
+                  const struct tnl_ptk_info *tpi)
+{
+       struct ovs_net *ovs_net;
+       struct vport *vport;
+
+       ovs_net = net_generic(dev_net(skb->dev), ovs_net_id);
+       vport = rcu_dereference(ovs_net->vport_net.gre_vport);
+       if (unlikely(!vport))
+               return PACKET_REJECT;
+       else
+               return PACKET_RCVD;
+}

Maybe I misunderstand something? I think if we discard all packet pass to us
when we use gre vport, new gre_cisco_protocol which has lower priority could
not see the packet intended to it.

I checked the implementation of the ipgre_err(), which has be called before
the err_handler of gre vport. It use the the (local address, remote address, key)
to distinguish the packet which is realy intended to it, although it could not 
always get the key from the icmp packet. Should we do as the same as it?
I'm not sure this is feasible, any advice is appreciate.

Regards,
Wei Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] openvswitch: supply a dummy err_handler of gre_cisco_protocol to prevent kernel crash

[Jesse Gross]

On Sun, Mar 30, 2014 at 5:12 AM, wei zhang <asuka.com@163.com> wrote:
> At 2014-03-29 06:02:25,"Jesse Gross" <jesse@nicira.com> wrote:
>
>>I'm not sure that rejecting all ICMP packets is the correct thing do
>>here since it means that we could pass them onto a later caller even
>>though they are intended for us. We should probably use the same logic
>>as for receiving packets and just discard them here.
>
> Thank you very much for your advice, did you mean this logic?

Yes, that's what I was thinking.

[...]

> Maybe I misunderstand something? I think if we discard all packet pass to us
> when we use gre vport, new gre_cisco_protocol which has lower priority could
> not see the packet intended to it.

That's true but in this case it would also not see any data packets,
so I don't think that situation would work well anyways.

> I checked the implementation of the ipgre_err(), which has be called before
> the err_handler of gre vport. It use the the (local address, remote address, key)
> to distinguish the packet which is realy intended to it, although it could not
> always get the key from the icmp packet. Should we do as the same as it?
> I'm not sure this is feasible, any advice is appreciate.

OVS does flow based matching rather than using a static set of
configuration parameters, so everything "matches" in some way
(although the result might be to drop). This generally means that OVS
is the receiver of last resort and nothing currently has a lower
priority. That actually means the difference between the patches is
somewhat academic but it seems more robust for the logic to be
consistent.

Re: [PATCH] openvswitch: supply a dummy err_handler of gre_cisco_protocol to prevent kernel crash

[wei zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1947 bytes --]

At 2014-04-01 08:49:53,"Jesse Gross" <jesse@nicira.com> wrote:
>On Sun, Mar 30, 2014 at 5:12 AM, wei zhang <asuka.com@163.com> wrote:
>> At 2014-03-29 06:02:25,"Jesse Gross" <jesse@nicira.com> wrote:

>> Maybe I misunderstand something? I think if we discard all packet pass to us
>> when we use gre vport, new gre_cisco_protocol which has lower priority could
>> not see the packet intended to it.
>
>That's true but in this case it would also not see any data packets,
>so I don't think that situation would work well anyways.
>
>> I checked the implementation of the ipgre_err(), which has be called before
>> the err_handler of gre vport. It use the the (local address, remote address, key)
>> to distinguish the packet which is realy intended to it, although it could not
>> always get the key from the icmp packet. Should we do as the same as it?
>> I'm not sure this is feasible, any advice is appreciate.
>
>OVS does flow based matching rather than using a static set of
>configuration parameters, so everything "matches" in some way
>(although the result might be to drop). 

So the flow based match could dynamically determine by the ovs daemon, we could
not find out the belonging of the packet as far as we call ovs_dp_upcall(), isn't it?

>This generally means that OVS
>is the receiver of last resort and nothing currently has a lower
>priority. 

Thanks for your kind help, this clarify my misunderstanding!

>That actually means the difference between the patches is
>somewhat academic but it seems more robust for the logic to be
>consistent.

Regards,
Wei Zhang 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] openvswitch: supply a dummy err_handler of gre_cisco_protocol to prevent kernel crash

[Jesse Gross]

On Tue, Apr 1, 2014 at 8:24 AM, wei zhang <asuka.com@163.com> wrote:
> At 2014-04-01 08:49:53,"Jesse Gross" <jesse@nicira.com> wrote:
>>On Sun, Mar 30, 2014 at 5:12 AM, wei zhang <asuka.com@163.com> wrote:
>>> At 2014-03-29 06:02:25,"Jesse Gross" <jesse@nicira.com> wrote:
>
>>> Maybe I misunderstand something? I think if we discard all packet pass to us
>>> when we use gre vport, new gre_cisco_protocol which has lower priority could
>>> not see the packet intended to it.
>>
>>That's true but in this case it would also not see any data packets,
>>so I don't think that situation would work well anyways.
>>
>>> I checked the implementation of the ipgre_err(), which has be called before
>>> the err_handler of gre vport. It use the the (local address, remote address, key)
>>> to distinguish the packet which is realy intended to it, although it could not
>>> always get the key from the icmp packet. Should we do as the same as it?
>>> I'm not sure this is feasible, any advice is appreciate.
>>
>>OVS does flow based matching rather than using a static set of
>>configuration parameters, so everything "matches" in some way
>>(although the result might be to drop).
>
> So the flow based match could dynamically determine by the ovs daemon, we could
> not find out the belonging of the packet as far as we call ovs_dp_upcall(), isn't it?

That's right - and since the OVS flow table always has a default
behavior (even if it is drop or send to controller) there's never a
packet that isn't considered to be destined to OVS once it is
received.

If this makes sense to you, would you mind submitting the patch you
had earlier formally with a commit message and signed off by line?

Re: [PATCH] openvswitch: supply a dummy err_handler of gre_cisco_protocol to prevent kernel crash

[wei zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2365 bytes --]

At 2014-04-02 02:27:56,"Jesse Gross" <jesse@nicira.com> wrote:
>On Tue, Apr 1, 2014 at 8:24 AM, wei zhang <asuka.com@163.com> wrote:
>> At 2014-04-01 08:49:53,"Jesse Gross" <jesse@nicira.com> wrote:
>>>On Sun, Mar 30, 2014 at 5:12 AM, wei zhang <asuka.com@163.com> wrote:
>>>> At 2014-03-29 06:02:25,"Jesse Gross" <jesse@nicira.com> wrote:
>>
>>>> Maybe I misunderstand something? I think if we discard all packet pass to us
>>>> when we use gre vport, new gre_cisco_protocol which has lower priority could
>>>> not see the packet intended to it.
>>>
>>>That's true but in this case it would also not see any data packets,
>>>so I don't think that situation would work well anyways.
>>>
>>>> I checked the implementation of the ipgre_err(), which has be called before
>>>> the err_handler of gre vport. It use the the (local address, remote address, key)
>>>> to distinguish the packet which is realy intended to it, although it could not
>>>> always get the key from the icmp packet. Should we do as the same as it?
>>>> I'm not sure this is feasible, any advice is appreciate.
>>>
>>>OVS does flow based matching rather than using a static set of
>>>configuration parameters, so everything "matches" in some way
>>>(although the result might be to drop).
>>
>> So the flow based match could dynamically determine by the ovs daemon, we could
>> not find out the belonging of the packet as far as we call ovs_dp_upcall(), isn't it?
>
>That's right - and since the OVS flow table always has a default
>behavior (even if it is drop or send to controller) there's never a
>packet that isn't considered to be destined to OVS once it is
>received.
>
>If this makes sense to you, would you mind submitting the patch you
>had earlier formally with a commit message and signed off by line?

Sure, thank you again as it's my first time to send patch to the kernel, your patient and kind
help give me confidence to do it :)

Regards,
Wei Zhangÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥