Re: Multi-thread udp 4.7 regression, bisected to 71d8c47fc653

[Marc Dionne]

On Mon, Jul 11, 2016 at 1:26 PM, Pablo Neira Ayuso  wrote:
> On Sun, Jul 10, 2016 at 04:48:26PM -0300, Marc Dionne wrote:
>> An update here since I've had some interactions with Pablo off list.
>>
>> Further testing shows that the underlying cause of the different test
>> results is a udp packet that has a bogus source port number.  In the
>> test the server process tries to send an ack to the bogus port and the
>> flow is disrupted.
>>
>> Notes:
>> - The packet with the bad source port is from a sendmsg() call that
>> has hit the connection tracker clash code introduced by 71d8c47fc653
>> - Packets are successfully sent after the bad one, from the same
>> socket, with the correct source port number
>> - The problem does not reproduce with 71d8c47fc653 reverted, or
>> without nf_conntrack loaded
>> - The bogus port numbers start at 1024, bumping up by 1 every few
>> times the problem occurs (1025, 1026, etc.)
>> - The patch above does not change the behaviour
>> - Enabling lockdep does not show anything
>>
>> Our workaround for the original race was to retry sendmsg() once on
>> EPERM errors, and that had been effective.
>> I can trigger the insertion clash easily with some simple test code,
>> but I have not been able so far to reproduce the packets with bad
>> source port numbers with some simpler code that I could share.
>
> The NAT nul-binding is triggering the source port mangling, even if
> there is no NAT rules in place. The following patch skips the clash
> resolution for NAT by now since we don't see a simple solution for
> this case at the moment.
>
> Could you give a try to this patch in these two cases?
>
> 1) No NAT in place: Make sure iptable_nat module is not there. Or if
>you're using nf_tables, just make sure you have no nat chains at
>all.
>
> 2) With NAT in place, you hit back the EPERM errors that you've
>observed so far.
>
> Please, test both scenarios and report back. Thanks.

Hi Pablo,

Testing out your patch:

1) With no NAT in place, the clash resolution happens, with no side
effects.  No EPERM errors are seen.

2) With ip(6)table_nat loaded, the clash resolution fails and I get
some EPERM errors from sendmsg(), same as before 71d8c47fc653.

Turns out that even though I have no NAT rules in my iptables config,
the system also had firewalld active and that caused the modules to be
loaded.

So the bottom line is that the patch looks good to me..

Thanks,
Marc
--
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 nf-next 3/3] netfilter: replace list_head with single linked list

[Aaron Conole]

Thanks for this;  I will send a v2 in the next two days.

-Aaron

Florian Westphal  writes:

