Re: [PATCH nft] meta: permit meta nfproto ip in ip family

2017-06-06 Thread Florian Westphal 
Florian Westphal  wrote:
> works:
> add rule ip filter input ct original saddr 1.2.3.4
> (family ctx init initialises network base to proto_ip).
> 
> fails to parse 1.2.3.4 address:
> add rule ip filter input meta nfproto ipv4 ct original saddr 1.2.3.4
> 
> ... because meta_expr_pctx_update() won't find a dependency
> from "ip" to "ip" and then overwrites the correct base with proto_unknown.
> 
> "meta nfproto ip" is useless in the ip family, as it will always match,
> i.e.  a better (but more compliated) fix would be to remove the statement
> during evaluation.
> 
> Signed-off-by: Florian Westphal 
> ---
>  src/meta.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/meta.c b/src/meta.c
> index cb7c1368e087..dd09d49560cd 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -497,6 +497,7 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
>   const struct hook_proto_desc *h = _proto_desc[ctx->family];
>   const struct expr *left = expr->left, *right = expr->right;
>   const struct proto_desc *desc;
> + uint8_t protonum;
>  
>   assert(expr->op == OP_EQ);
>  
> @@ -514,10 +515,16 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
>   proto_ctx_update(ctx, PROTO_BASE_LL_HDR, >location, desc);
>   break;
>   case NFT_META_NFPROTO:
> - desc = proto_find_upper(h->desc, mpz_get_uint8(right->value));
> - if (desc == NULL)
> + protonum = mpz_get_uint8(right->value);
> + desc = proto_find_upper(h->desc, protonum);
> + if (desc == NULL) {
>   desc = _unknown;
>  
> + if (protonum == ctx->family &&
> + h->base == PROTO_BASE_NETWORK_HDR)
> + desc = h->desc;
> + }
> +

I've pushed this.

One thing to keep in mind for future:

NFT_META_NFPROTO is basically useless except for inet pseudo-family.

For bridge it will be NFPROTO_BRIDGE, so if we ever get bridge
conntrack support we will need some other dependency instead to
set the needed context info (perhaps based off ct entry itself...)

Perhaps we should extend eval step to kill this dependency, or reject
statements when they occur in wrong context.

Examples:

add rule ip filter input meta nfproto ipv4  // always true
add rule ip filter input meta nfproto != ipv4  // always false
add rule ip filter input meta nfproto ipv6  // always false
add rule bridge filter input meta nfproto ipv6  // always false


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [conntrack-tools PATCH 4/4] conntrackd: deprecate unix backlog configuration

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 12:58:44PM +0200, Arturo Borrero Gonzalez wrote:
> This configuration option doesn't add any value to users.
> Use the magic value of 100 (i.e, the socket will keep 100 pending 
> connections),
> which I think is fair enough for what conntrackd can do in the unix socket.

And this one applied, thanks.

So you only have to sort out patch 3/4.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [conntrack-tools PATCH 3/4] conntrackd: cleanup if failed forking

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 12:58:38PM +0200, Arturo Borrero Gonzalez wrote:
> Close the logs and lockfile if error while forking.

Also applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [conntrack-tools PATCH 1/4] conntrackd: evaluate configuration earlier

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 12:58:27PM +0200, Arturo Borrero Gonzalez wrote:
> Run the evaluation step sooner in the conntrackd startup routine.
> Don't close log or unlink lockfile at this stage.

Applied, thanks Arturo!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nft] ct: fix inet/bridge/netdev family handling for saddr/daddr

2017-06-06 Thread Pablo Neira Ayuso 
On Mon, May 29, 2017 at 07:25:38PM +0200, Florian Westphal wrote:
> "ct orignal saddr" has an invalid data type, as the address can be either 
> ipv4 or ipv6.
> For some cases we could infer it from the rhs, but there are cases where we 
> don't have any
> information, e.g. when passing ct original saddr to jhash expression.
> 
> So do the same thing that we do for "rt nexthop" -- error out and hint to user
> they need to specifiy the desired address type with "meta nfproto".
> 
> Suggested-by: Pablo Neira Ayuso 
> Signed-off-by: Florian Westphal 

Acked-by: Pablo Neira Ayuso 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nft] meta: permit meta nfproto ip in ip family

2017-06-06 Thread Pablo Neira Ayuso 
On Mon, May 29, 2017 at 07:25:37PM +0200, Florian Westphal wrote:
> works:
> add rule ip filter input ct original saddr 1.2.3.4
> (family ctx init initialises network base to proto_ip).
> 
> fails to parse 1.2.3.4 address:
> add rule ip filter input meta nfproto ipv4 ct original saddr 1.2.3.4
> 
> ... because meta_expr_pctx_update() won't find a dependency
> from "ip" to "ip" and then overwrites the correct base with proto_unknown.
> 
> "meta nfproto ip" is useless in the ip family, as it will always match,
> i.e.  a better (but more compliated) fix would be to remove the statement
> during evaluation.
> 
> Signed-off-by: Florian Westphal 

