From: "Yang, Yi" <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
To: Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org>
Cc: ovs dev <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
Linux Kernel Network Developers
<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
Eric Garver <e@erig.me>,
Jiri Benc <jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
"David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Subject: Re: [PATCH net-next v15] openvswitch: enable NSH support
Date: Tue, 7 Nov 2017 19:55:04 +0800 [thread overview]
Message-ID: <20171107115504.GA56784@cran64.bj.intel.com> (raw)
In-Reply-To: <CAOrHB_D2khYGGJxNA8hpE4Qnhm7ONKrH=kcNteV=iYWLRnaGHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Nov 07, 2017 at 03:58:35AM -0800, Pravin Shelar wrote:
> On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yi <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote:
> >> On Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc <jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Sat, 4 Nov 2017 07:29:46 -0700, Pravin Shelar wrote:
> >> >> > +int nsh_push(struct sk_buff *skb, const struct nshhdr *pushed_nh)
> >> >> > +{
> >> >> > + struct nshhdr *nh;
> >> >> > + size_t length = nsh_hdr_len(pushed_nh);
> >> >> > + u8 next_proto;
> >> >> > +
> >> >> > + if (skb->mac_len) {
> >> >> > + next_proto = TUN_P_ETHERNET;
> >> >> > + } else {
> >> >> > + next_proto = tun_p_from_eth_p(skb->protocol);
> >> >> > + if (!next_proto)
> >> >> > + return -EAFNOSUPPORT;
> >> >> check for supported protocols can be moved to flow install validation
> >> >> in __ovs_nla_copy_actions().
> >> >
> >> > You mean the check for !next_proto? It needs to be present for
> >> > correctness of nsh_push. This function has to be self contained, it
> >> > will be used by more callers than opevswitch, namely tc.
> >> >
> >> > It's actually not so much a check for "supported protocols", it's
> >> > rather a check of return value of a function that converts ethertype to
> >> > a 1 byte tunnel type. Blindly using a result of a function that may
> >> > return error would be wrong. Openvswitch is free to perform additional
> >> > checks but this needs to stay.
> >> >
> >> I am not disputing validity of the checks, but it could be done at
> >> flow install phase.
> >> For other use case we could refactor code. If it is too complex, I am
> >> fine with duplicate code that check the protocol in flow install for
> >> now.
> >
> > Ok, I'll add check code in __ovs_nla_copy_actions for both nsh_push and
> > nsh_pop, but how can we get value of skb->protocol in
> > __ovs_nla_copy_actions? Is it argument eth_type of
> > __ovs_nla_copy_actions?
> >
> Yes.
So here is newly-added check code, is it ok?
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index fa07a17..b64b754 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -3001,20 +3001,34 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
mac_proto = MAC_PROTO_ETHERNET;
break;
- case OVS_ACTION_ATTR_PUSH_NSH:
+ case OVS_ACTION_ATTR_PUSH_NSH: {
+ u8 next_proto;
+
+ if (mac_proto != MAC_PROTO_ETHERNET) {
+ next_proto = tun_p_from_eth_p(eth_type);
+ if (!next_proto)
+ return -EINVAL;
+ }
mac_proto = MAC_PROTO_NONE;
if (!validate_nsh(nla_data(a), false, true, true))
return -EINVAL;
break;
+ }
+
+ case OVS_ACTION_ATTR_POP_NSH: {
+ __be16 inner_proto;
- case OVS_ACTION_ATTR_POP_NSH:
if (eth_type != htons(ETH_P_NSH))
return -EINVAL;
+ inner_proto = tun_p_to_eth_p(key->nsh.base.np);
+ if (!inner_proto)
+ return -EINVAL;
if (key->nsh.base.np == TUN_P_ETHERNET)
mac_proto = MAC_PROTO_ETHERNET;
else
mac_proto = MAC_PROTO_NONE;
break;
+ }
default:
OVS_NLERR(log, "Unknown Action type %d", type);
next prev parent reply other threads:[~2017-11-07 11:55 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-01 4:03 [PATCH net-next v15] openvswitch: enable NSH support Yi Yang
2017-11-01 8:46 ` Jiri Benc
2017-11-01 15:21 ` Eric Garver
2017-11-02 0:52 ` Pravin Shelar
2017-11-02 2:50 ` Yang, Yi
2017-11-02 12:06 ` Pravin Shelar
[not found] ` <CAOrHB_BaUS7WfisvX6G+450tEQ2skmsE0v-b27s-_21s25bigw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-11-02 12:09 ` Yang, Yi
2017-11-03 1:40 ` Yang, Yi
2017-11-04 12:30 ` Pravin Shelar
[not found] ` <1509508981-66202-1-git-send-email-yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-11-04 14:29 ` Pravin Shelar
2017-11-06 4:19 ` Yang, Yi
2017-11-06 12:09 ` Pravin Shelar
[not found] ` <CAOrHB_COGWb78kN70QFKgA3w5v3zasAW=TJbzoE5zZYKao_GvA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-11-06 12:22 ` Jiri Benc
[not found] ` <20171106132248.6fa4c5a0-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2017-11-07 10:57 ` Pravin Shelar
2017-11-07 11:28 ` Yang, Yi
[not found] ` <20171107112833.GA56179-re2EX8HDrk21gSHoDXDV2kEOCMrvLtNR@public.gmane.org>
2017-11-07 11:58 ` Pravin Shelar
[not found] ` <CAOrHB_D2khYGGJxNA8hpE4Qnhm7ONKrH=kcNteV=iYWLRnaGHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-11-07 11:55 ` Yang, Yi [this message]
2017-11-07 13:01 ` Pravin Shelar
2017-11-07 13:09 ` Yang, Yi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171107115504.GA56784@cran64.bj.intel.com \
--to=yi.y.yang-ral2jqcrhueavxtiumwx3w@public.gmane.org \
--cc=davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org \
--cc=dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org \
--cc=e@erig.me \
--cc=jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=pshelar-LZ6Gd1LRuIk@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).