Re: [PATCH net-next v2 05/11] openvswitch: Add recirc action

[Nikolay Aleksandrov <nikolay@redhat.com>]

On 07/15/2014 07:42 PM, Nikolay Aleksandrov wrote:
> On 07/15/2014 07:15 PM, Pravin B Shelar wrote:
>> From: Andy Zhou <azhou@nicira.com>
>>
>> Recirculation implementation for Linux kernel data path.
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>> Acked-by: Jesse Gross <jesse@nicira.com>
>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>> ---
>>  include/uapi/linux/openvswitch.h |  2 ++
>>  net/openvswitch/actions.c        | 46 +++++++++++++++++++++++++++++++++++++-
>>  net/openvswitch/datapath.c       | 48 ++++++++++++++++++++++++++++------------
>>  net/openvswitch/datapath.h       |  8 +++++--
>>  net/openvswitch/flow.h           |  1 +
>>  net/openvswitch/flow_netlink.c   | 16 ++++++++++++++
>>  6 files changed, 104 insertions(+), 17 deletions(-)
>>  };
> <snip>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 56c22f0..8875697 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2007-2013 Nicira, Inc.
>> + * Copyright (c) 2007-2014 Nicira, Inc.
>>   *
>>   * This program is free software; you can redistribute it and/or
>>   * modify it under the terms of version 2 of the GNU General Public
>> @@ -520,6 +520,26 @@ static int execute_set_action(struct sk_buff *skb,
>>  	return err;
>>  }
>>  
>> +static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>> +			  const struct nlattr *a)
>> +{
>> +	struct sw_flow_key recirc_key;
>> +	const struct vport *p = OVS_CB(skb)->input_vport;
>> +	uint32_t hash = OVS_CB(skb)->pkt_key->ovs_flow_hash;
>> +	int err;
>> +
>> +	err = ovs_flow_extract(skb, p->port_no, &recirc_key);
>> +	if (err)
>> +		return err;
>> +
>> +	recirc_key.ovs_flow_hash = hash;
>> +	recirc_key.recirc_id = nla_get_u32(a);
>> +
>> +	ovs_dp_process_packet_with_key(skb, &recirc_key);
>> +
>> +	return 0;
>> +}
>> +
>>  /* Execute a list of actions against 'skb'. */
>>  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>  			const struct nlattr *attr, int len, bool keep_skb)
>> @@ -564,6 +584,30 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>  			err = pop_vlan(skb);
>>  			break;
>>  
>> +		case OVS_ACTION_ATTR_RECIRC: {
>> +			struct sk_buff *recirc_skb;
>> +			const bool last_action = (a->nla_len == rem);
>> +
>> +			if (__this_cpu_read(net_xmit_recursion) > NET_RECURSION_LIMIT) {
>> +				net_crit_ratelimited("Net recursion limit readched\n");
>> +				break;
>> +			}
>> +
>> +			if (!last_action || keep_skb)
>> +				recirc_skb = skb_clone(skb, GFP_ATOMIC);
> 				^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> skb_clone can return NULL and later recirc_skb will be dereferenced in
> execute_recirc(). Shouldn't there be a NULL check in this "if" case ?
> 
> 
Nevermind, I saw patch 10 and 11 later :-)

>> +			else
>> +				recirc_skb = skb;
>> +
>> +			__this_cpu_inc(net_xmit_recursion);
>> +			err = execute_recirc(dp, recirc_skb, a);
>> +			__this_cpu_dec(net_xmit_recursion);
>> +
>> +			if (last_action || err)
>> +				return err;
>> +
>> +			break;
>> +		}
>> +
>>  		case OVS_ACTION_ATTR_SET:
>>  			err = execute_set_action(skb, nla_data(a));
>>  			break;
> <snip>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>