Acked-by: Pablo Neira Ayuso 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] src: Remove expire information from list stateless ruleset.

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 11:55:40AM +0530, Varsha Rao wrote:
> As expires is stateful information. This patch removes expire
> information from list stateless ruleset. With nft -s option, the
> ruleset will be as following.
> 
> table ip firewall {
> set host {
> type ipv4_addr
> flags timeout
> elements = { 10.0.0.2 timeout 10m }
> }
> }

Applied, thanks Varsha.

Congrats, your first patch to nftables is in.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] scanner: add support for include directories

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 05:58:24PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 06, 2017 at 02:50:09PM +0300, Ismo Puustinen wrote:
> > If a string after "include" keyword points to a directory instead of a
> > file, consider the directory to contain only nft rule files and try to
> > load them all. This helps with a use case where services drop their own
> > firewall configuration files into a directory and nft needs to include
> > those without knowing the exact file names.
> > 
> > File loading order from the include directory is not specified, so the
> > files inside an include directory should not depend on each other.
> > 
> > Fixes(Bug 1154 - Allow include statement to operate on directories and/or 
> > wildcards).
> 
> Applied, thanks Ismo.
> 
> BTW, it would be great if you can send us a patch for the nft(8)
> manpage too to document this new feature.
> 
> I can also create an account for your at wiki.nftables.org, in case
> you want to add there more examples. There are people even slightly
> describing their usecase and the way they are solving it. These
> patterns will be good for people that are looking into reusing good
> ideas when using nftables.

BTW, one more question: I still have a minor concern on the
assumptions that people can made on the order that files are included
in this feature.

Please, correct me if wrong, my understanding is at this stage there
is not guarantees in what order they would be loaded at all.

So I'm thinking that it would be good if we provide some good
default, ie. load them in alphanumeric order or similar.

That would be a follow up patch, but it would be good to sort out this
before nft 0.8 gets released.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] tests: test include directories

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 02:50:10PM +0300, Ismo Puustinen wrote:
> Add tests for:
>   * including an empty directory
>   * including directory with one or two files in it
>   * testing for required trailing slash in directory name
>   * testing for detecting non-existent directory
>   * testing for a broken file in included directory

Also applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] scanner: add support for include directories

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 02:50:09PM +0300, Ismo Puustinen wrote:
> If a string after "include" keyword points to a directory instead of a
> file, consider the directory to contain only nft rule files and try to
> load them all. This helps with a use case where services drop their own
> firewall configuration files into a directory and nft needs to include
> those without knowing the exact file names.
> 
> File loading order from the include directory is not specified, so the
> files inside an include directory should not depend on each other.
> 
> Fixes(Bug 1154 - Allow include statement to operate on directories and/or 
> wildcards).

Applied, thanks Ismo.

BTW, it would be great if you can send us a patch for the nft(8)
manpage too to document this new feature.

I can also create an account for your at wiki.nftables.org, in case
you want to add there more examples. There are people even slightly
describing their usecase and the way they are solving it. These
patterns will be good for people that are looking into reusing good
ideas when using nftables.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH iptables 2/3] libip6t_icmp6: xlate: remove leftover space

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 12:08:27AM +0200, Pablo M. Bermudo Garay wrote:
> This change should have been included in commit f035be35c749
> ("xtables-translate: fix multiple spaces issue"), but was forgotten.

Also applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH iptables 3/3] xtables-translate: fix double space before comment

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 12:08:28AM +0200, Pablo M. Bermudo Garay wrote:
> When a comment translation immediately follows a counter statement, two
> spaces are printed between "counter" and "comment" keywords.
> 
> The counter statement is almost always followed by a target, so we need
> to move the space following "counter" to the beginning of the target
> translation.

Neat, applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH iptables 1/3] tests: xlate: generalize owner

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 12:08:26AM +0200, Pablo M. Bermudo Garay wrote:
> The owner name was hard-coded in the owner extension translation test.
> The translation process requires the user to exist in the system, so
> this commit replaces it with the usual UID_MIN value (1000).

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH conntrack-tools v2] ipv6: remove use of HAVE_INET_PTON_IPV6

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, May 30, 2017 at 09:56:26AM +0200, Nicolas Dichtel wrote:
> The goal of this patch is to fix the ipv6 support when conntrackd is
> cross-compiled. The AC_RUN_IFELSE macro must be avoided as much as possible.
> See section 6.6 of the gnu autoconf:
> "If you really need to test for a runtime behavior while configuring, you can
>  write a test program to determine the result, and compile and run it using
>  AC_RUN_IFELSE. Avoid running test programs if possible, because this prevents
>  people from configuring your package for cross-compiling."
> 
> Let's remove this check and test the returned error to handle the case where
> ipv6 is not supported (inet_pton() returns -1 when the family is not 
> supported).

