Re: [ovs-dev] [PATCH v6] gre: Restructure tunneling.
I get a warning in gso.c after this patch:
datapath/linux/gso.c: In function ‘rpl_ip_local_out’:
datapath/linux/gso.c:134:174: warning: ‘id’ may be used uninitialized in
this function [-Wuninitialized]
Could you take a look at this?
On Fri, Jun 21, 2013 at 3:14 PM, Pravin Shelar wrote:
> On Thu, Jun 20, 2013 at 5:50 PM, Jesse Gross wrote:
> > On Thu, Jun 20, 2013 at 5:19 PM, Pravin B Shelar
> wrote:
> >> diff --git a/datapath/linux/compat/skbuff-openvswitch.c
> b/datapath/linux/compat/skbuff-openvswitch.c
> >> index ef43ba9..720bc18 100644
> >> --- a/datapath/linux/compat/skbuff-openvswitch.c
> >> +++ b/datapath/linux/compat/skbuff-openvswitch.c
> >> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,19)
> >> +int skb_checksum_help(struct sk_buff *skb, int inward)
> >> +#else
> >> +int skb_checksum_help(struct sk_buff *skb)
> >> +#endif
> >> +{
> >> + /* Use default features for dst device. */
> >> + if (unlikely(skb_is_nonlinear(skb))) {
> >> + int err;
> >
> > I think the above comment no longer applies since we're not pulling
> > any device features.
> >
> >> diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
> >> index dae35ac..fda6481 100644
> >> --- a/datapath/vport-gre.c
> >> +++ b/datapath/vport-gre.c
> >> +static int __send(struct vport *vport, struct sk_buff *skb,
> >> + int tunnel_hlen,
> >> + __be32 seq, __be16 gre64_flag)
> > [...]
> >> - ovs_tnl_rcv(vport, skb, &tun_key);
> >> - return 0;
> >> + /* Push Tunnel header. */
> >> + skb = __build_header(skb, tunnel_hlen, seq, gre64_flag);
> >> + if (unlikely(!skb)) {
> >> + err = 0;
> >> + goto err_free_rt;
> >> + }
> >>
> >> + df = OVS_CB(skb)->tun_key->tun_flags & TUNNEL_DONT_FRAGMENT ?
> >> + htons(IP_DF) : 0;
> >> +
> >> + /*
> >> +* Allow our local IP stack to fragment the outer packet even
> >> +* if the DF bit is set as a last resort. We also need to
> >> +* force selection of an IP ID here because Linux will
> >> +* otherwise leave it at 0 if the packet originally had DF set.
> >> +*/
> >
> > I believe this comment was deleted/moved in the upstream version.
> >
> > Those are both minor though so
> > Acked-by: Jesse Gross
>
> Thanks,
> I pushed code with above changes and compat checksum check in
> iptunnel_pull_header().
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Support for Linux 3.10
Hi OpenvSwitch Devs Ubuntu Saucy is about to bump kernel version from 3.9 to 3.10; I started looking at the impact of this on ovs but swiftly went down the rabbit hole of not really understanding how the introduction of 802.1ad support in this kernel version would impact on the compat layer of ovs; specifically the following kernel commits: net: vlan: rename NETIF_F_HW_VLAN_* feature flags to NETIF_F_HW_VLAN_CTAG_* http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f646968f8f7c624587de729115d802372b9063dd net: vlan: add 802.1ad support http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8ad227ff89a7e6f05d07cd0acfd95ed3a24450ca http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4ada8db38a44654446fe35ceb20a1972220e0f69 net: vlan: add protocol argument to packet tagging functions http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=86a9bad3ab6b6f858fd4443b48738cabbb6d094c I was unsure as to whether this was suitable for inclusion in datapath/compat/* or whether the datapath module itself should have conditional code to deal with >= 3.10.0. Help/guidance much appreciated. Cheers James -- James Page Ubuntu Core Developer Debian Maintainer james.p...@ubuntu.com ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 02/11] ovs-thread: Add per-thread data support.
On 20 June 2013 20:10, Ben Pfaff wrote: > Can you confirm that the autoconf test needs #include but > the actual program doesn't? It looks funny. Yes. It looks like it should be added in the actual program; right wow we end up with the definition leaking from another header. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] autoconf test OVS_ENABLE_OPTION fails to detect options unsupported by clang
In clang's attempt to be GCC compatible it ignores, but does not fail on, unknown -W options. Configure then ends up added the option to WARNING_FLAGS, and the warning is then emitted for every file compiled. >From config.log: configure:7388: checking whether clang accepts -Wno-override-init configure:7406: clang -c -g -O2 -Wno-override-init conftest.c >&5 clang: warning: unknown warning option '-Wno-override-init'; did you mean '-Wno-over-aligned'? warning: unknown warning option '-Wno-override-init'; did you mean '-Wno-over-aligned'? [-Wunknown-warning-option] 1 warning generated. configure:7406: $? = 0 With a quick search I didn't find a canonical way to test this in autoconf, although adding -Werror does change the return value to 1 in this case. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] linux-next: Tree for Jun 21 (netdev: openvswitch)
On 06/21/13 01:17, Stephen Rothwell wrote: > Hi all, > > Happy solstice! > > Changes since 20130620: > when CONFIG_INET is not enabled: CC net/openvswitch/flow.o In file included from net/openvswitch/flow.c:43:0: include/net/ip_tunnels.h: In function 'tunnel_ip_select_ident': include/net/ip_tunnels.h:155:3: error: implicit declaration of function '__ip_select_ident' [-Werror=implicit-function-declaration] -- ~Randy ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ext-260 v3 1/5] ofp-errors: Fix typos in comment.
Thanks, applied.
On Thu, Jun 20, 2013 at 10:37:36PM -0700, Alex Wang wrote:
> Looks good to me.,
>
>
> On Wed, Jun 12, 2013 at 11:35 AM, Ben Pfaff wrote:
>
> > Signed-off-by: Ben Pfaff
> > ---
> > lib/ofp-errors.h |6 +++---
> > 1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
> > index 593241d..aa3ba50 100644
> > --- a/lib/ofp-errors.h
> > +++ b/lib/ofp-errors.h
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> > *
> > * Licensed under the Apache License, Version 2.0 (the "License");
> > * you may not use this file except in compliance with the License.
> > @@ -108,7 +108,7 @@ enum ofperr {
> >
> > /* NX1.0(1,512), OF1.1+(1,9). Specified table-id invalid or does not
> > exist.
> > * [ A non-standard error (1,512), formerly OFPERR_NXBRC_BAD_TABLE_ID,
> > - * is used for OpenFlow 1.0 as there seems to be no appropriste
> > error
> > + * is used for OpenFlow 1.0 as there seems to be no appropriate
> > error
> > * code defined the specification. ] */
> > OFPERR_OFPBRC_BAD_TABLE_ID,
> >
> > @@ -118,7 +118,7 @@ enum ofperr {
> > /* NX1.0(1,514), NX1.1(1,514), OF1.2+(1,11). Invalid port.
> > * [ A non-standard error (1,514), formerly
> > * OFPERR_NXBRC_BAD_IN_PORT is used for OpenFlow 1.0 and 1.1 as
> > there
> > - * seems to be no appropriste error code defined the
> > specifications. ] */
> > + * seems to be no appropriate error code defined the
> > specifications. ] */
> > OFPERR_OFPBRC_BAD_PORT,
> >
> > /* OF1.2+(1,12). Invalid packet in packet-out. */
> > --
> > 1.7.2.5
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] autoconf test OVS_ENABLE_OPTION fails to detect options unsupported by clang
On Fri, Jun 21, 2013 at 10:03:21AM -0400, Ed Maste wrote: > In clang's attempt to be GCC compatible it ignores, but does not fail > on, unknown -W options. Configure then ends up added the option to > WARNING_FLAGS, and the warning is then emitted for every file > compiled. > > From config.log: > > configure:7388: checking whether clang accepts -Wno-override-init > configure:7406: clang -c -g -O2 -Wno-override-init conftest.c >&5 > clang: warning: unknown warning option '-Wno-override-init'; did you > mean '-Wno-over-aligned'? > warning: unknown warning option '-Wno-override-init'; did you mean > '-Wno-over-aligned'? [-Wunknown-warning-option] > 1 warning generated. > configure:7406: $? = 0 > > With a quick search I didn't find a canonical way to test this in > autoconf, although adding -Werror does change the return value to 1 in > this case. Here's an attempt. It works with my local clang, which is: blp@blp:~/nicira/ovs/_build(0)$ clang --version Debian clang version 3.0-6.2 (tags/RELEASE_30/final) (based on LLVM 3.0) Target: i386-pc-linux-gnu Thread model: posix blp@blp:~/nicira/ovs/_build(0)$ --8<--cut here-->8-- From: Ben Pfaff Date: Fri, 21 Jun 2013 09:03:44 -0700 Subject: [PATCH] acinclude: Improve detection of not-understood compiler options with clang. By default, clang warns about but does not fail on unknown -W options. This made configure add the option to WARNING_FLAGS, which caused the warning about not-understood warnings to be emitted for every file compiled. In combination with -Werror, clang does fail on unknown -W options. This commit adds -Werror during configure's warning tests, which should cause the not-understood warnings to be detected that way. Reported-by: Ed Maste Signed-off-by: Ben Pfaff --- acinclude.m4 | 27 +-- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/acinclude.m4 b/acinclude.m4 index eadc9a7..717c681 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -362,15 +362,15 @@ dnl This file is free software; the Free Software Foundation dnl gives unlimited permission to copy and/or distribute it, dnl with or without modifications, as long as this notice is preserved. -dnl OVS_CHECK_CC_OPTION([OPTION], [ACTION-IF-ACCEPTED], [ACTION-IF-REJECTED]) -dnl Check whether the given C compiler OPTION is accepted. -dnl If so, execute ACTION-IF-ACCEPTED, otherwise ACTION-IF-REJECTED. -AC_DEFUN([OVS_CHECK_CC_OPTION], -[ +AC_DEFUN([_OVS_CHECK_CC_OPTION], [dnl m4_define([ovs_cv_name], [ovs_cv_[]m4_translit([$1], [-], [_])])dnl AC_CACHE_CHECK([whether $CC accepts $1], [ovs_cv_name], [ovs_save_CFLAGS="$CFLAGS" - CFLAGS="$CFLAGS $1" + dnl Include -Werror in the compiler options, because without -Werror + dnl clang's GCC-compatible compiler driver does not return a failure + dnl exit status even though it complains about options it does not + dnl understand. + CFLAGS="$CFLAGS $WERROR $1" AC_COMPILE_IFELSE([AC_LANG_PROGRAM(,)], [ovs_cv_name[]=yes], [ovs_cv_name[]=no]) CFLAGS="$ovs_save_CFLAGS"]) if test $ovs_cv_name = yes; then @@ -380,6 +380,21 @@ AC_DEFUN([OVS_CHECK_CC_OPTION], fi ]) +dnl OVS_CHECK_WERROR +dnl +dnl Check whether the C compiler accepts -Werror. +dnl Sets $WERROR to "-Werror", if so, and otherwise to the empty string. +AC_DEFUN([OVS_CHECK_WERROR], + [WERROR= + _OVS_CHECK_CC_OPTION([-Werror], [WERROR=-Werror])]) + +dnl OVS_CHECK_CC_OPTION([OPTION], [ACTION-IF-ACCEPTED], [ACTION-IF-REJECTED]) +dnl Check whether the given C compiler OPTION is accepted. +dnl If so, execute ACTION-IF-ACCEPTED, otherwise ACTION-IF-REJECTED. +AC_DEFUN([OVS_CHECK_CC_OPTION], + [AC_REQUIRE([OVS_CHECK_WERROR]) + _OVS_CHECK_CC_OPTION([$1], [$2], [$3])]) + dnl OVS_ENABLE_OPTION([OPTION]) dnl Check whether the given C compiler OPTION is accepted. dnl If so, add it to WARNING_FLAGS. -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Fix a kernel crash caused by corrupted mask list.
When flow table is copied, the mask list from the old table is not properly copied into the new table. The corrupted mask list in the new table will lead to kernel crash. This patch fixes this bug. Bug #18110 Reported-by: Justin Pettit Signed-off-by: Andy Zhou --- datapath/flow.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datapath/flow.c b/datapath/flow.c index 38b9502..5d97ce1 100644 --- a/datapath/flow.c +++ b/datapath/flow.c @@ -563,7 +563,7 @@ static void flow_table_copy_flows(struct flow_table *old, struct flow_table *new __tbl_insert(new, flow); } - new->mask_list = old->mask_list; + list_replace(&old->mask_list, &new->mask_list); old->keep_flows = true; } -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Bug fix: Missing mask attributes
On Thu, Jun 20, 2013 at 11:38 PM, Andy Zhou wrote: > Fix a bug where some mask attributes are missing in the netlink packets > from kernel to the user space. > > Reported-by: Justin Pettit > Signed-off-by: Andy Zhou Can you describe this some more? Presumably, this is because some of the prerequisite values are either fully or partially masked out and this is preventing latter masks from being emitted. However, since we force prerequisites to be exact match, doesn't that mean the omitted masks are fully wildcarded? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Bug fix: Missing mask attributes
Yes, you are right about the root cause. A fully wildcarded ip proto in the key would prevent the mask of ICMP (may not be fully wildcarded) to be generated. This patch fixes it. Andy On Fri, Jun 21, 2013 at 9:27 AM, Jesse Gross wrote: > On Thu, Jun 20, 2013 at 11:38 PM, Andy Zhou wrote: > > Fix a bug where some mask attributes are missing in the netlink packets > > from kernel to the user space. > > > > Reported-by: Justin Pettit > > Signed-off-by: Andy Zhou > > Can you describe this some more? Presumably, this is because some of > the prerequisite values are either fully or partially masked out and > this is preventing latter masks from being emitted. However, since we > force prerequisites to be exact match, doesn't that mean the omitted > masks are fully wildcarded? > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Bug fix: Missing mask attributes
Correction. The ICMP mask will have to be fully masked in this example. But they still need to be present in the netlink. A missing ICMP mask attribute will be interpreted by the user space programs as exact match. On Fri, Jun 21, 2013 at 9:38 AM, Andy Zhou wrote: > Yes, you are right about the root cause. > > A fully wildcarded ip proto in the key would prevent the mask of ICMP (may > not be fully wildcarded) to be generated. > This patch fixes it. > > Andy > > > On Fri, Jun 21, 2013 at 9:27 AM, Jesse Gross wrote: > >> On Thu, Jun 20, 2013 at 11:38 PM, Andy Zhou wrote: >> > Fix a bug where some mask attributes are missing in the netlink packets >> > from kernel to the user space. >> > >> > Reported-by: Justin Pettit >> > Signed-off-by: Andy Zhou >> >> Can you describe this some more? Presumably, this is because some of >> the prerequisite values are either fully or partially masked out and >> this is preventing latter masks from being emitted. However, since we >> force prerequisites to be exact match, doesn't that mean the omitted >> masks are fully wildcarded? >> > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Bug fix: Missing mask attributes
It doesn't really make sense that userspace would interpret a missing attribute as exact match because when the flow is installed a missing attribute means that it is wildcarded. Therefore, if you echoed back the exact same flow it would have a different meaning. On Fri, Jun 21, 2013 at 9:43 AM, Andy Zhou wrote: > Correction. > > The ICMP mask will have to be fully masked in this example. But they still > need to be present in the netlink. A missing ICMP mask attribute will be > interpreted by the user space programs as exact match. > > > > > > On Fri, Jun 21, 2013 at 9:38 AM, Andy Zhou wrote: >> >> Yes, you are right about the root cause. >> >> A fully wildcarded ip proto in the key would prevent the mask of ICMP (may >> not be fully wildcarded) to be generated. >> This patch fixes it. >> >> Andy >> >> >> On Fri, Jun 21, 2013 at 9:27 AM, Jesse Gross wrote: >>> >>> On Thu, Jun 20, 2013 at 11:38 PM, Andy Zhou wrote: >>> > Fix a bug where some mask attributes are missing in the netlink packets >>> > from kernel to the user space. >>> > >>> > Reported-by: Justin Pettit >>> > Signed-off-by: Andy Zhou >>> >>> Can you describe this some more? Presumably, this is because some of >>> the prerequisite values are either fully or partially masked out and >>> this is preventing latter masks from being emitted. However, since we >>> force prerequisites to be exact match, doesn't that mean the omitted >>> masks are fully wildcarded? >> >> > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Bug fix: Missing mask attributes
You are right. The user space code needs to distinguish between two cases: 1) The entire mask attribute is missing, then the entire key is exact match. 2) The mask attribute is there, then the missing key attributes should be treated as wildcarded fields. It seems we don't need this patch, but make fixes to the user space program instead. What do you think? On Fri, Jun 21, 2013 at 9:55 AM, Jesse Gross wrote: > It doesn't really make sense that userspace would interpret a missing > attribute as exact match because when the flow is installed a missing > attribute means that it is wildcarded. Therefore, if you echoed back > the exact same flow it would have a different meaning. > > On Fri, Jun 21, 2013 at 9:43 AM, Andy Zhou wrote: > > Correction. > > > > The ICMP mask will have to be fully masked in this example. But they > still > > need to be present in the netlink. A missing ICMP mask attribute will be > > interpreted by the user space programs as exact match. > > > > > > > > > > > > On Fri, Jun 21, 2013 at 9:38 AM, Andy Zhou wrote: > >> > >> Yes, you are right about the root cause. > >> > >> A fully wildcarded ip proto in the key would prevent the mask of ICMP > (may > >> not be fully wildcarded) to be generated. > >> This patch fixes it. > >> > >> Andy > >> > >> > >> On Fri, Jun 21, 2013 at 9:27 AM, Jesse Gross wrote: > >>> > >>> On Thu, Jun 20, 2013 at 11:38 PM, Andy Zhou wrote: > >>> > Fix a bug where some mask attributes are missing in the netlink > packets > >>> > from kernel to the user space. > >>> > > >>> > Reported-by: Justin Pettit > >>> > Signed-off-by: Andy Zhou > >>> > >>> Can you describe this some more? Presumably, this is because some of > >>> the prerequisite values are either fully or partially masked out and > >>> this is preventing latter masks from being emitted. However, since we > >>> force prerequisites to be exact match, doesn't that mean the omitted > >>> masks are fully wildcarded? > >> > >> > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] autoconf test OVS_ENABLE_OPTION fails to detect options unsupported by clang
On 21 June 2013 12:05, Ben Pfaff wrote: > By default, clang warns about but does not fail on unknown -W options. > This made configure add the option to WARNING_FLAGS, which caused the > warning about not-understood warnings to be emitted for every file > compiled. > > In combination with -Werror, clang does fail on unknown -W options. This > commit adds -Werror during configure's warning tests, which should cause > the not-understood warnings to be detected that way. > > Reported-by: Ed Maste > Signed-off-by: Ben Pfaff Thanks Ben, looks good and it works for me. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Bug fix: Missing mask attributes
That sounds right to me. On Fri, Jun 21, 2013 at 10:06 AM, Andy Zhou wrote: > You are right. > > The user space code needs to distinguish between two cases: > 1) The entire mask attribute is missing, then the entire key is exact match. > 2) The mask attribute is there, then the missing key attributes should be > treated as wildcarded fields. > > It seems we don't need this patch, but make fixes to the user space program > instead. What do you think? > > > On Fri, Jun 21, 2013 at 9:55 AM, Jesse Gross wrote: >> >> It doesn't really make sense that userspace would interpret a missing >> attribute as exact match because when the flow is installed a missing >> attribute means that it is wildcarded. Therefore, if you echoed back >> the exact same flow it would have a different meaning. >> >> On Fri, Jun 21, 2013 at 9:43 AM, Andy Zhou wrote: >> > Correction. >> > >> > The ICMP mask will have to be fully masked in this example. But they >> > still >> > need to be present in the netlink. A missing ICMP mask attribute will be >> > interpreted by the user space programs as exact match. >> > >> > >> > >> > >> > >> > On Fri, Jun 21, 2013 at 9:38 AM, Andy Zhou wrote: >> >> >> >> Yes, you are right about the root cause. >> >> >> >> A fully wildcarded ip proto in the key would prevent the mask of ICMP >> >> (may >> >> not be fully wildcarded) to be generated. >> >> This patch fixes it. >> >> >> >> Andy >> >> >> >> >> >> On Fri, Jun 21, 2013 at 9:27 AM, Jesse Gross wrote: >> >>> >> >>> On Thu, Jun 20, 2013 at 11:38 PM, Andy Zhou wrote: >> >>> > Fix a bug where some mask attributes are missing in the netlink >> >>> > packets >> >>> > from kernel to the user space. >> >>> > >> >>> > Reported-by: Justin Pettit >> >>> > Signed-off-by: Andy Zhou >> >>> >> >>> Can you describe this some more? Presumably, this is because some of >> >>> the prerequisite values are either fully or partially masked out and >> >>> this is preventing latter masks from being emitted. However, since we >> >>> force prerequisites to be exact match, doesn't that mean the omitted >> >>> masks are fully wildcarded? >> >> >> >> >> > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Bug fix: Missing mask attributes
Thank you for pointing this out. Please disregard the patch. On Fri, Jun 21, 2013 at 10:12 AM, Jesse Gross wrote: > That sounds right to me. > > On Fri, Jun 21, 2013 at 10:06 AM, Andy Zhou wrote: > > You are right. > > > > The user space code needs to distinguish between two cases: > > 1) The entire mask attribute is missing, then the entire key is exact > match. > > 2) The mask attribute is there, then the missing key attributes should be > > treated as wildcarded fields. > > > > It seems we don't need this patch, but make fixes to the user space > program > > instead. What do you think? > > > > > > On Fri, Jun 21, 2013 at 9:55 AM, Jesse Gross wrote: > >> > >> It doesn't really make sense that userspace would interpret a missing > >> attribute as exact match because when the flow is installed a missing > >> attribute means that it is wildcarded. Therefore, if you echoed back > >> the exact same flow it would have a different meaning. > >> > >> On Fri, Jun 21, 2013 at 9:43 AM, Andy Zhou wrote: > >> > Correction. > >> > > >> > The ICMP mask will have to be fully masked in this example. But they > >> > still > >> > need to be present in the netlink. A missing ICMP mask attribute will > be > >> > interpreted by the user space programs as exact match. > >> > > >> > > >> > > >> > > >> > > >> > On Fri, Jun 21, 2013 at 9:38 AM, Andy Zhou wrote: > >> >> > >> >> Yes, you are right about the root cause. > >> >> > >> >> A fully wildcarded ip proto in the key would prevent the mask of ICMP > >> >> (may > >> >> not be fully wildcarded) to be generated. > >> >> This patch fixes it. > >> >> > >> >> Andy > >> >> > >> >> > >> >> On Fri, Jun 21, 2013 at 9:27 AM, Jesse Gross > wrote: > >> >>> > >> >>> On Thu, Jun 20, 2013 at 11:38 PM, Andy Zhou > wrote: > >> >>> > Fix a bug where some mask attributes are missing in the netlink > >> >>> > packets > >> >>> > from kernel to the user space. > >> >>> > > >> >>> > Reported-by: Justin Pettit > >> >>> > Signed-off-by: Andy Zhou > >> >>> > >> >>> Can you describe this some more? Presumably, this is because some of > >> >>> the prerequisite values are either fully or partially masked out and > >> >>> this is preventing latter masks from being emitted. However, since > we > >> >>> force prerequisites to be exact match, doesn't that mean the omitted > >> >>> masks are fully wildcarded? > >> >> > >> >> > >> > > > > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().
Looks good to me, thanks
On Wed, Jun 19, 2013 at 1:17 PM, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff
> ---
> configure.ac |1 +
> lib/util.c | 48 +---
> lib/util.h |3 ++-
> 3 files changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 6db4a00..734b2ff 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -45,6 +45,7 @@ AC_SEARCH_LIBS([pow], [m])
> AC_SEARCH_LIBS([clock_gettime], [rt])
> AC_SEARCH_LIBS([timer_create], [rt])
> AC_SEARCH_LIBS([pthread_sigmask], [pthread])
> +AC_FUNC_STRERROR_R
>
> OVS_CHECK_ESX
> OVS_CHECK_COVERAGE
> diff --git a/lib/util.c b/lib/util.c
> index 2a06461..6ee8b5c 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -18,6 +18,7 @@
> #include "util.h"
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -45,6 +46,9 @@ const char *subprogram_name = "";
> /* --version option output. */
> static char *program_version;
>
> +/* Buffer used by ovs_strerror(). */
> +DEFINE_PER_THREAD_DATA(struct { char s[128]; }, strerror_buffer, { "" });
> +
>
This is really cool, learned the Variadic Macros ;D
> void
> ovs_assert_failure(const char *where, const char *function,
> const char *condition)
> @@ -307,19 +311,41 @@ ovs_error_valist(int err_no, const char *format,
> va_list args)
> const char *
> ovs_retval_to_string(int retval)
> {
> -static char unknown[48];
> +return (!retval ? ""
> +: retval == EOF ? "End of file"
> +: ovs_strerror(retval));
> +}
>
> -if (!retval) {
> -return "";
> -}
> -if (retval > 0) {
> -return strerror(retval);
> -}
> -if (retval == EOF) {
> -return "End of file";
> +const char *
> +ovs_strerror(int error)
> +{
> +enum { BUFSIZE = sizeof strerror_buffer_get()->s };
> +int save_errno;
> +char *buffer;
> +char *s;
> +
> +save_errno = errno;
> +buffer = strerror_buffer_get()->s;
> +
> +#if STRERROR_R_CHAR_P
> +/* GNU style strerror_r() might return an immutable static string, or
> it
> + * might write and return 'buffer', but in either case we can pass the
> + * returned string directly to the caller. */
> +s = strerror_r(error, buffer, BUFSIZE);
> +#else /* strerror_r() returns an int. */
> +s = buffer;
> +if (strerror_r(error, buffer, BUFSIZE)) {
> +/* strerror_r() is only allowed to fail on ERANGE (because the
> buffer
> + * is too short). We don't check the actual failure reason
> because
> + * POSIX requires strerror_r() to return the error but old glibc
> + * (before 2.13) returns -1 and sets errno. */
> +snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
> }
> -snprintf(unknown, sizeof unknown, "***unknown return value: %d***",
> retval);
> -return unknown;
> +#endif
> +
> +errno = save_errno;
> +
> +return s;
> }
>
> /* Sets global "program_name" and "program_version" variables. Should
> diff --git a/lib/util.h b/lib/util.h
> index f5589e3..d7fbe09 100644
> --- a/lib/util.h
> +++ b/lib/util.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -213,6 +213,7 @@ void ovs_error(int err_no, const char *format, ...)
> PRINTF_FORMAT(2, 3);
> void ovs_error_valist(int err_no, const char *format, va_list)
> PRINTF_FORMAT(2, 0);
> const char *ovs_retval_to_string(int);
> +const char *ovs_strerror(int);
> void ovs_hex_dump(FILE *, const void *, size_t, uintptr_t offset, bool
> ascii);
>
> bool str_to_int(const char *, int base, int *);
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 01/11] ovs-thread: New module, initially just with pthreads wrapper functions.
The #include_next in sparse/pthread.h is a bit exotic. Would you
please include a short comment explaining why it's necessary?
sparse/pthread.h has some trailing newlines at the end of the file.
In ovs-thread.c any reason not to macro-ize the trylock functions as well?
Acked-by: Ethan Jackson
On Wed, Jun 19, 2013 at 1:17 PM, Ben Pfaff wrote:
> The only tricky part here is that I'm throwing in annotations to allow
> "sparse" to report unbalanced locking.
>
> Signed-off-by: Ben Pfaff
> ---
> include/sparse/automake.mk |1 +
> include/sparse/pthread.h | 61 +
> lib/automake.mk|2 +
> lib/compiler.h | 36 ++-
> lib/ovs-thread.c | 108
>
> lib/ovs-thread.h | 89
> 6 files changed, 296 insertions(+), 1 deletions(-)
> create mode 100644 include/sparse/pthread.h
> create mode 100644 lib/ovs-thread.c
> create mode 100644 lib/ovs-thread.h
>
> diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
> index 1a77500..45ae1f5 100644
> --- a/include/sparse/automake.mk
> +++ b/include/sparse/automake.mk
> @@ -4,5 +4,6 @@ noinst_HEADERS += \
> include/sparse/math.h \
> include/sparse/netinet/in.h \
> include/sparse/netinet/ip6.h \
> +include/sparse/pthread.h \
> include/sparse/sys/socket.h \
> include/sparse/sys/wait.h
> diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
> new file mode 100644
> index 000..1e54e95
> --- /dev/null
> +++ b/include/sparse/pthread.h
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (c) 2013 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef __CHECKER__
> +#error "Use this header only with sparse. It is not a correct
> implementation."
> +#endif
> +
> +#include_next
> +
> +#include "compiler.h"
> +
> +int pthread_mutex_lock(pthread_mutex_t *mutex) OVS_ACQUIRES(mutex);
> +int pthread_mutex_unlock(pthread_mutex_t *mutex) OVS_RELEASES(mutex);
> +
> +int pthread_rwlock_rdlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock);
> +int pthread_rwlock_wrlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock);
> +int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) OVS_RELEASES(rwlock);
> +
> +int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
> +OVS_MUST_HOLD(mutex);
> +
> +#define pthread_mutex_trylock(MUTEX)\
> +({ \
> +int retval = pthread_mutex_trylock(mutex); \
> +if (!retval) { \
> +OVS_ACQUIRE(MUTEX); \
> +} \
> +retval; \
> +})
> +
> +#define pthread_rwlock_tryrdlock(RWLOCK)\
> +({ \
> +int retval = pthread_rwlock_tryrdlock(rwlock); \
> +if (!retval) { \
> +OVS_ACQUIRE(RWLOCK);\
> +} \
> +retval; \
> +})
> +#define pthread_rwlock_trywrlock(RWLOCK)\
> +({ \
> +int retval = pthread_rwlock_trywrlock(rwlock); \
> +if (!retval) { \
> +OVS_ACQUIRE(RWLOCK);\
> +} \
> +retval; \
> +})
> +
> +
> diff --git a/lib/automake.mk b/lib/automake.mk
> index eec71dd..c6de4fe 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -121,6 +121,8 @@ lib_libopenvswitch_a_SOURCES = \
> lib/ofp-version-opt.c \
> lib/ofpbuf.c \
> lib/ofpbuf.h \
> + lib/ovs-thread.c \
> + lib/ovs-thread.h \
> lib/ovsdb-data.c \
> lib/ovsdb-data.h \
> lib/ovsdb-error.c \
> diff --git a/lib/compiler.h b/lib/compiler.h
> index 760389d..f3cbe96 100644
> --- a/lib/compiler.h
> +++ b/lib/compiler.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * L
Re: [ovs-dev] [PATCH] ovs-xapi-sync: Increase the tolerance level for xapi failures for bridges.
On Wed, Jun 19, 2013 at 1:27 PM, Ben Pfaff wrote: > On Wed, Jun 19, 2013 at 02:03:15AM -0700, Gurucharan Shetty wrote: > > Specifically for the case, where we know that a bridge record > > should exist in xapi, if we don't get any bridge records, log > > the failure and retry again later. > > > > Signed-off-by: Gurucharan Shetty > > Could we add a helper function for the common code here? > I decided to abandon this patch because this makes an assumption that all the bridges that are created in xenserver are through standard xapi methods. If anyone creates an asynchronous bridge through command line (ex: using ovs-vsctl), ovs-xapi-sync will think that xapi is down because there are no records for the new bridge. If xapi is completely down, ovs-xapi-sync will crash and restart. During the restart, it will wait for xapi to start. So we are protected against that. For the hypothetical scenario of xapi giving wrong information, there is only so much you can do. Thanks, Guru > Thanks, > > Ben. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Fix uninitializaed variable with GRE GSO.
If a packet is neither GSO nor CHECKSUM_PARTIAL then the
GRE GSO compatibility code can use an uninitialized IP ID.
CC: Pravin Shelar
Reported-by: Joe Stringer
Signed-off-by: Jesse Gross
---
datapath/linux/compat/gso.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
index 7379a57..3cadde9 100644
--- a/datapath/linux/compat/gso.c
+++ b/datapath/linux/compat/gso.c
@@ -103,7 +103,7 @@ free:
int rpl_ip_local_out(struct sk_buff *skb)
{
int ret = NETDEV_TX_OK;
- int id;
+ int id = -1;
if (skb_is_gso(skb)) {
struct iphdr *iph;
@@ -119,7 +119,6 @@ int rpl_ip_local_out(struct sk_buff *skb)
err = skb_checksum_help(skb);
if (unlikely(err))
return 0;
- id = -1;
}
while (skb) {
--
1.8.1.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v6] gre: Restructure tunneling.
Thanks, it looks like a real issue to me. I sent out a patch that should fix it.
On Fri, Jun 21, 2013 at 1:19 AM, Joe Stringer wrote:
> I get a warning in gso.c after this patch:
>
> datapath/linux/gso.c: In function ‘rpl_ip_local_out’:
> datapath/linux/gso.c:134:174: warning: ‘id’ may be used uninitialized in
> this function [-Wuninitialized]
>
> Could you take a look at this?
>
>
> On Fri, Jun 21, 2013 at 3:14 PM, Pravin Shelar wrote:
>>
>> On Thu, Jun 20, 2013 at 5:50 PM, Jesse Gross wrote:
>> > On Thu, Jun 20, 2013 at 5:19 PM, Pravin B Shelar
>> > wrote:
>> >> diff --git a/datapath/linux/compat/skbuff-openvswitch.c
>> >> b/datapath/linux/compat/skbuff-openvswitch.c
>> >> index ef43ba9..720bc18 100644
>> >> --- a/datapath/linux/compat/skbuff-openvswitch.c
>> >> +++ b/datapath/linux/compat/skbuff-openvswitch.c
>> >> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,19)
>> >> +int skb_checksum_help(struct sk_buff *skb, int inward)
>> >> +#else
>> >> +int skb_checksum_help(struct sk_buff *skb)
>> >> +#endif
>> >> +{
>> >> + /* Use default features for dst device. */
>> >> + if (unlikely(skb_is_nonlinear(skb))) {
>> >> + int err;
>> >
>> > I think the above comment no longer applies since we're not pulling
>> > any device features.
>> >
>> >> diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
>> >> index dae35ac..fda6481 100644
>> >> --- a/datapath/vport-gre.c
>> >> +++ b/datapath/vport-gre.c
>> >> +static int __send(struct vport *vport, struct sk_buff *skb,
>> >> + int tunnel_hlen,
>> >> + __be32 seq, __be16 gre64_flag)
>> > [...]
>> >> - ovs_tnl_rcv(vport, skb, &tun_key);
>> >> - return 0;
>> >> + /* Push Tunnel header. */
>> >> + skb = __build_header(skb, tunnel_hlen, seq, gre64_flag);
>> >> + if (unlikely(!skb)) {
>> >> + err = 0;
>> >> + goto err_free_rt;
>> >> + }
>> >>
>> >> + df = OVS_CB(skb)->tun_key->tun_flags & TUNNEL_DONT_FRAGMENT ?
>> >> + htons(IP_DF) : 0;
>> >> +
>> >> + /*
>> >> +* Allow our local IP stack to fragment the outer packet even
>> >> +* if the DF bit is set as a last resort. We also need to
>> >> +* force selection of an IP ID here because Linux will
>> >> +* otherwise leave it at 0 if the packet originally had DF set.
>> >> +*/
>> >
>> > I believe this comment was deleted/moved in the upstream version.
>> >
>> > Those are both minor though so
>> > Acked-by: Jesse Gross
>>
>> Thanks,
>> I pushed code with above changes and compat checksum check in
>> iptunnel_pull_header().
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH net-next 2/2] ip_tunnel: Protect tunnel functions with CONFIG_INET guard.
Tunnel constants can be used in generic code but in these cases
the inline functions in ip_tunnels.h cause compilation problems
if CONFIG_INET is not set.
CC: Pravin Shelar
Reported-by: Randy Dunlap
Signed-off-by: Jesse Gross
---
include/net/ip_tunnels.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 10bbb42..b0d9824 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -93,6 +93,8 @@ struct ip_tunnel_net {
struct net_device *fb_tunnel_dev;
};
+#ifdef CONFIG_INET
+
int ip_tunnel_init(struct net_device *dev);
void ip_tunnel_uninit(struct net_device *dev);
void ip_tunnel_dellink(struct net_device *dev, struct list_head *head);
@@ -180,4 +182,7 @@ static inline void iptunnel_xmit_stats(int err,
err_stats->tx_dropped++;
}
}
+
+#endif /* CONFIG_INET */
+
#endif /* __NET_IP_TUNNELS_H */
--
1.8.1.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH net-next 1/2] openvswitch: Allow GRE ports when compiled as a module.
Open vSwitch GRE tunnels are currently only enabled if the GRE
datapath code is statically compiled in but it should be possible
to use it as a module as well.
CC: Pravin Shelar
Signed-off-by: Jesse Gross
---
net/openvswitch/vport-gre.c | 2 +-
net/openvswitch/vport.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
index 3a8d190..943e5c4 100644
--- a/net/openvswitch/vport-gre.c
+++ b/net/openvswitch/vport-gre.c
@@ -16,7 +16,7 @@
* 02110-1301, USA
*/
-#ifdef CONFIG_NET_IPGRE_DEMUX
+#if IS_ENABLED(CONFIG_NET_IPGRE_DEMUX)
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index f52dfb9..ba81294 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -39,7 +39,7 @@ static const struct vport_ops *vport_ops_list[] = {
&ovs_netdev_vport_ops,
&ovs_internal_vport_ops,
-#ifdef CONFIG_NET_IPGRE_DEMUX
+#if IS_ENABLED(CONFIG_NET_IPGRE_DEMUX)
&ovs_gre_vport_ops,
#endif
};
--
1.8.1.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] linux-next: Tree for Jun 21 (netdev: openvswitch)
On Fri, Jun 21, 2013 at 8:22 AM, Randy Dunlap wrote: > On 06/21/13 01:17, Stephen Rothwell wrote: >> Hi all, >> >> Happy solstice! >> >> Changes since 20130620: >> > > when CONFIG_INET is not enabled: > > CC net/openvswitch/flow.o > In file included from net/openvswitch/flow.c:43:0: > include/net/ip_tunnels.h: In function 'tunnel_ip_select_ident': > include/net/ip_tunnels.h:155:3: error: implicit declaration of function > '__ip_select_ident' [-Werror=implicit-function-declaration] Thanks, I just sent out a couple of patches to fix config issues with the recent tunneling series. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [megaflow masklist fix V2] datapath: Fix a kernel crash caused by corrupted mask list.
When flow table is copied, the mask list from the old table
is not properly copied into the new table. The corrupted mask
list in the new table will lead to kernel crash. This patch
fixes this bug.
Bug #18110
Reported-by: Justin Pettit
---
v1 -> v2:
Jesse pointed out the race condition in v1. Fastpath can be
walking the list while the head is being replaced.
In V2, instead of have mask_list head as part of the flow
table data structure, it only has a pointer to the head. The
actual head is allocated at run time, and stay consistent with
the mask list.
Signed-off-by: Andy Zhou
---
datapath/flow.c | 36
datapath/flow.h |2 +-
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/datapath/flow.c b/datapath/flow.c
index 38b9502..864c31c 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -461,7 +461,7 @@ struct flow_table *ovs_flow_tbl_alloc(int new_size)
table->node_ver = 0;
table->keep_flows = false;
get_random_bytes(&table->hash_seed, sizeof(u32));
- INIT_LIST_HEAD(&table->mask_list);
+ table->mask_list = NULL;
return table;
}
@@ -485,6 +485,11 @@ static void __flow_tbl_destroy(struct flow_table *table)
}
}
+ if (table->mask_list) {
+ BUG_ON(!list_empty(table->mask_list));
+ kfree(table->mask_list);
+ }
+
skip_flows:
free_buckets(table->buckets);
kfree(table);
@@ -1028,12 +1033,15 @@ struct sw_flow *ovs_flow_lookup(struct flow_table *tbl,
const struct sw_flow_key *key)
{
struct sw_flow *flow = NULL;
- struct sw_flow_mask *mask;
- list_for_each_entry_rcu(mask, &tbl->mask_list, list) {
- flow = ovs_masked_flow_lookup(tbl, key, mask);
- if (flow) /* Found */
- break;
+ if (tbl->mask_list) {
+ struct sw_flow_mask *mask;
+
+ list_for_each_entry_rcu(mask, tbl->mask_list, list) {
+ flow = ovs_masked_flow_lookup(tbl, key, mask);
+ if (flow) /* Found */
+ break;
+ }
}
return flow;
@@ -1843,7 +1851,10 @@ struct sw_flow_mask *ovs_sw_flow_mask_find(const struct
flow_table *tbl,
{
struct list_head *ml;
- list_for_each(ml, &tbl->mask_list) {
+ if (!tbl->mask_list)
+ return NULL;
+
+ list_for_each(ml, tbl->mask_list) {
struct sw_flow_mask *m;
m = container_of(ml, struct sw_flow_mask, list);
if (ovs_sw_flow_mask_equal(mask, m))
@@ -1860,7 +1871,16 @@ struct sw_flow_mask *ovs_sw_flow_mask_find(const struct
flow_table *tbl,
*/
void ovs_sw_flow_mask_insert(struct flow_table *tbl, struct sw_flow_mask *mask)
{
- list_add_rcu(&mask->list, &tbl->mask_list);
+ if (!tbl->mask_list) {
+ tbl->mask_list = (struct list_head *)
+ kmalloc(sizeof(struct list_head), GFP_KERNEL);
+
+ if (tbl->mask_list)
+ INIT_LIST_HEAD(tbl->mask_list);
+ }
+
+ if (tbl->mask_list)
+ list_add_rcu(&mask->list, tbl->mask_list);
}
/**
diff --git a/datapath/flow.h b/datapath/flow.h
index 45eba03..a31dab0 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -191,7 +191,7 @@ struct flow_table {
struct flex_array *buckets;
unsigned int count, n_buckets;
struct rcu_head rcu;
- struct list_head mask_list;
+ struct list_head *mask_list;
int node_ver;
u32 hash_seed;
bool keep_flows;
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow masklist fix V2] datapath: Fix a kernel crash caused by corrupted mask list.
On Fri, Jun 21, 2013 at 4:18 PM, Andy Zhou wrote: > When flow table is copied, the mask list from the old table > is not properly copied into the new table. The corrupted mask > list in the new table will lead to kernel crash. This patch > fixes this bug. > > Bug #18110 > Reported-by: Justin Pettit Can we just allocate the mask list head when we create v1 of the table? I think that would eliminate a lot of conditions and also allow us to return -ENOMEM up front rather than silent failing later on. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Fix uninitializaed variable with GRE GSO.
Right, looks good to me.
On 22 Jun 2013 08:12, "Jesse Gross" wrote:
> If a packet is neither GSO nor CHECKSUM_PARTIAL then the
> GRE GSO compatibility code can use an uninitialized IP ID.
>
> CC: Pravin Shelar
> Reported-by: Joe Stringer
> Signed-off-by: Jesse Gross
> ---
> datapath/linux/compat/gso.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
> index 7379a57..3cadde9 100644
> --- a/datapath/linux/compat/gso.c
> +++ b/datapath/linux/compat/gso.c
> @@ -103,7 +103,7 @@ free:
> int rpl_ip_local_out(struct sk_buff *skb)
> {
> int ret = NETDEV_TX_OK;
> - int id;
> + int id = -1;
>
> if (skb_is_gso(skb)) {
> struct iphdr *iph;
> @@ -119,7 +119,6 @@ int rpl_ip_local_out(struct sk_buff *skb)
> err = skb_checksum_help(skb);
> if (unlikely(err))
> return 0;
> - id = -1;
> }
>
> while (skb) {
> --
> 1.8.1.2
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH net-next 0/8] openvswitch: Add vxlan tunneling support.
On Thu, 20 Jun 2013 13:47:11 -0700 Pravin Shelar wrote: > On Thu, Jun 20, 2013 at 10:13 AM, Stephen Hemminger > wrote: > > On Thu, 20 Jun 2013 08:58:42 -0700 > > Stephen Hemminger wrote: > > > >> On Thu, 20 Jun 2013 00:26:15 -0700 > >> Pravin B Shelar wrote: > >> > >> > First two patches fixes vxlan issues. Next two patches > >> > extends vxlan so that openvswitch can share vxlan udp port > >> > with vxlan module. Rest of patches refactors vxlan data > >> > plane so that ovs can share that code with vxlan module. > >> > Last patch adds vxlan-vport to openvswitch. > >> > > >> > Pravin B Shelar (8): > >> > vxlan: Fix error handling while creating device. > >> > vxlan: Fix sparse wornings. > >> > vxlan: Allow multiple recieve handlers. > >> > vxlan: Extend vxlan handlers for openvswitch. > >> > vxlan: Factor out vxlan send api. > >> > vxlan: Improve vxlan headroom calculation. > >> > vxlan: Add tx-vlan offload support. > >> > openvswitch: Add vxlan tunneling support. > >> > > >> > drivers/net/vxlan.c | 337 > >> > +- > >> > include/net/vxlan.h | 33 > >> > include/uapi/linux/openvswitch.h | 11 ++ > >> > net/openvswitch/Kconfig |1 + > >> > net/openvswitch/Makefile |3 +- > >> > net/openvswitch/vport-vxlan.c| 220 + > >> > net/openvswitch/vport.c |3 + > >> > net/openvswitch/vport.h |1 + > >> > 8 files changed, 490 insertions(+), 119 deletions(-) > >> > create mode 100644 include/net/vxlan.h > >> > create mode 100644 net/openvswitch/vport-vxlan.c > >> > >> I will merge the first 7 into my vxlan repository and send a pull request. > > > > The patch "vxlan: Allow multiple recieve handlers." and beyond conflict > > with the changes to manage socket in work queue. > > > > Could you rebase them against > > git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/vxlan-next.git > > > > The repo may not be available right now because kernel.org hasn't mirrored > > it out yet. > Can you sync your tree with net-next, it has ovs changes that are > required for ovs-vxlan changes. Ok vxlan-next tree is rebuilt now as fork off of latest net-next ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Fix uninitializaed variable with GRE GSO.
Thanks, I applied it.
On Fri, Jun 21, 2013 at 4:57 PM, Joe Stringer wrote:
> Right, looks good to me.
>
> On 22 Jun 2013 08:12, "Jesse Gross" wrote:
>>
>> If a packet is neither GSO nor CHECKSUM_PARTIAL then the
>> GRE GSO compatibility code can use an uninitialized IP ID.
>>
>> CC: Pravin Shelar
>> Reported-by: Joe Stringer
>> Signed-off-by: Jesse Gross
>> ---
>> datapath/linux/compat/gso.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
>> index 7379a57..3cadde9 100644
>> --- a/datapath/linux/compat/gso.c
>> +++ b/datapath/linux/compat/gso.c
>> @@ -103,7 +103,7 @@ free:
>> int rpl_ip_local_out(struct sk_buff *skb)
>> {
>> int ret = NETDEV_TX_OK;
>> - int id;
>> + int id = -1;
>>
>> if (skb_is_gso(skb)) {
>> struct iphdr *iph;
>> @@ -119,7 +119,6 @@ int rpl_ip_local_out(struct sk_buff *skb)
>> err = skb_checksum_help(skb);
>> if (unlikely(err))
>> return 0;
>> - id = -1;
>> }
>>
>> while (skb) {
>> --
>> 1.8.1.2
>>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow masklist fix V2] datapath: Fix a kernel crash caused by corrupted mask list.
O.K. How about the attached incremental patch? On Fri, Jun 21, 2013 at 4:37 PM, Jesse Gross wrote: > On Fri, Jun 21, 2013 at 4:18 PM, Andy Zhou wrote: > > When flow table is copied, the mask list from the old table > > is not properly copied into the new table. The corrupted mask > > list in the new table will lead to kernel crash. This patch > > fixes this bug. > > > > Bug #18110 > > Reported-by: Justin Pettit > > Can we just allocate the mask list head when we create v1 of the > table? I think that would eliminate a lot of conditions and also allow > us to return -ENOMEM up front rather than silent failing later on. > 0001-new-fix.patch Description: Binary data ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow masklist fix V2] datapath: Fix a kernel crash caused by corrupted mask list.
Do we need any of the tests for tbl->mask_list at this point if we always allocate a list head? On Fri, Jun 21, 2013 at 5:52 PM, Andy Zhou wrote: > O.K. How about the attached incremental patch? > > > On Fri, Jun 21, 2013 at 4:37 PM, Jesse Gross wrote: >> >> On Fri, Jun 21, 2013 at 4:18 PM, Andy Zhou wrote: >> > When flow table is copied, the mask list from the old table >> > is not properly copied into the new table. The corrupted mask >> > list in the new table will lead to kernel crash. This patch >> > fixes this bug. >> > >> > Bug #18110 >> > Reported-by: Justin Pettit >> >> Can we just allocate the mask list head when we create v1 of the >> table? I think that would eliminate a lot of conditions and also allow >> us to return -ENOMEM up front rather than silent failing later on. > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH net-next 2/2] ip_tunnel: Protect tunnel functions with CONFIG_INET guard.
On 06/21/13 16:17, Jesse Gross wrote:
> Tunnel constants can be used in generic code but in these cases
> the inline functions in ip_tunnels.h cause compilation problems
> if CONFIG_INET is not set.
>
> CC: Pravin Shelar
> Reported-by: Randy Dunlap
> Signed-off-by: Jesse Gross
Acked-by: Randy Dunlap
Thanks.
> ---
> include/net/ip_tunnels.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 10bbb42..b0d9824 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -93,6 +93,8 @@ struct ip_tunnel_net {
> struct net_device *fb_tunnel_dev;
> };
>
> +#ifdef CONFIG_INET
> +
> int ip_tunnel_init(struct net_device *dev);
> void ip_tunnel_uninit(struct net_device *dev);
> void ip_tunnel_dellink(struct net_device *dev, struct list_head *head);
> @@ -180,4 +182,7 @@ static inline void iptunnel_xmit_stats(int err,
> err_stats->tx_dropped++;
> }
> }
> +
> +#endif /* CONFIG_INET */
> +
> #endif /* __NET_IP_TUNNELS_H */
>
--
~Randy
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow masklist fix V2] datapath: Fix a kernel crash caused by corrupted mask list.
No. there is no need any more. An incremental patch is attached that removes all the tbl->mask_list checks. I have also rolled the last 3 patches in to a single one in case it is useful. On Fri, Jun 21, 2013 at 6:10 PM, Jesse Gross wrote: > Do we need any of the tests for tbl->mask_list at this point if we > always allocate a list head? > > On Fri, Jun 21, 2013 at 5:52 PM, Andy Zhou wrote: > > O.K. How about the attached incremental patch? > > > > > > On Fri, Jun 21, 2013 at 4:37 PM, Jesse Gross wrote: > >> > >> On Fri, Jun 21, 2013 at 4:18 PM, Andy Zhou wrote: > >> > When flow table is copied, the mask list from the old table > >> > is not properly copied into the new table. The corrupted mask > >> > list in the new table will lead to kernel crash. This patch > >> > fixes this bug. > >> > > >> > Bug #18110 > >> > Reported-by: Justin Pettit > >> > >> Can we just allocate the mask list head when we create v1 of the > >> table? I think that would eliminate a lot of conditions and also allow > >> us to return -ENOMEM up front rather than silent failing later on. > > > > > 0001-remove-mask_list-NULL-check.patch Description: Binary data 0001-datapath-Fix-a-kernel-crash-caused-by-corrupted-mask.patch Description: Binary data ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow masklist fix V2] datapath: Fix a kernel crash caused by corrupted mask list.
Applied, thanks. On Fri, Jun 21, 2013 at 9:07 PM, Andy Zhou wrote: > No. there is no need any more. An incremental patch is attached that > removes all the tbl->mask_list checks. I have also rolled the last 3 patches > in to a single one in case it is useful. > > > On Fri, Jun 21, 2013 at 6:10 PM, Jesse Gross wrote: >> >> Do we need any of the tests for tbl->mask_list at this point if we >> always allocate a list head? >> >> On Fri, Jun 21, 2013 at 5:52 PM, Andy Zhou wrote: >> > O.K. How about the attached incremental patch? >> > >> > >> > On Fri, Jun 21, 2013 at 4:37 PM, Jesse Gross wrote: >> >> >> >> On Fri, Jun 21, 2013 at 4:18 PM, Andy Zhou wrote: >> >> > When flow table is copied, the mask list from the old table >> >> > is not properly copied into the new table. The corrupted mask >> >> > list in the new table will lead to kernel crash. This patch >> >> > fixes this bug. >> >> > >> >> > Bug #18110 >> >> > Reported-by: Justin Pettit >> >> >> >> Can we just allocate the mask list head when we create v1 of the >> >> table? I think that would eliminate a lot of conditions and also allow >> >> us to return -ENOMEM up front rather than silent failing later on. >> > >> > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
