[ovs-dev] datapath: Fix wrong push/pop ethernet validation

When validating push/pop ethernet actions, mac_proto is not being reset
correctly.

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@suse.com>
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
Fixes: 6fcecb85ab ("datapath: add Ethernet push and pop actions")
---
 datapath/flow_netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On 10/31/2018 8:30 AM, Jaime Caamaño Ruiz wrote:
> When validating push/pop ethernet actions, mac_proto is not being reset
> correctly.
>
> Signed-off-by: Jaime Caamaño Ruiz <jcaamano@suse.com>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047554.html
> Fixes: 6fcecb85ab ("datapath: add Ethernet push and pop actions")

Jaime,

The order of tags should be:
Reported-at:
Fixes:
Signed-off-by:

Here's a helpful guide:
http://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/

Perhaps the maintainers can fix that up on commit or they might want you 
to send another.

In any case:

Tested-by: Greg Rose <gvrose8192@gmail.com>
Reviewed-by: Greg Rose <gvrose8192@gmail.com>

Thanks Jaime!

- Greg

> ---
>   datapath/flow_netlink.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index c3f1baa05..ee0c18422 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -2998,7 +2998,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>   			 * is already present */
>   			if (mac_proto != MAC_PROTO_NONE)
>   				return -EINVAL;
> -			mac_proto = MAC_PROTO_NONE;
> +			mac_proto = MAC_PROTO_ETHERNET;
>   			break;
>   
>   		case OVS_ACTION_ATTR_POP_ETH:
> @@ -3006,7 +3006,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>   				return -EINVAL;
>   			if (vlan_tci & htons(VLAN_TAG_PRESENT))
>   				return -EINVAL;
> -			mac_proto = MAC_PROTO_ETHERNET;
> +			mac_proto = MAC_PROTO_NONE;
>   			break;
>   
>   		case OVS_ACTION_ATTR_PUSH_NSH:

On Wed, Oct 31, 2018 at 08:35:13AM -0700, Gregory Rose wrote:
> The order of tags should be:
> Reported-at:
> Fixes:
> Signed-off-by:

I agree that's the most common order but do we have anything that says
so?  (Should we?)

On 10/31/2018 10:32 AM, Ben Pfaff wrote:
> On Wed, Oct 31, 2018 at 08:35:13AM -0700, Gregory Rose wrote:
>> The order of tags should be:
>> Reported-at:
>> Fixes:
>> Signed-off-by:
> I agree that's the most common order but do we have anything that says
> so?  (Should we?)

Uh - good point.  I don't know of anything that says so.  I've been told 
by maintainers on the Linux
lists to reorder tags along those lines but I don't know of any official 
documentation that says it.

Should we?

I guess when I think about it I'd just want the Signed-off-by: line 
last.  But I suppose that's just my
preference.

- Greg

On Wed, Oct 31, 2018 at 10:40:36AM -0700, Gregory Rose wrote:
> On 10/31/2018 10:32 AM, Ben Pfaff wrote:
> >On Wed, Oct 31, 2018 at 08:35:13AM -0700, Gregory Rose wrote:
> >>The order of tags should be:
> >>Reported-at:
> >>Fixes:
> >>Signed-off-by:
> >I agree that's the most common order but do we have anything that says
> >so?  (Should we?)
> 
> Uh - good point.  I don't know of anything that says so.  I've been told by
> maintainers on the Linux
> lists to reorder tags along those lines but I don't know of any official
> documentation that says it.
> 
> Should we?
> 
> I guess when I think about it I'd just want the Signed-off-by: line last. 
> But I suppose that's just my
> preference.

Well, if the Linux folks do it that way, we should too, to avoid
confusion.

I sent a patch:
        https://patchwork.ozlabs.org/patch/991629/