Applied, thanks Nicolas.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] scanner: add support for include directories

2017-06-06 Thread Ismo Puustinen 
If a string after "include" keyword points to a directory instead of a
file, consider the directory to contain only nft rule files and try to
load them all. This helps with a use case where services drop their own
firewall configuration files into a directory and nft needs to include
those without knowing the exact file names.

File loading order from the include directory is not specified, so the
files inside an include directory should not depend on each other.

Fixes(Bug 1154 - Allow include statement to operate on directories and/or 
wildcards).

Signed-off-by: Ismo Puustinen 
---
 src/scanner.l | 135 +++---
 1 file changed, 110 insertions(+), 25 deletions(-)

diff --git a/src/scanner.l b/src/scanner.l
index c2c008d..1beadb1 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -10,6 +10,8 @@
 
 %{
 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -637,8 +639,8 @@ static struct error_record *scanner_push_file(void 
*scanner, const char *filenam
return NULL;
 }
 
-int scanner_read_file(void *scanner, const char *filename,
- const struct location *loc)
+static int include_file(void *scanner, const char *filename,
+   const struct location *loc)
 {
struct parser_state *state = yyget_extra(scanner);
struct error_record *erec;
@@ -655,12 +657,92 @@ int scanner_read_file(void *scanner, const char *filename,
if (erec != NULL)
goto err;
return 0;
+err:
+   erec_queue(erec, state->msgs);
+   return -1;
+}
 
+int scanner_read_file(void *scanner, const char *filename,
+ const struct location *loc)
+{
+   return include_file(scanner, filename, loc);
+}
+
+static int include_directory(void *scanner, const char *dirname,
+DIR *directory, const struct location *loc)
+{
+   struct parser_state *state = yyget_extra(scanner);
+   struct error_record *erec;
+   struct dirent *de;
+   FILE *f;
+   int ret;
+
+   if (!dirname[0] || dirname[strlen(dirname)-1] != '/') {
+   erec = error(loc, "Include directory name \"%s\" does not end 
in '/'",
+   dirname);
+   goto err;
+   }
+
+   /* If the path is a directory, assume that all files there need
+* to be included.
+*/
+   while ((de = readdir(directory))) {
+   char dirbuf[PATH_MAX];
+
+   ret = snprintf(dirbuf, sizeof(dirbuf), "%s/%s", dirname, 
de->d_name);
+   if (ret < 0 || ret >= PATH_MAX) {
+   erec = error(loc, "Too long file path \"%s/%s\"\n",
+   dirname, de->d_name);
+   goto err;
+   }
+
+   if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 
0)
+   continue;
+
+   f = fopen(dirbuf, "r");
+   if (f == NULL) {
+   erec = error(loc, "Could not open file \"%s\": %s\n",
+   dirbuf, strerror(errno));
+   goto err;
+   }
+
+   erec = scanner_push_file(scanner, de->d_name, f, loc);
+   if (erec != NULL)
+   goto err;
+   }
+   return 0;
 err:
erec_queue(erec, state->msgs);
return -1;
 }
 
+static int include_dentry(void *scanner, const char *filename,
+ const struct location *loc)
+{
+   DIR *directory;
+   int ret;
+
+   /* The file can be either a simple file or a directory which
+* contains files.
+*/
+
+   directory = opendir(filename);
+
+   if (directory == NULL && errno != ENOTDIR)
+   /* Could not access the directory or file, keep on searching.
+* Return value '1' indicates to the caller that we should still
+* search in the next include directory.
+*/
+   ret = 1;
+   else if (directory != NULL) {
+   ret = include_directory(scanner, filename, directory, loc);
+   closedir(directory);
+   } else
+   ret = include_file(scanner, filename, loc);
+
+   return ret;
+}
+
 static bool search_in_include_path(const char *filename)
 {
return (strncmp(filename, "./", strlen("./")) != 0 &&
@@ -671,40 +753,43 @@ static bool search_in_include_path(const char *filename)
 int scanner_include_file(void *scanner, const char *filename,
 const struct location *loc)
 {
-   struct parser_state *state = yyget_extra(scanner);
-   struct error_record *erec;
char buf[PATH_MAX];
-   const char *name = buf;
unsigned int i;
-   FILE *f;
+   int ret;
+   struct error_record *erec;
+   struct parser_state *state = yyget_extra(scanner);
 
-   f = NULL;

[PATCH v3 2/2] tests: test include directories

2017-06-06 Thread Ismo Puustinen 
Add tests for:
  * including an empty directory
  * including directory with one or two files in it
  * testing for required trailing slash in directory name
  * testing for detecting non-existent directory
  * testing for a broken file in included directory

Signed-off-by: Ismo Puustinen 
---
 tests/shell/testcases/include/0005dir_empty_0  | 29 +
 tests/shell/testcases/include/0006dir_single_0 | 36 +
 tests/shell/testcases/include/0007dir_double_0 | 45 +
 tests/shell/testcases/include/0008dir_no_slash_1   | 29 +
 tests/shell/testcases/include/0009dir_nodir_1  | 31 ++
 .../shell/testcases/include/0010dir_broken_file_1  | 47 ++
 6 files changed, 217 insertions(+)
 create mode 100755 tests/shell/testcases/include/0005dir_empty_0
 create mode 100755 tests/shell/testcases/include/0006dir_single_0
 create mode 100755 tests/shell/testcases/include/0007dir_double_0
 create mode 100755 tests/shell/testcases/include/0008dir_no_slash_1
 create mode 100755 tests/shell/testcases/include/0009dir_nodir_1
 create mode 100755 tests/shell/testcases/include/0010dir_broken_file_1

diff --git a/tests/shell/testcases/include/0005dir_empty_0 
b/tests/shell/testcases/include/0005dir_empty_0
new file mode 100755
index 000..f16acf8
--- /dev/null
+++ b/tests/shell/testcases/include/0005dir_empty_0
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+echo "Failed to create tmp directory" >&2
+exit 0
+fi
+
+tmpfile1=$(mktemp)
+if [ ! -w $tmpfile1 ] ; then
+echo "Failed to create tmp file" >&2
+exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 && rmdir $tmpdir" EXIT
+
+RULESET1="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+
+$NFT -f $tmpfile1
+
+if [ $? -ne 0 ] ; then
+echo "E: unable to load good ruleset" >&2
+exit 1
+fi
diff --git a/tests/shell/testcases/include/0006dir_single_0 
b/tests/shell/testcases/include/0006dir_single_0
new file mode 100755
index 000..ae4fd5f
--- /dev/null
+++ b/tests/shell/testcases/include/0006dir_single_0
@@ -0,0 +1,36 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+echo "Failed to create tmp directory" >&2
+exit 0
+fi
+
+tmpfile1=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile1 ] ; then
+echo "Failed to create tmp file" >&2
+exit 0
+fi
+
+tmpfile2=$(mktemp)
+if [ ! -w $tmpfile2 ] ; then
+echo "Failed to create tmp file" >&2
+exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 $tmpfile2 && rmdir $tmpdir" EXIT
+
+RULESET1="add table x"
+RULESET2="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+echo "$RULESET2" > $tmpfile2
+
+$NFT -f $tmpfile2
+if [ $? -ne 0 ] ; then
+echo "E: unable to load good ruleset" >&2
+exit 1
+fi
diff --git a/tests/shell/testcases/include/0007dir_double_0 
b/tests/shell/testcases/include/0007dir_double_0
new file mode 100755
index 000..0a14ade
--- /dev/null
+++ b/tests/shell/testcases/include/0007dir_double_0
@@ -0,0 +1,45 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+echo "Failed to create tmp directory" >&2
+exit 0
+fi
+
+tmpfile1=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile1 ] ; then
+echo "Failed to create tmp file" >&2
+exit 0
+fi
+
+tmpfile2=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile2 ] ; then
+echo "Failed to create tmp file" >&2
+exit 0
+fi
+
+tmpfile3=$(mktemp)
+if [ ! -w $tmpfile3 ] ; then
+echo "Failed to create tmp file" >&2
+exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 && rmdir $tmpdir" EXIT
+
+RULESET1="add table x"
+RULESET2="add table y"
+RULESET3="include \"$tmpdir/\""
+
+echo "$RULESET1" > $tmpfile1
+echo "$RULESET2" > $tmpfile2
+echo "$RULESET3" > $tmpfile3
+
+$NFT -f $tmpfile3
+
+if [ $? -ne 0 ] ; then
+echo "E: unable to load good ruleset" >&2
+exit 1
+fi
diff --git a/tests/shell/testcases/include/0008dir_no_slash_1 
b/tests/shell/testcases/include/0008dir_no_slash_1
new file mode 100755
index 000..2820dc9
--- /dev/null
+++ b/tests/shell/testcases/include/0008dir_no_slash_1
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+set -e
+
+tmpdir=$(mktemp -d)
+if [ ! -d $tmpdir ] ; then
+echo "Failed to create tmp directory" >&2
+exit 0
+fi
+
+tmpfile1=$(mktemp -p $tmpdir)
+if [ ! -w $tmpfile1 ] ; then
+echo "Failed to create tmp file" >&2
+exit 0
+fi
+
+# cleanup if aborted
+trap "rm -rf $tmpfile1 && rmdir $tmpdir" EXIT
+
+RULESET1="include \"$tmpdir\""
+
+echo "$RULESET1" > $tmpfile1
+
+$NFT -f $tmpfile1
+
+if [ $? -eq 0 ] ; then
+echo "E: did not catch missing slash in directory name" >&2
+exit 1
+fi
diff --git a/tests/shell/testcases/include/0009dir_nodir_1 
b/tests/shell/testcases/include/0009dir_nodir_1
new file mode 100755
index

Re: [nf-next PATCH] netfilter: nf_tables: Report transactions' process info to user space

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, May 30, 2017 at 06:21:49PM +0200, Phil Sutter wrote:
> On Tue, May 30, 2017 at 02:12:11PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, May 19, 2017 at 12:41:28PM +0200, Phil Sutter wrote:
> > What I mean is that that we should use the heading netlink message as
> > netlink context (portid, flags) for the NEWGEN message. This is
> > currently broken. I'll send a patch to fix this, so you can send a
> > follow up of this on top of this.
> 
> OK, thanks!

Actually the existing code for NEWGEN looks good. We pass the original
skbuff to ->commit(), so we can fetch from the _BEGIN netlink message
coming in first place in the batch the context we need.

> > > | @@ -4538,7 +4539,9 @@ static int nf_tables_fill_gen_info(struct sk_buff 
> > > *skb, struct net *net,
> > > | nfmsg->version  = NFNETLINK_V0;
> > > | nfmsg->res_id   = htons(net->nft.base_seq & 0x);
> > > |  
> > > | -   if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq)))
> > > | +   if (nla_put_be32(skb, NFTA_GEN_ID, htonl(net->nft.base_seq)) ||
> > > | +   nla_put_be32(skb, NFTA_GEN_PROC_PID, 
> > > htonl(task_pid_nr(current))) ||
> > > | +   nla_put_string(skb, NFTA_GEN_PROC_NAME, get_task_comm(buf, 
> > > current)))
> > > | goto nla_put_failure;

