Re: [ovs-dev] [PATCH v6 1/4] nsh: rework NSH netlink keys and actions

2018-01-06 Thread Ben Pfaff 
On Sat, Jan 06, 2018 at 01:56:20PM +0800, Yang, Yi wrote:
> On Fri, Jan 05, 2018 at 04:42:00AM +0800, Ben Pfaff wrote:
> > On Fri, Dec 08, 2017 at 10:04:22PM +0800, Yi Yang wrote:
> > > Signed-off-by: Yi Yang 
> > 
> > This fails to build with Clang (and, I would guess, MSVC):
> > ../lib/odp-execute.c:497:21: error: fields must have a constant size: 
> > 'variable length array in structure' extension will never be supported
> > 
> > "sparse" issues some warnings.  This one is probably sensible:
> > ../lib/odp-util.c:1884:32: warning: incorrect type in argument 1 
> > (different base types)
> > ../lib/odp-util.c:1884:32:expected unsigned int [unsigned] 
> > [usertype] x
> > ../lib/odp-util.c:1884:32:got restricted ovs_be32 [assigned] 
> > [usertype] spi
> > 
> > These I don't understand.
> > https://marc.info/?l=linux-sparse=110218288411207 suggests that they
> > might be a sparse bug:
> > 
> > ../lib/odp-util.c:7123:32: warning: crazy programmer
> > ../lib/odp-util.c:7123:43: warning: crazy programmer
> > ../lib/odp-util.c:7123:22: warning: crazy programmer
> > 
> > In odp_execute_actions(), this looks bogus: there is nothing to
> > guarantee that 'buffer' is properly aligned for struct nsh_hdr.
> > +uint8_t buffer[NSH_HDR_MAX_LEN];
> > +struct nsh_hdr *nsh_hdr = ALIGNED_CAST(struct nsh_hdr *, 
> > buffer);
> > 
> > Similarly in format_odp_action().
> > 
> > In nsh_key_to_attr(), can this:
> > +for (int i = 0; i < 4; i++) {
> > +md1.context[i] = nsh->context[i];
> > +}
> > +nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, , sizeof md1);
> > be written as just this?
> > +nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, nsh->context,
> > +  sizeof nsh->context);
> > and similarly in a second place.
> > 
> > In parse_odp_push_nsh_action(), the idea of xmalloc()'ing a stub is
> > weird.  A stub should be a local array.  There are many examples in the
> > tree.
> > 
> > Please don't check a pointer for null before calling free():
> > +if (metadata != NULL) {
> > +free(metadata);
> >  }
> 
> Thanks, Ben, I have posted v7
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/342698.html
> to fix the above issues.
> 
> By the way, what command do you run to do static code analysis by sparse?
> "make clang-analyze" will have too much warnings. I can't get sparse
> warnings, it will be better if I can run it locally.

Just follow the instructions in the documentation.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6 1/4] nsh: rework NSH netlink keys and actions

2018-01-06 Thread Ben Pfaff 
On Sat, Jan 06, 2018 at 02:03:09PM +0800, Yang, Yi wrote:
> On Fri, Jan 05, 2018 at 04:42:00AM +0800, Ben Pfaff wrote:
> > On Fri, Dec 08, 2017 at 10:04:22PM +0800, Yi Yang wrote:
> > > 
> > > Signed-off-by: Yi Yang 
> > 
> > 
> > In odp_execute_actions(), this looks bogus: there is nothing to
> > guarantee that 'buffer' is properly aligned for struct nsh_hdr.
> > +uint8_t buffer[NSH_HDR_MAX_LEN];
> > +struct nsh_hdr *nsh_hdr = ALIGNED_CAST(struct nsh_hdr *, 
> > buffer);
> > 
> > Similarly in format_odp_action().
> > 
> 
> Ben, do you mean buffer isn't 4 bytes aligned? I redefine it as
> "uint32_t buffer[NSH_HDR_MAX_LEN / 4];", it is enough for struct nsh_hdr to
> align to 4 bytes boundary.

Yes, that's fine, thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6 1/4] nsh: rework NSH netlink keys and actions

