[ovs-dev] [PATCH v3 01/10] datapath: Avoid assigning a NULL pointer to flow actions.

[Jarno Rajahalme]

On Mar 21, 2014, at 1:52 PM, Pravin Shelar <pshelar at nicira.com> wrote:

> On Fri, Feb 21, 2014 at 11:41 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> Flow SET can set an empty set of actions by not passing any actions.
>> Previously, we assigned the flow's actions pointer to NULL in this
>> case, but we never check for the NULL pointer later on.  This patch
>> modifies this case to allocate a valid but empty set of actions
>> instead.
>> 
> Good catch.
> 
>> Note: If might be prudent to allow missing actions also in
>> OVS_FLOW_CMD_NEW.
>> 
> dpif-linux can avoid sending dummy action if we allow that. But for
> compatibility we need to keep it anyways. So lets keep it as it is.
> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
>> ---
>> datapath/datapath.c |    9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index f7c3391..130300f 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -810,7 +810,14 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
>>                        OVS_NLERR("Flow actions may not be safe on all matching packets.\n");
>>                        goto err_kfree;
>>                }
>> -       } else if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW) {
>> +       } else if (info->genlhdr->cmd == OVS_FLOW_CMD_SET) {
>> +               /* Need empty actions. */
>> +               acts = ovs_nla_alloc_flow_actions(0);
>> +               error = PTR_ERR(acts);
>> +               if (IS_ERR(acts))
>> +                       goto error;
>> +       } else {
>> +               /* OVS_FLOW_CMD_NEW must have actions. */
>>                error = -EINVAL;
>>                goto error;
>>        }
> 
> Before this bug (OVS 1.7) set action use to ignore action if they were
> NULL. In that case set-action was pure stats-clear call. Can we use
> same semantics now? I mean ignore NULL actions for set-actions.

Do we have this behavior documented somewhere? In include/linux/openvswitch.h we currently have:

 * @OVS_FLOW_ATTR_ACTIONS: Nested %OVS_ACTION_ATTR_* attributes specifying
 * the actions to take for packets that match the key.  Always present in
 * notifications.  Required for %OVS_FLOW_CMD_NEW requests, optional for
 * %OVS_FLOW_CMD_SET requests.

How about adding:

“An %OVS_FLOW_CMD_SET without %OVS_FLOW_ATTR_ACTIONS will not modify the actions. To clear the actions, an %OVS_FLOW_ATTR_ACTIONS without any nested attributes must be given.”

  Jarno