So you only need to add these missing to the NEWGEN message.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [conntrack-tools PATCH 4/4] conntrackd: deprecate unix backlog configuration

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 01:11:53PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 06, 2017 at 12:58:44PM +0200, Arturo Borrero Gonzalez wrote:
> > This configuration option doesn't add any value to users.
> > Use the magic value of 100 (i.e, the socket will keep 100 pending 
> > connections),
> > which I think is fair enough for what conntrackd can do in the unix socket.
> 
> I don't think conntrackd will ever get more than 100 connection that
> are pending to be accepted.

And this only refers to unix socket indeed, really we can deprecate
this.

Back to what I said for Nice/Scheduler, I'm not so sure about removing
them.  Actually I remember this was useful when I was testing long
time ago.

Basically what I observed is that RT scheduler + process pinning to
spare CPU makes Netlink reliable (no event message loss). And that is
good to have in place under high load, otherwise nodes get out of
sync.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [conntrack-tools PATCH 4/4] conntrackd: deprecate unix backlog configuration

2017-06-06 Thread Pablo Neira Ayuso 
On Tue, Jun 06, 2017 at 12:58:44PM +0200, Arturo Borrero Gonzalez wrote:
> This configuration option doesn't add any value to users.
> Use the magic value of 100 (i.e, the socket will keep 100 pending 
> connections),
> which I think is fair enough for what conntrackd can do in the unix socket.

