| Message ID | 1556920600-47298-2-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/4] datapath: compat: Backport nf_conntrack_timeout support | expand |
Bleep bloop. Greetings Yi-Hung Wei, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Author Dan Carpenter <dan.carpenter@oracle.com> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Yi-Hung Wei <yihung.wei@gmail.com>
Lines checked: 158, Warnings: 1, Errors: 1
build:
/bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dummy.lo -MD -MP -MF $depbase.Tpo -c -o lib/dummy.lo lib/dummy.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dummy.lo -MD -MP -MF lib/.deps/dummy.Tpo -c lib/dummy.c -o lib/dummy.o
depbase=`echo lib/dpctl.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpctl.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpctl.lo lib/dpctl.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpctl.lo -MD -MP -MF lib/.deps/dpctl.Tpo -c lib/dpctl.c -o lib/dpctl.o
depbase=`echo lib/dp-packet.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dp-packet.lo -MD -MP -MF $depbase.Tpo -c -o lib/dp-packet.lo lib/dp-packet.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dp-packet.lo -MD -MP -MF lib/.deps/dp-packet.Tpo -c lib/dp-packet.c -o lib/dp-packet.o
depbase=`echo lib/dpif-netdev.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netdev.lo lib/dpif-netdev.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF lib/.deps/dpif-netdev.Tpo -c lib/dpif-netdev.c -o lib/dpif-netdev.o
lib/dpif-netdev.c: In function 'dp_execute_cb':
lib/dpif-netdev.c:7112:13: error: enumeration value 'OVS_CT_ATTR_TIMEOUT' not handled in switch [-Werror=switch]
switch(sub_type) {
^
cc1: all warnings being treated as errors
make[2]: *** [lib/dpif-netdev.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2
Please check this out. If you feel there has been an error, please email aconole@bytheb.org
Thanks,
0-day Robot
0-day Robot <robot@bytheb.org> writes: > Bleep bloop. Greetings Yi-Hung Wei, I am a robot and I have tried out your patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > checkpatch: > ERROR: Author Dan Carpenter <dan.carpenter@oracle.com> needs to sign off. > WARNING: Unexpected sign-offs from developers who are not authors or > co-authors or committers: Yi-Hung Wei <yihung.wei@gmail.com> > Lines checked: 158, Warnings: 1, Errors: 1 > > > build: > /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 > -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib > -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith > -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter > -Wbad-function-cast -Wcast-align -Wstrict-prototypes > -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dummy.lo -MD -MP -MF $depbase.Tpo -c -o > lib/dummy.lo lib/dummy.c &&\ > mv -f $depbase.Tpo $depbase.Plo > libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I > ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra > -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security > -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dummy.lo -MD -MP -MF lib/.deps/dummy.Tpo -c > lib/dummy.c -o lib/dummy.o > depbase=`echo lib/dpctl.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ > /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 > -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib > -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith > -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter > -Wbad-function-cast -Wcast-align -Wstrict-prototypes > -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dpctl.lo -MD -MP -MF $depbase.Tpo -c -o > lib/dpctl.lo lib/dpctl.c &&\ > mv -f $depbase.Tpo $depbase.Plo > libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I > ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra > -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security > -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dpctl.lo -MD -MP -MF lib/.deps/dpctl.Tpo -c > lib/dpctl.c -o lib/dpctl.o > depbase=`echo lib/dp-packet.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ > /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 > -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib > -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith > -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter > -Wbad-function-cast -Wcast-align -Wstrict-prototypes > -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dp-packet.lo -MD -MP -MF $depbase.Tpo -c -o > lib/dp-packet.lo lib/dp-packet.c &&\ > mv -f $depbase.Tpo $depbase.Plo > libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I > ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra > -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security > -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dp-packet.lo -MD -MP -MF > lib/.deps/dp-packet.Tpo -c lib/dp-packet.c -o lib/dp-packet.o > depbase=`echo lib/dpif-netdev.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\ > /bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 > -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib > -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith > -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter > -Wbad-function-cast -Wcast-align -Wstrict-prototypes > -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF $depbase.Tpo -c -o > lib/dpif-netdev.lo lib/dpif-netdev.c &&\ > mv -f $depbase.Tpo $depbase.Plo > libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I > ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra > -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security > -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes > -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror > -Werror -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF > lib/.deps/dpif-netdev.Tpo -c lib/dpif-netdev.c -o lib/dpif-netdev.o > lib/dpif-netdev.c: In function 'dp_execute_cb': > lib/dpif-netdev.c:7112:13: error: enumeration value > 'OVS_CT_ATTR_TIMEOUT' not handled in switch [-Werror=switch] > switch(sub_type) { > ^ > cc1: all warnings being treated as errors Are you taking a look at this? It doesn't look to be related to the setup (but I haven't looked at the code or commit): https://travis-ci.org/ovsrobot/ovs/jobs/528009507 > make[2]: *** [lib/dpif-netdev.lo] Error 1 > make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace' > make[1]: *** [all-recursive] Error 1 > make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace' > make: *** [all] Error 2 > > > Please check this out. If you feel there has been an error, please email aconole@bytheb.org > > Thanks, > 0-day Robot > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, May 6, 2019 at 1:02 PM Aaron Conole <aconole@redhat.com> wrote: > > > lib/.deps/dpif-netdev.Tpo -c lib/dpif-netdev.c -o lib/dpif-netdev.o > > lib/dpif-netdev.c: In function 'dp_execute_cb': > > lib/dpif-netdev.c:7112:13: error: enumeration value > > 'OVS_CT_ATTR_TIMEOUT' not handled in switch [-Werror=switch] > > switch(sub_type) { > > ^ > > cc1: all warnings being treated as errors > > Are you taking a look at this? It doesn't look to be related to the > setup (but I haven't looked at the code or commit): > > https://travis-ci.org/ovsrobot/ovs/jobs/528009507 Thanks Aaron for bringing this up. It is because this patch introduced a new symbol OVS_CT_ATTR_TIMEOUT, but it is not handled in lib/dpif-netdev.c with this patch. Actually, OVS_CT_ATTR_TIMEOUT will be taken care of in the 3rd patch of this series. When the whole series is applied, the travis build will pass. https://travis-ci.org/YiHungWei/ovs/builds/527968147 Anyway, it should not break the build when this patch is applied. I will squash patch 2 and 3 together in the next revision. Thanks, -Yi-Hung
diff --git a/datapath/conntrack.c b/datapath/conntrack.c index 52208bad3029..d9287288df63 100644 --- a/datapath/conntrack.c +++ b/datapath/conntrack.c @@ -29,6 +29,7 @@ #include <net/netfilter/nf_conntrack_helper.h> #include <net/netfilter/nf_conntrack_labels.h> #include <net/netfilter/nf_conntrack_seqadj.h> +#include <net/netfilter/nf_conntrack_timeout.h> #include <net/netfilter/nf_conntrack_zones.h> #include <net/netfilter/ipv6/nf_defrag_ipv6.h> @@ -85,6 +86,7 @@ struct ovs_conntrack_info { u32 eventmask; /* Mask of 1 << IPCT_*. */ struct md_mark mark; struct md_labels labels; + char timeout[CTNL_TIMEOUT_NAME_MAX]; #ifdef CONFIG_NF_NAT_NEEDED struct nf_nat_range2 range; /* Only present for SRC NAT and DST NAT. */ #endif @@ -1525,6 +1527,8 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = { #endif [OVS_CT_ATTR_EVENTMASK] = { .minlen = sizeof(u32), .maxlen = sizeof(u32) }, + [OVS_CT_ATTR_TIMEOUT] = { .minlen = 1, + .maxlen = CTNL_TIMEOUT_NAME_MAX }, }; static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info, @@ -1610,6 +1614,15 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info, info->have_eventmask = true; info->eventmask = nla_get_u32(a); break; +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT + case OVS_CT_ATTR_TIMEOUT: + memcpy(info->timeout, nla_data(a), nla_len(a)); + if (!memchr(info->timeout, '\0', nla_len(a))) { + OVS_NLERR(log, "Invalid conntrack helper"); + return -EINVAL; + } + break; +#endif default: OVS_NLERR(log, "Unknown conntrack attr (%d)", @@ -1691,6 +1704,14 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, OVS_NLERR(log, "Failed to allocate conntrack template"); return -ENOMEM; } + + if (ct_info.timeout[0]) { + if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto, + ct_info.timeout)) + pr_info_ratelimited("Failed to associated timeout " + "policy `%s'\n", ct_info.timeout); + } + if (helper) { err = ovs_ct_add_helper(&ct_info, helper, key, log); if (err) @@ -1815,6 +1836,10 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info, if (ct_info->have_eventmask && nla_put_u32(skb, OVS_CT_ATTR_EVENTMASK, ct_info->eventmask)) return -EMSGSIZE; + if (ct_info->timeout[0]) { + if (nla_put_string(skb, OVS_CT_ATTR_TIMEOUT, ct_info->timeout)) + return -EMSGSIZE; + } #ifdef CONFIG_NF_NAT_NEEDED if (ct_info->nat && !ovs_ct_nat_to_attr(ct_info, skb)) @@ -1836,8 +1861,11 @@ static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info) { if (ct_info->helper) nf_conntrack_helper_put(ct_info->helper); - if (ct_info->ct) + if (ct_info->ct) { + if (ct_info->timeout[0]) + nf_ct_destroy_timeout(ct_info->ct); nf_ct_tmpl_free(ct_info->ct); + } } #if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h index 65a003a62cf5..7b16b1d5bfe0 100644 --- a/datapath/linux/compat/include/linux/openvswitch.h +++ b/datapath/linux/compat/include/linux/openvswitch.h @@ -801,6 +801,7 @@ struct ovs_action_push_tnl { * be received on NFNLGRP_CONNTRACK_NEW and NFNLGRP_CONNTRACK_DESTROY groups, * respectively. Remaining bits control the changes for which an event is * delivered on the NFNLGRP_CONNTRACK_UPDATE group. + * @OVS_CT_ATTR_TIMEOUT: Variable length string defining conntrack timeout. */ enum ovs_ct_attr { OVS_CT_ATTR_UNSPEC, @@ -813,6 +814,9 @@ enum ovs_ct_attr { OVS_CT_ATTR_NAT, /* Nested OVS_NAT_ATTR_* */ OVS_CT_ATTR_FORCE_COMMIT, /* No argument */ OVS_CT_ATTR_EVENTMASK, /* u32 mask of IPCT_* events. */ + OVS_CT_ATTR_TIMEOUT, /* Associate timeout with this connection for + * fine-grain timeout tuning. */ + __OVS_CT_ATTR_MAX };
Upstream commit: commit 06bd2bdf19d2f3d22731625e1a47fa1dff5ac407 Author: Yi-Hung Wei <yihung.wei@gmail.com> Date: Tue Mar 26 11:31:14 2019 -0700 openvswitch: Add timeout support to ct action Add support for fine-grain timeout support to conntrack action. The new OVS_CT_ATTR_TIMEOUT attribute of the conntrack action specifies a timeout to be associated with this connection. If no timeout is specified, it acts as is, that is the default timeout for the connection will be automatically applied. Example usage: $ nfct timeout add timeout_1 inet tcp syn_sent 100 established 200 $ ovs-ofctl add-flow br0 in_port=1,ip,tcp,action=ct(commit,timeout=timeout_1) CC: Pravin Shelar <pshelar@ovn.org> CC: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> Acked-by: Pravin B Shelar <pshelar@ovn.org> Signed-off-by: David S. Miller <davem@davemloft.net> commit 6d670497e01803b486aa72cc1a718401ab986896 Author: Dan Carpenter <dan.carpenter@oracle.com> Date: Tue Apr 2 09:53:14 2019 +0300 openvswitch: use after free in __ovs_ct_free_action() We free "ct_info->ct" and then use it on the next line when we pass it to nf_ct_destroy_timeout(). This patch swaps the order to avoid the use after free. Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Acked-by: Yi-Hung Wei <yihung.wei@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- datapath/conntrack.c | 30 ++++++++++++++++++++++- datapath/linux/compat/include/linux/openvswitch.h | 4 +++ 2 files changed, 33 insertions(+), 1 deletion(-)