2018-01-05 Thread Yang, Yi 
On Fri, Jan 05, 2018 at 04:42:00AM +0800, Ben Pfaff wrote:
> On Fri, Dec 08, 2017 at 10:04:22PM +0800, Yi Yang wrote:
> > 
> > Signed-off-by: Yi Yang 
> 
> 
> In odp_execute_actions(), this looks bogus: there is nothing to
> guarantee that 'buffer' is properly aligned for struct nsh_hdr.
> +uint8_t buffer[NSH_HDR_MAX_LEN];
> +struct nsh_hdr *nsh_hdr = ALIGNED_CAST(struct nsh_hdr *, buffer);
> 
> Similarly in format_odp_action().
> 

Ben, do you mean buffer isn't 4 bytes aligned? I redefine it as
"uint32_t buffer[NSH_HDR_MAX_LEN / 4];", it is enough for struct nsh_hdr to
align to 4 bytes boundary.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6 1/4] nsh: rework NSH netlink keys and actions

2018-01-05 Thread Yang, Yi 
On Fri, Jan 05, 2018 at 04:42:00AM +0800, Ben Pfaff wrote:
> On Fri, Dec 08, 2017 at 10:04:22PM +0800, Yi Yang wrote:
> > Signed-off-by: Yi Yang 
> 
> This fails to build with Clang (and, I would guess, MSVC):
> ../lib/odp-execute.c:497:21: error: fields must have a constant size: 
> 'variable length array in structure' extension will never be supported
> 
> "sparse" issues some warnings.  This one is probably sensible:
> ../lib/odp-util.c:1884:32: warning: incorrect type in argument 1 
> (different base types)
> ../lib/odp-util.c:1884:32:expected unsigned int [unsigned] [usertype] 
> x
> ../lib/odp-util.c:1884:32:got restricted ovs_be32 [assigned] 
> [usertype] spi
> 
> These I don't understand.
> https://marc.info/?l=linux-sparse=110218288411207 suggests that they
> might be a sparse bug:
> 
> ../lib/odp-util.c:7123:32: warning: crazy programmer
> ../lib/odp-util.c:7123:43: warning: crazy programmer
> ../lib/odp-util.c:7123:22: warning: crazy programmer
> 
> In odp_execute_actions(), this looks bogus: there is nothing to
> guarantee that 'buffer' is properly aligned for struct nsh_hdr.
> +uint8_t buffer[NSH_HDR_MAX_LEN];
> +struct nsh_hdr *nsh_hdr = ALIGNED_CAST(struct nsh_hdr *, buffer);
> 
> Similarly in format_odp_action().
> 
> In nsh_key_to_attr(), can this:
> +for (int i = 0; i < 4; i++) {
> +md1.context[i] = nsh->context[i];
> +}
> +nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, , sizeof md1);
> be written as just this?
> +nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, nsh->context,
> +  sizeof nsh->context);
> and similarly in a second place.
> 
> In parse_odp_push_nsh_action(), the idea of xmalloc()'ing a stub is
> weird.  A stub should be a local array.  There are many examples in the
> tree.
> 
> Please don't check a pointer for null before calling free():
> +if (metadata != NULL) {
> +free(metadata);
>  }

Thanks, Ben, I have posted v7
https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/342698.html
to fix the above issues.

By the way, what command do you run to do static code analysis by sparse?
"make clang-analyze" will have too much warnings. I can't get sparse
warnings, it will be better if I can run it locally.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6 1/4] nsh: rework NSH netlink keys and actions

