Re: [PATCH net] openvswitch: fix skb_panic due to the incorrect actions attrlen

2017-08-15 Thread Liping Zhang 
2017-08-15 13:01 GMT+08:00 Pravin Shelar :
[...]
>>  net/openvswitch/actions.c  | 39 +--
>>  net/openvswitch/datapath.c |  2 +-
>>  net/openvswitch/datapath.h |  1 +
>>  3 files changed, 27 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index e4610676299b..799a22dfb89e 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -48,6 +48,7 @@ struct deferred_action {
>> struct sk_buff *skb;
>> const struct nlattr *actions;
>> int actions_len;
>> +   int actions_attrlen;
>>
> Have you considered passing this value using struct ovs_skb_cb? That
> would save passing this parameter in all these functions.

Thanks for your reviewing.

Right, this will make codes more clean, I will send V2 later.

Re: [PATCH net] openvswitch: fix skb_panic due to the incorrect actions attrlen

2017-08-14 Thread Pravin Shelar 
On Sun, Aug 13, 2017 at 12:04 AM, Liping Zhang  wrote:
> From: Liping Zhang 
>
> For sw_flow_actions, the actions_len only represents the kernel part's
> size, and when we dump the actions to the userspace, we will do the
> convertions, so it's true size may become bigger than the actions_len.
>
> But unfortunately, for OVS_PACKET_ATTR_ACTIONS, we use the actions_len
> to alloc the skbuff, so the user_skb's size may become insufficient and
> oops will happen like this:
>   skbuff: skb_over_panic: text:8148fabf len:1749 put:157 head:
>   881300f39000 data:881300f39000 tail:0x6d5 end:0x6c0 dev:
>   [ cut here ]
>   kernel BUG at net/core/skbuff.c:129!
>   [...]
>   Call Trace:
>
>[] skb_put+0x43/0x44
>[] skb_zerocopy+0x6c/0x1f4
>[] queue_userspace_packet+0x3a3/0x448 [openvswitch]
>[] ovs_dp_upcall+0x30/0x5c [openvswitch]
>[] output_userspace+0x132/0x158 [openvswitch]
>[] ? ip6_rcv_finish+0x74/0x77 [ipv6]
>[] do_execute_actions+0xcc1/0xdc8 [openvswitch]
>[] ovs_execute_actions+0x74/0x106 [openvswitch]
>[] ovs_dp_process_packet+0xe1/0xfd [openvswitch]
>[] ? key_extract+0x63c/0x8d5 [openvswitch]
>[] ovs_vport_receive+0xa1/0xc3 [openvswitch]
>   [...]
>
> Also we can find that the actions_len is much little than the orig_len:
>   crash> struct sw_flow_actions 0x8812f539d000
>   struct sw_flow_actions {
> rcu = {
>   next = 0x8812f5398800,
>   func = 0xe3b00035db32
> },
> orig_len = 1384,
> actions_len = 592,
> actions = 0x8812f539d01c
>   }
>
> So as a quick fix, use the orig_len instead of the actions_len to alloc
> the user_skb.
>
> Last, this oops happened on our system running a relative old kernel, but
> the same risk still exists on the mainline, since we use the wrong
> actions_len from the beginning.
>
Thanks for fixing it.

> Fixes: ccea74457bbd ("openvswitch: include datapath actions with 
> sampled-packet upcall to userspace")
> Cc: Neil McKee 
> Signed-off-by: Liping Zhang 
> ---
>  net/openvswitch/actions.c  | 39 +--
>  net/openvswitch/datapath.c |  2 +-
>  net/openvswitch/datapath.h |  1 +
>  3 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index e4610676299b..799a22dfb89e 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -48,6 +48,7 @@ struct deferred_action {
> struct sk_buff *skb;
> const struct nlattr *actions;
> int actions_len;
> +   int actions_attrlen;
>
Have you considered passing this value using struct ovs_skb_cb? That
would save passing this parameter in all these functions.

[PATCH net] openvswitch: fix skb_panic due to the incorrect actions attrlen

2017-08-13 Thread Liping Zhang 
From: Liping Zhang 

For sw_flow_actions, the actions_len only represents the kernel part's
size, and when we dump the actions to the userspace, we will do the
convertions, so it's true size may become bigger than the actions_len.

But unfortunately, for OVS_PACKET_ATTR_ACTIONS, we use the actions_len
to alloc the skbuff, so the user_skb's size may become insufficient and
oops will happen like this:
  skbuff: skb_over_panic: text:8148fabf len:1749 put:157 head:
  881300f39000 data:881300f39000 tail:0x6d5 end:0x6c0 dev:
  [ cut here ]
  kernel BUG at net/core/skbuff.c:129!
  [...]
  Call Trace:
   
   [] skb_put+0x43/0x44
   [] skb_zerocopy+0x6c/0x1f4
   [] queue_userspace_packet+0x3a3/0x448 [openvswitch]
   [] ovs_dp_upcall+0x30/0x5c [openvswitch]
   [] output_userspace+0x132/0x158 [openvswitch]
   [] ? ip6_rcv_finish+0x74/0x77 [ipv6]
   [] do_execute_actions+0xcc1/0xdc8 [openvswitch]
   [] ovs_execute_actions+0x74/0x106 [openvswitch]
   [] ovs_dp_process_packet+0xe1/0xfd [openvswitch]
   [] ? key_extract+0x63c/0x8d5 [openvswitch]
   [] ovs_vport_receive+0xa1/0xc3 [openvswitch]
  [...]

Also we can find that the actions_len is much little than the orig_len:
  crash> struct sw_flow_actions 0x8812f539d000
  struct sw_flow_actions {
rcu = {
  next = 0x8812f5398800,
  func = 0xe3b00035db32
},
orig_len = 1384,
actions_len = 592,
actions = 0x8812f539d01c
  }

So as a quick fix, use the orig_len instead of the actions_len to alloc
the user_skb.

Last, this oops happened on our system running a relative old kernel, but
the same risk still exists on the mainline, since we use the wrong
actions_len from the beginning.

Fixes: ccea74457bbd ("openvswitch: include datapath actions with sampled-packet 
upcall to userspace")
Cc: Neil McKee 
Signed-off-by: Liping Zhang 
---
 net/openvswitch/actions.c  | 39 +--
 net/openvswitch/datapath.c |  2 +-
 net/openvswitch/datapath.h |  1 +
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index e4610676299b..799a22dfb89e 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -48,6 +48,7 @@ struct deferred_action {
struct sk_buff *skb;
const struct nlattr *actions;
int actions_len;
+   int actions_attrlen;
 
/* Store pkt_key clone when creating deferred action. */
struct sw_flow_key pkt_key;
@@ -135,7 +136,8 @@ static struct deferred_action *action_fifo_put(struct 
action_fifo *fifo)
 static struct deferred_action *add_deferred_actions(struct sk_buff *skb,
const struct sw_flow_key *key,
const struct nlattr *actions,
-   const int actions_len)
+   const int actions_len,
+   const int actions_attrlen)
 {
struct action_fifo *fifo;
struct deferred_action *da;
@@ -146,6 +148,7 @@ static struct deferred_action *add_deferred_actions(struct 
sk_buff *skb,
da->skb = skb;
da->actions = actions;
da->actions_len = actions_len;
+   da->actions_attrlen = actions_attrlen;
da->pkt_key = *key;
}
 
@@ -166,6 +169,7 @@ static int clone_execute(struct datapath *dp, struct 
sk_buff *skb,
 struct sw_flow_key *key,
 u32 recirc_id,
 const struct nlattr *actions, int len,
+int actions_attrlen,
 bool last, bool clone_flow_key);
 
 static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
@@ -880,7 +884,7 @@ static void do_output(struct datapath *dp, struct sk_buff 
*skb, int out_port,
 static int output_userspace(struct datapath *dp, struct sk_buff *skb,
struct sw_flow_key *key, const struct nlattr *attr,
const struct nlattr *actions, int actions_len,
-   uint32_t cutlen)
+   int actions_attrlen, uint32_t cutlen)
 {
struct dp_upcall_info upcall;
const struct nlattr *a;
@@ -921,6 +925,7 @@ static int output_userspace(struct datapath *dp, struct 
sk_buff *skb,
/* Include actions. */
upcall.actions = actions;
upcall.actions_len = actions_len;
+   upcall.actions_attrlen = actions_attrlen;
break;
}
 
@@ -936,7 +941,7 @@ static int output_userspace(struct datapath *dp, struct 
sk_buff *skb,
  */
 static int sample(struct datapath *dp, struct sk_buff *skb,
  struct sw_flow_key *key, const struct nlattr *attr,
- bool last)
+