> Aaron Conole  wrote:
>> --- a/net/netfilter/core.c
>> +++ b/net/netfilter/core
> [..]
>> +#define nf_entry_dereference(e) \
>> +rcu_dereference_protected(e, lockdep_is_held(_hook_mutex))
>>  
>> -static struct list_head *nf_find_hook_list(struct net *net,
>> -   const struct nf_hook_ops *reg)
>> +static struct nf_hook_entry *nf_find_hook_list(struct net *net,
>> +   const struct nf_hook_ops *reg)
>>  {
>> -struct list_head *hook_list = NULL;
>> +struct nf_hook_entry *hook_list = NULL;
>>  
>>  if (reg->pf != NFPROTO_NETDEV)
>> -hook_list = >nf.hooks[reg->pf][reg->hooknum];
>> +hook_list = rcu_dereference(net->nf.hooks[reg->pf]
>> +[reg->hooknum]);
>>  else if (reg->hooknum == NF_NETDEV_INGRESS) {
>>  #ifdef CONFIG_NETFILTER_INGRESS
>>  if (reg->dev && dev_net(reg->dev) == net)
>> -hook_list = >dev->nf_hooks_ingress;
>> +hook_list =
>> +rcu_dereference(reg->dev->nf_hooks_ingress);
>
> Both of these should use nf_entry_dereference() to avoid the lockdep
> splat reported by kbuild robot:
>
> net/netfilter/core.c:75 suspicious rcu_dereference_check() usage!
> 2 locks held by swapper/1:
> #0:  (rtnl_mutex){+.+.+.}, at: [] rtnl_lock+0x17/0x20
> #1: (nf_hook_mutex){+.+...}, at: []
> nf_register_net_hook+0xcb/0x240
>

--
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, libnftnl] Fix nftnl_*_get to set data_len

[Pablo Neira Ayuso]

On Mon, Jul 11, 2016 at 06:07:40PM +0200, Carlos Falgueras García wrote:
> All getters must set the output parameter 'data_len'

Applied.

Carlos, I have enhanced this description. Please, include more
detailed justifications on your follow up patches. 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: Multi-thread udp 4.7 regression, bisected to 71d8c47fc653

[Pablo Neira Ayuso]

On Sun, Jul 10, 2016 at 04:48:26PM -0300, Marc Dionne wrote:
> An update here since I've had some interactions with Pablo off list.
> 
> Further testing shows that the underlying cause of the different test
> results is a udp packet that has a bogus source port number.  In the
> test the server process tries to send an ack to the bogus port and the
> flow is disrupted.
> 
> Notes:
> - The packet with the bad source port is from a sendmsg() call that
> has hit the connection tracker clash code introduced by 71d8c47fc653
> - Packets are successfully sent after the bad one, from the same
> socket, with the correct source port number
> - The problem does not reproduce with 71d8c47fc653 reverted, or
> without nf_conntrack loaded
> - The bogus port numbers start at 1024, bumping up by 1 every few
> times the problem occurs (1025, 1026, etc.)
> - The patch above does not change the behaviour
> - Enabling lockdep does not show anything
> 
> Our workaround for the original race was to retry sendmsg() once on
> EPERM errors, and that had been effective.
> I can trigger the insertion clash easily with some simple test code,
> but I have not been able so far to reproduce the packets with bad
> source port numbers with some simpler code that I could share.

The NAT nul-binding is triggering the source port mangling, even if
there is no NAT rules in place. The following patch skips the clash
resolution for NAT by now since we don't see a simple solution for
this case at the moment.

Could you give a try to this patch in these two cases?

1) No NAT in place: Make sure iptable_nat module is not there. Or if
   you're using nf_tables, just make sure you have no nat chains at
   all.

2) With NAT in place, you hit back the EPERM errors that you've
   observed so far.

Please, test both scenarios and report back. Thanks.
>From c3b9dfbcf35ea38a3dce22daf7450fc23c620aea Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso 
Date: Mon, 11 Jul 2016 17:28:54 +0200
Subject: [PATCH] netfilter: conntrack: skip clash resolution if nat is in
 place

The clash resolution is not easy to apply if the NAT table is
registered. Even if no NAT rules are installed, the nul-binding ensures
that a unique tuple is used, thus, the packet that loses race gets a
different source port number, as described by:

http://marc.info/?l=netfilter-devel=146818011604484=2

Clash resolution with NAT is also problematic if addresses/port range
ports are used since the conntrack that wins race may describe a
different mangling that we may have earlier applied to the packet via
nf_nat_setup_info().