2018-01-04 Thread Ben Pfaff 
On Fri, Dec 08, 2017 at 10:04:22PM +0800, Yi Yang wrote:
> This patch changes OVS_KEY_ATTR_NSH
> to nested attribute and adds three new NSH sub attribute keys:
> 
> OVS_NSH_KEY_ATTR_BASE: for length-fixed NSH base header
> OVS_NSH_KEY_ATTR_MD1:  for length-fixed MD type 1 context
> OVS_NSH_KEY_ATTR_MD2:  for length-variable MD type 2 metadata
> 
> Its intention is to align to NSH kernel implementation.
> 
> NSH match fields, set and PUSH_NSH action all use the below
> nested attribute format:
> 
> OVS_KEY_ATTR_NSH begin
> OVS_NSH_KEY_ATTR_BASE
> OVS_NSH_KEY_ATTR_MD1
> OVS_KEY_ATTR_NSH end
> 
> or
> 
> OVS_KEY_ATTR_NSH begin
> OVS_NSH_KEY_ATTR_BASE
> OVS_NSH_KEY_ATTR_MD2
> OVS_KEY_ATTR_NSH end
> 
> In addition, NSH encap and decap actions are renamed as push_nsh
> and pop_nsh to meet action naming convention.
> 
> Signed-off-by: Yi Yang 

This fails to build with Clang (and, I would guess, MSVC):
../lib/odp-execute.c:497:21: error: fields must have a constant size: 
'variable length array in structure' extension will never be supported

"sparse" issues some warnings.  This one is probably sensible:
../lib/odp-util.c:1884:32: warning: incorrect type in argument 1 (different 
base types)
../lib/odp-util.c:1884:32:expected unsigned int [unsigned] [usertype] x
../lib/odp-util.c:1884:32:got restricted ovs_be32 [assigned] [usertype] 
spi

These I don't understand.
https://marc.info/?l=linux-sparse=110218288411207 suggests that they
might be a sparse bug:

../lib/odp-util.c:7123:32: warning: crazy programmer
../lib/odp-util.c:7123:43: warning: crazy programmer
../lib/odp-util.c:7123:22: warning: crazy programmer

In odp_execute_actions(), this looks bogus: there is nothing to
guarantee that 'buffer' is properly aligned for struct nsh_hdr.
+uint8_t buffer[NSH_HDR_MAX_LEN];
+struct nsh_hdr *nsh_hdr = ALIGNED_CAST(struct nsh_hdr *, buffer);

Similarly in format_odp_action().

In nsh_key_to_attr(), can this:
+for (int i = 0; i < 4; i++) {
+md1.context[i] = nsh->context[i];
+}
+nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, , sizeof md1);
be written as just this?
+nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, nsh->context,
+  sizeof nsh->context);
and similarly in a second place.

In parse_odp_push_nsh_action(), the idea of xmalloc()'ing a stub is
weird.  A stub should be a local array.  There are many examples in the
tree.

Please don't check a pointer for null before calling free():
+if (metadata != NULL) {
+free(metadata);
 }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v6 1/4] nsh: rework NSH netlink keys and actions

2017-12-08 Thread Yi Yang 
This patch changes OVS_KEY_ATTR_NSH
to nested attribute and adds three new NSH sub attribute keys:

OVS_NSH_KEY_ATTR_BASE: for length-fixed NSH base header
OVS_NSH_KEY_ATTR_MD1:  for length-fixed MD type 1 context
OVS_NSH_KEY_ATTR_MD2:  for length-variable MD type 2 metadata

Its intention is to align to NSH kernel implementation.

NSH match fields, set and PUSH_NSH action all use the below
nested attribute format:

OVS_KEY_ATTR_NSH begin
OVS_NSH_KEY_ATTR_BASE
OVS_NSH_KEY_ATTR_MD1
OVS_KEY_ATTR_NSH end

or

OVS_KEY_ATTR_NSH begin
OVS_NSH_KEY_ATTR_BASE
OVS_NSH_KEY_ATTR_MD2
OVS_KEY_ATTR_NSH end

In addition, NSH encap and decap actions are renamed as push_nsh
and pop_nsh to meet action naming convention.