I don't think conntrackd will ever get more than 100 connection that
are pending to be accepted.

So I'm fine with this 4/4 patch.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [conntrack-tools PATCH 2/4] conntrackd: make the daemon run in RT mode by default

2017-06-06 Thread Pablo Neira Ayuso 
Hi Arturo,

On Tue, Jun 06, 2017 at 12:58:32PM +0200, Arturo Borrero Gonzalez wrote:
> In order to prevent netlink buffer overrun, conntrackd is recommended to run
> at max priority.
> Make conntrackd to use a RT (SHED_RR) scheduler by default at max priority.
> This is common among other HA daemons. For example corosync uses SCHED_RR
> by default.
> This change should help ease the configuration of conntrackd.
> 
> Note that a sched priority that high makes the nice value useless, so 
> deprecate
> both options now.
> 
> The code is moved to the init() routine. In case of error setting the
> scheduler, the system default will be used. Report a message to the user
> and continue working.

I think we should provide a good default if someone doesn't specify
anything. So defaulting to RT is fine to me so we converge to what
other HA software is doing.

But I think we should keep the Nice and Scheduler clauses. Just in
case anyone wants to do this fine grain tunning.

So my proposal is:

1) Remove them from the examples configuration files.
2) Keep these toggles documented in manpage.
3) Provide this default if someone doesn't specify anything.