Fixes: 71d8c47fc653 ("netfilter: conntrack: introduce clash resolution on insertion race")
Signed-off-by: Pablo Neira Ayuso 
---
 net/netfilter/nf_conntrack_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index e0e9c9a..2b0f80a 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -657,6 +657,7 @@ static int nf_ct_resolve_clash(struct net *net, struct sk_buff *skb,
 
 	l4proto = __nf_ct_l4proto_find(nf_ct_l3num(ct), nf_ct_protonum(ct));
 	if (l4proto->allow_clash &&
+	!nfct_nat(ct) &&
 	!nf_ct_is_dying(ct) &&
 	atomic_inc_not_zero(>ct_general.use)) {
 		nf_ct_acct_merge(ct, ctinfo, (struct nf_conn *)skb->nfct);
-- 
2.1.4

[PATCH v3, libnftnl] Fix nftnl_*_get to set data_len

[Carlos Falgueras García]

All getters must set the output parameter 'data_len'

Signed-off-by: Carlos Falgueras García 
---
 src/chain.c   | 3 +++
 src/expr.c| 1 +
 src/expr/dynset.c | 3 +++
 src/expr/lookup.c | 3 +++
 src/gen.c | 1 +
 src/rule.c| 2 ++
 src/set.c | 2 ++
 src/set_elem.c| 6 ++
 src/table.c   | 1 +
 src/trace.c   | 6 +++---
 10 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/src/chain.c b/src/chain.c
index cab64b5..4c562fe 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -272,8 +272,10 @@ const void *nftnl_chain_get_data(const struct nftnl_chain 
*c, uint16_t attr,
 
switch(attr) {
case NFTNL_CHAIN_NAME:
+   *data_len = strlen(c->name) + 1;
return c->name;
case NFTNL_CHAIN_TABLE:
+   *data_len = strlen(c->table) + 1;
return c->table;
case NFTNL_CHAIN_HOOKNUM:
*data_len = sizeof(uint32_t);
@@ -303,6 +305,7 @@ const void *nftnl_chain_get_data(const struct nftnl_chain 
*c, uint16_t attr,
*data_len = sizeof(uint32_t);
return c->type;
case NFTNL_CHAIN_DEV:
+   *data_len = strlen(c->dev) + 1;
return c->dev;
}
return NULL;
diff --git a/src/expr.c b/src/expr.c
index f802725..e5c1dd3 100644
--- a/src/expr.c
+++ b/src/expr.c
@@ -119,6 +119,7 @@ const void *nftnl_expr_get(const struct nftnl_expr *expr,
 
switch(type) {
case NFTNL_EXPR_NAME:
+   *data_len = strlen(expr->ops->name) + 1;
ret = expr->ops->name;
break;
default:
diff --git a/src/expr/dynset.c b/src/expr/dynset.c
index 0404359..111bf8c 100644
--- a/src/expr/dynset.c
+++ b/src/expr/dynset.c
@@ -88,10 +88,13 @@ nftnl_expr_dynset_get(const struct nftnl_expr *e, uint16_t 
type,
*data_len = sizeof(dynset->timeout);
return >timeout;
case NFTNL_EXPR_DYNSET_SET_NAME:
+   *data_len = strlen(dynset->set_name) + 1;
return dynset->set_name;
case NFTNL_EXPR_DYNSET_SET_ID:
+   *data_len = sizeof(dynset->set_id);
return >set_id;
case NFTNL_EXPR_DYNSET_EXPR:
+   *data_len = 0;
return dynset->expr;
}
return NULL;
diff --git a/src/expr/lookup.c b/src/expr/lookup.c
index 7f68f74..16cfce2 100644
--- a/src/expr/lookup.c
+++ b/src/expr/lookup.c
@@ -73,10 +73,13 @@ nftnl_expr_lookup_get(const struct nftnl_expr *e, uint16_t 
type,
*data_len = sizeof(lookup->dreg);
return >dreg;
case NFTNL_EXPR_LOOKUP_SET:
+   *data_len = strlen(lookup->set_name) + 1;
return lookup->set_name;
case NFTNL_EXPR_LOOKUP_SET_ID:
+   *data_len = sizeof(lookup->set_id);
return >set_id;
case NFTNL_EXPR_LOOKUP_FLAGS:
+   *data_len = sizeof(lookup->flags);
return >flags;
}
return NULL;
diff --git a/src/gen.c b/src/gen.c
index 37a9049..c69d2f8 100644
--- a/src/gen.c
+++ b/src/gen.c
@@ -100,6 +100,7 @@ const void *nftnl_gen_get_data(const struct nftnl_gen *gen, 
uint16_t attr,
 
switch(attr) {
case NFTNL_GEN_ID:
+   *data_len = sizeof(gen->id);
return >id;
}
return NULL;
diff --git a/src/rule.c b/src/rule.c
index 2b23c8e..a0edca7 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -213,8 +213,10 @@ const void *nftnl_rule_get_data(const struct nftnl_rule 
*r, uint16_t attr,
*data_len = sizeof(uint32_t);
return >family;
case NFTNL_RULE_TABLE:
+   *data_len = strlen(r->table) + 1;
return r->table;
case NFTNL_RULE_CHAIN:
+   *data_len = strlen(r->chain) + 1;
return r->chain;
case NFTNL_RULE_HANDLE:
*data_len = sizeof(uint64_t);
diff --git a/src/set.c b/src/set.c
index e48ff78..8a592db 100644
--- a/src/set.c
+++ b/src/set.c
@@ -215,8 +215,10 @@ const void *nftnl_set_get_data(const struct nftnl_set *s, 
uint16_t attr,
 
switch(attr) {
case NFTNL_SET_TABLE:
+   *data_len = strlen(s->table) + 1;
return s->table;
case NFTNL_SET_NAME:
+   *data_len = strlen(s->name) + 1;
return s->name;
case NFTNL_SET_FLAGS:
*data_len = sizeof(uint32_t);
diff --git a/src/set_elem.c b/src/set_elem.c
index 40b5bfe..4e89210 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -160,25 +160,31 @@ const void *nftnl_set_elem_get(struct nftnl_set_elem *s, 
uint16_t attr, uint32_t
 
switch(attr) {
case NFTNL_SET_ELEM_FLAGS:
+   *data_len = sizeof(s->set_elem_flags);
return >set_elem_flags;
case NFTNL_SET_ELEM_KEY:/* NFTA_SET_ELEM_KEY */
*data_len = s->key.len;
  

Re: [PATCH v2, libnftnl] Fix nftnl_*_get to set data_len

[Pablo Neira Ayuso]

On Mon, Jul 11, 2016 at 01:41:07PM +0200, Carlos Falgueras García wrote:
> diff --git a/src/expr/lookup.c b/src/expr/lookup.c
> index 7f68f74..a29b7e5 100644
> --- a/src/expr/lookup.c
> +++ b/src/expr/lookup.c
> @@ -73,10 +73,13 @@ nftnl_expr_lookup_get(const struct nftnl_expr *e, 
> uint16_t type,
>   *data_len = sizeof(lookup->dreg);
>   return >dreg;
>   case NFTNL_EXPR_LOOKUP_SET:
> + *data_len = strlen(lookup->set_name) + 1;
>   return lookup->set_name;
>   case NFTNL_EXPR_LOOKUP_SET_ID:
> + *data_len = sizeof(lookup->set_id) + 1;
>   return >set_id;
>   case NFTNL_EXPR_LOOKUP_FLAGS:
> + *data_len = sizeof(lookup->flags) + 1;

This + 1 here doesn't make any sense.

Please, send v3 without all these + 1.

And as I said in the previous version, you have to return strlen() as is.
--
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 v2, libnftnl] Fix nftnl_*_get to set data_len

[Carlos Falgueras García]

All getters must set the output parameter 'data_len'

Signed-off-by: Carlos Falgueras García 
---
 src/chain.c   | 3 +++
 src/expr.c| 1 +
 src/expr/dynset.c | 3 +++
 src/expr/lookup.c | 3 +++
 src/gen.c | 1 +
 src/rule.c| 2 ++
 src/set.c | 2 ++
 src/set_elem.c| 6 ++
 src/table.c   | 1 +
 9 files changed, 22 insertions(+)

diff --git a/src/chain.c b/src/chain.c
index cab64b5..4c562fe 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -272,8 +272,10 @@ const void *nftnl_chain_get_data(const struct nftnl_chain 
*c, uint16_t attr,
 
switch(attr) {
case NFTNL_CHAIN_NAME:
+   *data_len = strlen(c->name) + 1;
return c->name;
case NFTNL_CHAIN_TABLE:
+   *data_len = strlen(c->table) + 1;
return c->table;
case NFTNL_CHAIN_HOOKNUM:
*data_len = sizeof(uint32_t);
@@ -303,6 +305,7 @@ const void *nftnl_chain_get_data(const struct nftnl_chain 
*c, uint16_t attr,
*data_len = sizeof(uint32_t);
return c->type;
case NFTNL_CHAIN_DEV:
+   *data_len = strlen(c->dev) + 1;
return c->dev;
}
return NULL;
diff --git a/src/expr.c b/src/expr.c
index f802725..e5c1dd3 100644
--- a/src/expr.c
+++ b/src/expr.c
@@ -119,6 +119,7 @@ const void *nftnl_expr_get(const struct nftnl_expr *expr,
 
switch(type) {
case NFTNL_EXPR_NAME:
+   *data_len = strlen(expr->ops->name) + 1;
ret = expr->ops->name;
break;
default:
diff --git a/src/expr/dynset.c b/src/expr/dynset.c
index 0404359..3f6fb08 100644
--- a/src/expr/dynset.c
+++ b/src/expr/dynset.c
@@ -88,10 +88,13 @@ nftnl_expr_dynset_get(const struct nftnl_expr *e, uint16_t 
type,
*data_len = sizeof(dynset->timeout);
return >timeout;
case NFTNL_EXPR_DYNSET_SET_NAME:
+   *data_len = strlen(dynset->set_name) + 1;
return dynset->set_name;
case NFTNL_EXPR_DYNSET_SET_ID:
+   *data_len = sizeof(dynset->set_id) + 1;
return >set_id;
case NFTNL_EXPR_DYNSET_EXPR:
+   *data_len = 0;
return dynset->expr;
}
return NULL;
diff --git a/src/expr/lookup.c b/src/expr/lookup.c
index 7f68f74..a29b7e5 100644
--- a/src/expr/lookup.c
+++ b/src/expr/lookup.c
@@ -73,10 +73,13 @@ nftnl_expr_lookup_get(const struct nftnl_expr *e, uint16_t 
type,
*data_len = sizeof(lookup->dreg);
return >dreg;
case NFTNL_EXPR_LOOKUP_SET:
+   *data_len = strlen(lookup->set_name) + 1;
return lookup->set_name;
case NFTNL_EXPR_LOOKUP_SET_ID:
+   *data_len = sizeof(lookup->set_id) + 1;
return >set_id;
case NFTNL_EXPR_LOOKUP_FLAGS:
+   *data_len = sizeof(lookup->flags) + 1;
return >flags;
}
return NULL;
diff --git a/src/gen.c b/src/gen.c
index 37a9049..ed815e5 100644
--- a/src/gen.c
+++ b/src/gen.c
@@ -100,6 +100,7 @@ const void *nftnl_gen_get_data(const struct nftnl_gen *gen, 
uint16_t attr,
 
switch(attr) {
case NFTNL_GEN_ID:
+   *data_len = sizeof(gen->id) + 1;
return >id;
}
return NULL;
diff --git a/src/rule.c b/src/rule.c
index 2b23c8e..a0edca7 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -213,8 +213,10 @@ const void *nftnl_rule_get_data(const struct nftnl_rule 
*r, uint16_t attr,
*data_len = sizeof(uint32_t);
return >family;
case NFTNL_RULE_TABLE:
+   *data_len = strlen(r->table) + 1;
return r->table;
case NFTNL_RULE_CHAIN:
+   *data_len = strlen(r->chain) + 1;
return r->chain;
case NFTNL_RULE_HANDLE:
*data_len = sizeof(uint64_t);
diff --git a/src/set.c b/src/set.c
index e48ff78..8a592db 100644
--- a/src/set.c
+++ b/src/set.c
@@ -215,8 +215,10 @@ const void *nftnl_set_get_data(const struct nftnl_set *s, 
uint16_t attr,
 
switch(attr) {
case NFTNL_SET_TABLE:
+   *data_len = strlen(s->table) + 1;
return s->table;
case NFTNL_SET_NAME:
+   *data_len = strlen(s->name) + 1;
return s->name;
case NFTNL_SET_FLAGS:
*data_len = sizeof(uint32_t);
diff --git a/src/set_elem.c b/src/set_elem.c
index 40b5bfe..02808aa 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -160,25 +160,31 @@ const void *nftnl_set_elem_get(struct nftnl_set_elem *s, 
uint16_t attr, uint32_t
 
switch(attr) {
case NFTNL_SET_ELEM_FLAGS:
+   *data_len = sizeof(s->set_elem_flags) + 1;
return >set_elem_flags;
case NFTNL_SET_ELEM_KEY:/* NFTA_SET_ELEM_KEY */
*data_len = s->key.len;
return >key.val;

Re: [PATCH 2/2] netfilter: add missing macro

[Pablo Neira Ayuso]

On Fri, Jul 08, 2016 at 05:29:11PM +0100, Eric Engestrom wrote:
> Signed-off-by: Eric Engestrom 
> ---
> 
> This can't compile without this macro… Is this header really used by anyone?
> Should it be removed, to avoid bit-rot?

Probably better to define something like:

#define SCTP_BITMAP_LEN (256 / sizeof (u_int32_t))

and use it consistently all around the code, so we can get rid of
these ARRAY_SIZE() from the uapi header.

> ---
>  include/uapi/linux/netfilter/xt_sctp.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/netfilter/xt_sctp.h 
> b/include/uapi/linux/netfilter/xt_sctp.h
> index 58ffcfb..e4410db 100644
> --- a/include/uapi/linux/netfilter/xt_sctp.h
> +++ b/include/uapi/linux/netfilter/xt_sctp.h
> @@ -3,6 +3,8 @@
>  
>  #include 
>  
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr)[0])
> +
>  #define XT_SCTP_SRC_PORTS0x01
>  #define XT_SCTP_DEST_PORTS   0x02
>  #define XT_SCTP_CHUNK_TYPES  0x04
> -- 
> 2.9.0
> 
--
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] fix off-by-one in DecodeQ931

[Pablo Neira Ayuso]

On Wed, Jul 13, 2016 at 02:59:00PM -0400, Toby DiPasquale wrote:
> fix off-by-one in DecodeQ931
> 
> This patch corrects an off-by-one error in the DecodeQ931 function in
> the nf_conntrack_h323 module. This error could result in reading off
> the end of a Q.931 frame.

Applied to nf-next, thanks.

I have fixed the timestamp since your clock doesn't look right as this
was placed way ahead in the future.
--
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 nf-next 3/3] netfilter: replace list_head with single linked list

[Pablo Neira Ayuso]

On Sat, Jul 09, 2016 at 01:30:38AM +0200, Florian Westphal wrote:
> Aaron Conole  wrote:
> > --- a/net/netfilter/core.c
> > +++ b/net/netfilter/core
> [..]
> > +#define nf_entry_dereference(e) \
> > +   rcu_dereference_protected(e, lockdep_is_held(_hook_mutex))
> >  
> > -static struct list_head *nf_find_hook_list(struct net *net,
> > -  const struct nf_hook_ops *reg)
> > +static struct nf_hook_entry *nf_find_hook_list(struct net *net,
> > +  const struct nf_hook_ops *reg)
> >  {
> > -   struct list_head *hook_list = NULL;
> > +   struct nf_hook_entry *hook_list = NULL;
> >  
> > if (reg->pf != NFPROTO_NETDEV)
> > -   hook_list = >nf.hooks[reg->pf][reg->hooknum];
> > +   hook_list = rcu_dereference(net->nf.hooks[reg->pf]
> > +   [reg->hooknum]);
> > else if (reg->hooknum == NF_NETDEV_INGRESS) {
> >  #ifdef CONFIG_NETFILTER_INGRESS
> > if (reg->dev && dev_net(reg->dev) == net)
> > -   hook_list = >dev->nf_hooks_ingress;
> > +   hook_list =
> > +   rcu_dereference(reg->dev->nf_hooks_ingress);
> 
> Both of these should use nf_entry_dereference() to avoid the lockdep
> splat reported by kbuild robot:
> 
> net/netfilter/core.c:75 suspicious rcu_dereference_check() usage!
> 2 locks held by swapper/1:
> #0:  (rtnl_mutex){+.+.+.}, at: [] rtnl_lock+0x17/0x20
> #1:  (nf_hook_mutex){+.+...}, at: [] 
> nf_register_net_hook+0xcb/0x240

Aaron, please, send a v2.

I have a patchset that changes the footprint of the hook function as
it was discussed during the last Netfilter Workshop that clashes with
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

Re: [PATCH 2/2 libnfntl] Fix nftnl_*_get to set data_len

[Pablo Neira Ayuso]

On Mon, Jul 11, 2016 at 12:24:27PM +0200, Pablo Neira Ayuso wrote:
> Carlos,
> 
> Habla con Laura para ver cómo lleva este cambio en la reunión:
> 
> http://patchwork.ozlabs.org/patch/639253/
> 
> Si ella no anda con tiempo, creo que tú tienes los conocimientos para
> hacer este cambio que describo ahí.
> 
> No lo olvides. Gracias.

Sorry, I accidentally sent this email to the mailing list in Spanish.
--
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 2/2 libnfntl] Fix nftnl_*_get to set data_len

[Pablo Neira Ayuso]

Carlos,

Habla con Laura para ver cómo lleva este cambio en la reunión:

http://patchwork.ozlabs.org/patch/639253/

Si ella no anda con tiempo, creo que tú tienes los conocimientos para
hacer este cambio que describo ahí.

No lo olvides. Gracias.
--
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: [GIT PULL nf-next] IPVS Updates for v4.8

[Pablo Neira Ayuso]

On Thu, Jul 07, 2016 at 08:40:39PM +0200, Simon Horman wrote:
> Hi Pablo,
> 
> please consider these enhancements to the IPVS. This alters the behaviour
> of the "least connection" schedulers such that pre-established connections
> are included in the active connection count. This avoids overloading
> servers when a large number of new connections arrive in a short space of
> time - e.g. when clients reconnect after a node or network failure.
> 
> 
> The following changes since commit c6ac37d8d8843fb1fdc34e4a2a41a4f027ab670c:
> 
>   netfilter: nf_log: fix error on write NONE to logger choice sysctl 
> (2016-07-05 14:57:57 +0200)
> 
> are available in the git repository at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs-next.git 
> tags/ipvs-for-v4.8

Pulled, thanks Simon.
--
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: [GIT PULL nf] Second Round of IPVS Fixes for v4.7

[Pablo Neira Ayuso]

On Thu, Jul 07, 2016 at 08:30:21PM +0200, Simon Horman wrote:
> Hi Pablo,
> 
> please consider this IPVS fix for v4.7.
> 
> The fix from Quentin Armitage allows the backup sync daemon to
> be bound to a link-local mcast IPv6 address as is already the case
> for IPv4.
> 
> The following changes since commit 62131e5d735226074cba53095545d76b491e5003:
> 
>   netfilter: nft_meta: set skb->nf_trace appropriately (2016-06-23 14:15:33 
> +0200)
> 
> are available in the git repository at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git 
> tags/ipvs-fixes2-for-v4.7

Also pulled, 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 -next] netfilter: conntrack: simplify early_drop

[Pablo Neira Ayuso]

On Sun, Jul 03, 2016 at 08:44:01PM +0200, Florian Westphal wrote:
> We don't need to acquire the bucket lock during early drop, we can
> use lockless traveral just like nf_conntrack_find.
> 
> The timer deletion serves as synchronization point, if another cpu
> attempts to evict same entry, only one will succeed with timer deletion.

Applied, thanks Florian.
--
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 nf-next 1/2] netfilter: move nat hlist_head to nf_conn

[Pablo Neira Ayuso]

On Tue, Jul 05, 2016 at 12:07:23PM +0200, Florian Westphal wrote:
> The nat extension structure is 32bytes in size on x86_64:
> 
> struct nf_conn_nat {
> struct hlist_node  bysource; /* 016 */
> struct nf_conn *   ct;   /*16 8 */
> union nf_conntrack_nat_help help;/*24 4 */
> intmasq_index;   /*28 4 */
> /* size: 32, cachelines: 1, members: 4 */
> /* last cacheline: 32 bytes */
> };
> 
> The hlist is needed to quickly check for possible tuple collisions
> when installing a new nat binding. Storing this in the extension
> area has two drawbacks:
> 
> 1. We need ct backpointer to get the conntrack struct from the extension.
> 2. When reallocation of extension area occurs we need to fixup the bysource
>hash head via hlist_replace_rcu.
> 
> We can avoid both by placing the hlist_head in nf_conn and place nf_conn in
> the bysource hash rather than the extenstion.
> 
> We can also remove the ->move support; no other extension needs it.
> 
> Moving the entire nat extension into nf_conn would be possible as well but
> then we have to add yet another callback for deletion from the bysource
> hash table rather than just using nat extension ->destroy hook for this.
> 
> nf_conn size doesn't increase due to aligment, followup patch replaces
> hlist_node with single pointer.

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 nf-next] netfilter: nft_ct: make byte/packet expr more friendly

[Pablo Neira Ayuso]

On Tue, Jul 05, 2016 at 11:23:00PM +0800, Liping Zhang wrote:
> From: Liping Zhang 
> 
> If we want to use ct packets expr, and add a rule like follows:
>   # nft add rule filter input ct packets gt 1 counter
> 
> We will find that no packets will hit it, because
> nf_conntrack_acct is disabled by default. So It will
> not work until we enable it manually via
> "echo 1 > /proc/sys/net/netfilter/nf_conntrack_acct".
> 
> This is not friendly, so like xt_connbytes do, if the user
> want to use ct byte/packet expr, enable nf_conntrack_acct
> automatically.

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 V2,nf 0/3] netfilter: conntrack: fix race condition associated with hash resize

[Pablo Neira Ayuso]

On Sun, Jul 03, 2016 at 01:18:42PM +0800, Liping Zhang wrote:
> From: Liping Zhang 
> 
> When user adjust the hash size via 
> /sys/module/nf_conntrack/parameters/hashsize,
> something will break because race condition happened.
> 
> This patch set aim to fix these bugs.
> 
> When we do "cat /proc/net/nf_conntrack", and at the same time do hash resize,
> nf_conntrack_htable_size and nf_conntrack_hash may become unrelated if we
> read them separately, so oops will happen. Fix this in patch #1.
> 
> When we do unlink help or timeout objects, and at the same time do hash 
> resize,
> we may miss unlinking some objects, later we will end up with invalid 
> references.
> Fix this in patch #2 and #3.

Series applied to nf-next. We're already a bit late in the rc cycle
and this has been broken since the beginning, so I'm inclined to
follow the nf-next path.
--
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 nf-next 2/6] netfilter: nat: convert nat bysrc hash to rhashtable

[Pablo Neira Ayuso]

On Tue, Jul 05, 2016 at 12:07:24PM +0200, Florian Westphal wrote:
> It did use a fixed-size bucket list plus single lock to protect add/del.
> 
> Unlike the main conntrack table we only need to add and remove keys.
> Convert it to rhashtable to get table autosizing and per-bucket locking.
> 
> The maximum number of entries is -- as before -- tied to the number of
> conntracks so we do not need another upperlimit.
> 
> The change does not handle rhashtable_remove_fast error, only possible
> "error" is -ENOENT, and that is something that can happen legitimetely,
> e.g. because nat module was inserted at a later time and no src manip
> took place yet.
> 
> Tested with http-client-benchmark + httpterm with DNAT and SNAT rules
> in place.

Applied, thanks.

I'm fixing this minor glitch here.

  CC [M]  net/netfilter/nf_nat_core.o
net/netfilter/nf_nat_core.c: In function ‘nf_nat_proto_clean’:
net/netfilter/nf_nat_core.c:555:6: warning: unused variable ‘err’
[-Wunused-variable]
  int err;
  ^

No need to resend.
--
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