Signed-off-by: Yi Yang 
---
 datapath/linux/compat/include/linux/openvswitch.h |  58 +-
 include/openvswitch/nsh.h |  26 +-
 include/openvswitch/packets.h |  11 +-
 lib/dpif-netdev.c |   4 +-
 lib/dpif.c|   4 +-
 lib/flow.c|  40 +-
 lib/match.c   |  12 +-
 lib/meta-flow.c   |  13 +-
 lib/nx-match.c|   4 +-
 lib/odp-execute.c |  76 ++-
 lib/odp-util.c| 745 ++
 lib/odp-util.h|   4 +
 lib/packets.c |  26 +-
 lib/packets.h |   5 +-
 ofproto/ofproto-dpif-ipfix.c  |   4 +-
 ofproto/ofproto-dpif-sflow.c  |   4 +-
 ofproto/ofproto-dpif-xlate.c  |  24 +-
 tests/nsh.at  |  30 +-
 18 files changed, 803 insertions(+), 287 deletions(-)

diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
b/datapath/linux/compat/include/linux/openvswitch.h
index 561f895..3ddb1c5 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -369,7 +369,7 @@ enum ovs_key_attr {
 #ifndef __KERNEL__
/* Only used within userspace data path. */
OVS_KEY_ATTR_PACKET_TYPE,  /* be32 packet type */
-   OVS_KEY_ATTR_NSH,  /* struct ovs_key_nsh */
+   OVS_KEY_ATTR_NSH,  /* Nested set of ovs_nsh_key_* */
 #endif
 
__OVS_KEY_ATTR_MAX
@@ -492,13 +492,28 @@ struct ovs_key_ct_labels {
};
 };
 
-struct ovs_key_nsh {
-__u8 flags;
-__u8 mdtype;
-__u8 np;
-__u8 pad;
-__be32 path_hdr;
-__be32 c[4];
+enum ovs_nsh_key_attr {
+   OVS_NSH_KEY_ATTR_UNSPEC,
+   OVS_NSH_KEY_ATTR_BASE,  /* struct ovs_nsh_key_base. */
+   OVS_NSH_KEY_ATTR_MD1,   /* struct ovs_nsh_key_md1. */
+   OVS_NSH_KEY_ATTR_MD2,   /* variable-length octets. */
+   __OVS_NSH_KEY_ATTR_MAX
+};
+
+#define OVS_NSH_KEY_ATTR_MAX (__OVS_NSH_KEY_ATTR_MAX - 1)
+
+struct ovs_nsh_key_base {
+   __u8 flags;
+   __u8 mdtype;
+   __u8 np;
+   __u8 pad;
+   __be32 path_hdr;
+};
+
+#define NSH_MD1_CONTEXT_SIZE 4
+
+struct ovs_nsh_key_md1 {
+   __be32 context[NSH_MD1_CONTEXT_SIZE];
 };
 
 /* OVS_KEY_ATTR_CT_STATE flags */
@@ -793,25 +808,6 @@ struct ovs_action_push_eth {
struct ovs_key_ethernet addresses;
 };
 
-#define OVS_ENCAP_NSH_MAX_MD_LEN 248
-/*
- * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
- * @flags: NSH header flags.
- * @mdtype: NSH metadata type.
- * @mdlen: Length of NSH metadata in bytes, including padding.
- * @np: NSH next_protocol: Inner packet type.
- * @path_hdr: NSH service path id and service index.
- * @metadata: NSH context metadata, padded to 4-bytes
- */
-struct ovs_action_encap_nsh {
-uint8_t flags;
-uint8_t mdtype;
-uint8_t mdlen;
-uint8_t np;
-__be32 path_hdr;
-uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN];
-};
-
 /**
  * enum ovs_nat_attr - Attributes for %OVS_CT_ATTR_NAT.
  *
@@ -887,8 +883,8 @@ enum ovs_nat_attr {
  * @OVS_ACTION_ATTR_PUSH_ETH: Push a new outermost Ethernet header onto the
  * packet.
  * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the packet.
- * @OVS_ACTION_ATTR_ENCAP_NSH: encap NSH action to push NSH header.
- * @OVS_ACTION_ATTR_DECAP_NSH: decap NSH action to remove NSH header.
+ * @OVS_ACTION_ATTR_PUSH_NSH: push NSH header to the packet.
+ * @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -930,8 +926,8 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*.  */
OVS_ACTION_ATTR_METER, /* u32 meter