So the idea is that we provide good defaults.

BTW, an option I would really deprecate is the Checksum, a lot of
experimentation was going on at the time I added this (more than 10
years ago), this should really go away since I don't see a usecase for
this.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[conntrack-tools PATCH 1/4] conntrackd: evaluate configuration earlier

2017-06-06 Thread Arturo Borrero Gonzalez 
Run the evaluation step sooner in the conntrackd startup routine.
Don't close log or unlink lockfile at this stage.

Signed-off-by: Arturo Borrero Gonzalez 
---
 src/main.c |   20 +---
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/main.c b/src/main.c
index fb20f1d..4b6d17d 100644
--- a/src/main.c
+++ b/src/main.c
@@ -338,6 +338,15 @@ int main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
 
+   /*
+* Evaluate configuration
+*/
+   if (evaluate() == -1) {
+   dlog(LOG_ERR, "conntrackd cannot start, please review your "
+"configuration");
+   exit(EXIT_FAILURE);
+   }
+
if (type == REQUEST) {
if (do_local_request(action, , local_step) == -1) {
dlog(LOG_ERR, "can't connect: is conntrackd "
@@ -383,17 +392,6 @@ int main(int argc, char *argv[])
}
 
/*
-* Evaluate configuration
-*/
-   if (evaluate() == -1) {
-   dlog(LOG_ERR, "conntrackd cannot start, please review your "
-"configuration");
-   close_log();
-   unlink(CONFIG(lockfile));
-   exit(EXIT_FAILURE);
-   }
-
-   /*
 * initialization process
 */
 

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[conntrack-tools PATCH 2/4] conntrackd: make the daemon run in RT mode by default

2017-06-06 Thread Arturo Borrero Gonzalez 
In order to prevent netlink buffer overrun, conntrackd is recommended to run
at max priority.
Make conntrackd to use a RT (SHED_RR) scheduler by default at max priority.
This is common among other HA daemons. For example corosync uses SCHED_RR
by default.
This change should help ease the configuration of conntrackd.

Note that a sched priority that high makes the nice value useless, so deprecate
both options now.

The code is moved to the init() routine. In case of error setting the
scheduler, the system default will be used. Report a message to the user
and continue working.

Signed-off-by: Arturo Borrero Gonzalez 
---
 conntrackd.conf.5|   46 +++---
 doc/helper/conntrackd.conf   |   21 -
 doc/stats/conntrackd.conf|   19 
 doc/sync/alarm/conntrackd.conf   |   21 -
 doc/sync/ftfw/conntrackd.conf|   21 -
 doc/sync/notrack/conntrackd.conf |   21 -
 include/conntrackd.h |5 
 src/main.c   |   28 ---
 src/read_config_yy.y |   21 +
 src/run.c|   18 +++
 10 files changed, 28 insertions(+), 193 deletions(-)

diff --git a/conntrackd.conf.5 b/conntrackd.conf.5
index 94de327..1e56a1f 100644
--- a/conntrackd.conf.5
+++ b/conntrackd.conf.5
@@ -480,14 +480,8 @@ By default runtime support is disabled.
 
 .TP
 .BI "Nice "
-Set the \fBnice(1)\fP value of the daemon, this value goes from -20 (most
-favorable scheduling) to 19 (least favorable). Using a very low value reduces
-the chances to lose state-change events.
-
-Example: Nice -20
-
-Default is 0 but this example sets it to most favourable scheduling as
-this is generally a good idea.
+Deprecated. This option will be removed in the future.
+Conntrackd now uses by default a RT scheduler.
 
 .TP
 .BI "HashSize "
@@ -731,29 +725,8 @@ Example:
 .fi
 
 .SS SCHEDULER
-Select a different scheduler for the daemon, you can select between \fBRR\fP
-and \fBFIFO\fP and the process priority.
-
-See \fBsched_setscheduler(2)\fP for more information. Using a RT scheduler
-reduces the chances to overrun the Netlink buffer.
-
-Example:
-.nf
-   Scheduler {
-   Type FIFO
-   Priority 99
-   }
-.fi
-
-.TP
-.BI "Type "
-Supported values are \fBRR\fP or \fBFIFO\fP.
-
-.TP
-.BI "Priority "
-Value of the scheduler priority.
-
-Minimum is 0, maximum is 99.
+Deprecated. This section will be removed in the future.
+Conntrackd now uses by default a RT scheduler.
 
 .SH STATS
 This top-level section indicates \fBconntrackd(8)\fP to work as a statistic
@@ -907,7 +880,6 @@ Stats {
 }
 General {
Systemd on
-   Nice -1
HashSize 8192
HashLimit 65535
Syslog on
@@ -973,11 +945,6 @@ Sync {
 }
 General {
Systemd on
-   Nice -20
-   Scheduler {
-   Type FIFO
-   Priority 99
-   }
HashSize 32768
HashLimit 131072
LogFile on
@@ -1036,11 +1003,6 @@ Sync {
 }
 General {
Systemd on
-   Nice -20
-   Scheduler {
-   Type FIFO
-   Priority 99
-   }
HashSize 32768
HashLimit 131072
LogFile on
diff --git a/doc/helper/conntrackd.conf b/doc/helper/conntrackd.conf
index 7eae8bc..abc4087 100644
--- a/doc/helper/conntrackd.conf
+++ b/doc/helper/conntrackd.conf
@@ -103,27 +103,6 @@ Helper {
 #
 General {
#
-   # Set the nice value of the daemon, this value goes from -20
-   # (most favorable scheduling) to 19 (least favorable). Using a
-   # very low value reduces the chances to lose state-change events.
-   # Default is 0 but this example file sets it to most favourable
-   # scheduling as this is generally a good idea. See man nice(1) for
-   # more information.
-   #
-   Nice -20
-
-   #
-   # Select a different scheduler for the daemon, you can select between
-   # RR and FIFO and the process priority (minimum is 0, maximum is 99).
-   # See man sched_setscheduler(2) for more information. Using a RT
-   # scheduler reduces the chances to overrun the Netlink buffer.
-   #
-   # Scheduler {
-   #   Type FIFO
-   #   Priority 99
-   # }
-
-   #
# Logfile: on (/var/log/conntrackd.log), off, or a filename
# Default: off
#
diff --git a/doc/stats/conntrackd.conf b/doc/stats/conntrackd.conf
index 6a9aec8..e62ad4b 100644
--- a/doc/stats/conntrackd.conf
+++ b/doc/stats/conntrackd.conf
@@ -11,25 +11,6 @@ General {
#Systemd on
 
#
-   # Set the nice value of the daemon. This value goes from -20
-   # (most favorable scheduling) to 19 (least favorable). Using a
-   # negative value reduces the chances to lose state-change events.
-   # Default is 0. See man nice(1) for more information.
-   #
-

[conntrack-tools PATCH 4/4] conntrackd: deprecate unix backlog configuration

2017-06-06 Thread Arturo Borrero Gonzalez 
This configuration option doesn't add any value to users.
Use the magic value of 100 (i.e, the socket will keep 100 pending connections),
which I think is fair enough for what conntrackd can do in the unix socket.

Signed-off-by: Arturo Borrero Gonzalez 
---
 conntrackd.conf.5|8 +---
 doc/helper/conntrackd.conf   |1 -
 doc/stats/conntrackd.conf|1 -
 doc/sync/alarm/conntrackd.conf   |1 -
 doc/sync/ftfw/conntrackd.conf|1 -
 doc/sync/notrack/conntrackd.conf |1 -
 include/local.h  |1 -
 src/local.c  |4 +++-
 src/read_config_yy.y |2 +-
 9 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/conntrackd.conf.5 b/conntrackd.conf.5
index 1e56a1f..4785f47 100644
--- a/conntrackd.conf.5
+++ b/conntrackd.conf.5
@@ -603,7 +603,6 @@ Example:
 .nf
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
 .fi
 
@@ -615,9 +614,7 @@ Example: Path /var/run/conntrackd.ctl
 
 .TP
 .BI "Backlog "
-Number of items in the backlog.
-
-Example: Backlog 20
+Deprecated option.
 
 .SS FILTER
 Event filtering. This clause allows you to filter certain traffic.
@@ -886,7 +883,6 @@ General {
LockFile /var/lock/conntrack.lock
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
NetlinkBufferSize 262142
NetlinkBufferSizeMaxGrowth 655355
@@ -952,7 +948,6 @@ General {
LockFile /var/lock/conntrack.lock
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
NetlinkBufferSize 2097152
NetlinkBufferSizeMaxGrowth 8388608
@@ -1010,7 +1005,6 @@ General {
LockFile /var/lock/conntrack.lock
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
NetlinkBufferSize 2097152
NetlinkBufferSizeMaxGrowth 8388608
diff --git a/doc/helper/conntrackd.conf b/doc/helper/conntrackd.conf
index abc4087..4148544 100644
--- a/doc/helper/conntrackd.conf
+++ b/doc/helper/conntrackd.conf
@@ -124,6 +124,5 @@ General {
#
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
 }
diff --git a/doc/stats/conntrackd.conf b/doc/stats/conntrackd.conf
index e62ad4b..ba957a1 100644
--- a/doc/stats/conntrackd.conf
+++ b/doc/stats/conntrackd.conf
@@ -43,7 +43,6 @@ General {
#
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
 
#
diff --git a/doc/sync/alarm/conntrackd.conf b/doc/sync/alarm/conntrackd.conf
index f609310..831be15 100644
--- a/doc/sync/alarm/conntrackd.conf
+++ b/doc/sync/alarm/conntrackd.conf
@@ -262,7 +262,6 @@ General {
#
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
 
#
diff --git a/doc/sync/ftfw/conntrackd.conf b/doc/sync/ftfw/conntrackd.conf
index f500637..9da0fb6 100644
--- a/doc/sync/ftfw/conntrackd.conf
+++ b/doc/sync/ftfw/conntrackd.conf
@@ -285,7 +285,6 @@ General {
#
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
 
#
diff --git a/doc/sync/notrack/conntrackd.conf b/doc/sync/notrack/conntrackd.conf
index 718668d..600fc89 100644
--- a/doc/sync/notrack/conntrackd.conf
+++ b/doc/sync/notrack/conntrackd.conf
@@ -324,7 +324,6 @@ General {
#
UNIX {
Path /var/run/conntrackd.ctl
-   Backlog 20
}
 
#
diff --git a/include/local.h b/include/local.h
index f9121b1..22859d7 100644
--- a/include/local.h
+++ b/include/local.h
@@ -6,7 +6,6 @@
 #endif
 
 struct local_conf {
-   int backlog;
int reuseaddr;
char path[UNIX_PATH_MAX];
 };
diff --git a/src/local.c b/src/local.c
index 3395b4c..2b67885 100644
--- a/src/local.c
+++ b/src/local.c
@@ -26,6 +26,8 @@
 #include 
 #include 
 
+#define UNIX_SOCKET_BACKLOG 100
+
 int local_server_create(struct local_server *server, struct local_conf *conf)
 {
int fd;
@@ -53,7 +55,7 @@ int local_server_create(struct local_server *server, struct 
local_conf *conf)
return -1;
}
 
-   if (listen(fd, conf->backlog) == -1) {
+   if (listen(fd, UNIX_SOCKET_BACKLOG) == -1) {
close(fd);
unlink(conf->path);
return -1;
diff --git a/src/read_config_yy.y b/src/read_config_yy.y
index ef6b284..5dca1f6 100644
--- a/src/read_config_yy.y
+++ b/src/read_config_yy.y
@@ -650,7 +650,7 @@ unix_option : T_PATH T_PATH_VAL
 
 unix_option : T_BACKLOG T_NUMBER
 {
-   conf.local.backlog = $2;
+   dlog(LOG_WARNING, "deprecated unix backlog configuration.");
 };
 
 sync: T_SYNC '{' sync_list '}'

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at

[conntrack-tools PATCH 3/4] conntrackd: cleanup if failed forking

2017-06-06 Thread Arturo Borrero Gonzalez 
Close the logs and lockfile if error while forking.

Signed-off-by: Arturo Borrero Gonzalez 
---
 src/main.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/main.c b/src/main.c
index bab7772..3b19160 100644
--- a/src/main.c
+++ b/src/main.c
@@ -386,6 +386,8 @@ int main(int argc, char *argv[])
 
if ((pid = fork()) == -1) {
dlog(LOG_ERR, "fork has failed: %s", strerror(errno));
+   close_log();
+   unlink(CONFIG(lockfile));
exit(EXIT_FAILURE);
} else if (pid) {
sd_ct_mainpid(pid);

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] src: Remove expire information from list stateless ruleset.

2017-06-06 Thread Varsha Rao 
As expires is stateful information. This patch removes expire
information from list stateless ruleset. With nft -s option, the
ruleset will be as following.

table ip firewall {
set host {
type ipv4_addr
flags timeout
elements = { 10.0.0.2 timeout 10m }
}
}

Signed-off-by: Varsha Rao 
---
 src/expression.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/expression.c b/src/expression.c
index 55dd391..4fef830 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -962,7 +962,7 @@ static void set_elem_expr_print(const struct expr *expr)
printf(" timeout ");
time_print(expr->timeout / 1000);
}
-   if (expr->expiration) {
+   if (!stateless_output && expr->expiration) {
printf(" expires ");
time_print(expr->expiration / 1000);
}
-- 
2.9.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html