date:20170412

Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Liping Zhang

Hi Feng,

2017-04-13 11:22 GMT+08:00 Gao Feng :
[...]
>> No, it's better to do this together, there are two invocations, it's not 
>> good to
>> copy these codes twice.
>
> You mean " on &= ~ IPS_UNCHANGEABLE_MASK " and " off &= ~ 
> IPS_UNCHANGEABLE_MASK " seems duplicated?

I see. I misunderstood your initial meaning. So I will send V2 :)
--
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] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Gao Feng

Hi Liping,

> -Original Message-
> From: Liping Zhang [mailto:zlpnob...@gmail.com]
> Sent: Thursday, April 13, 2017 11:15 AM
> To: Gao Feng 
> Cc: Liping Zhang ; Pablo Neira Ayuso
> ; Netfilter Developer Mailing List
> ; cerne...@chromium.org
> Subject: Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> ct->status
> 
> Hi Feng,
> 
> 2017-04-13 10:42 GMT+08:00 Gao Feng :
> [...]
> >> +static void
> >> +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
> >> +   unsigned long off) {
> >> + unsigned long mask;
> >> + unsigned int bit;
> >> +
> >> + for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
> >> + mask = 1 << bit;
> >> + /* Ignore these unchangable bits */
> >> + if (mask & IPS_UNCHANGEABLE_MASK)
> >> + continue;
> >
> > How about clear the bits of on and off with IPS_UNCHANGEABLE_MASK
> > before loop.
> > Like "on &= ~ IPS_UNCHANGEABLE_MASK";
> > Then the "if (mask & IPS_UNCHANGEABLE_MASK)" could be removed.
> 
> No, it's better to do this together, there are two invocations, it's not good 
> to
> copy these codes twice.

You mean " on &= ~ IPS_UNCHANGEABLE_MASK " and " off &= ~ IPS_UNCHANGEABLE_MASK 
" seems duplicated?

> 
> >
> > BTW, when some bits are set both of on and off, the "on" would be
> > effective, but "off" not.
> 
> This won't happen, see the invocation:
> 1. __ctnetlink_change_status(ct, status, 0); 2. __ctnetlink_change_status(ct,
> status, ~status);
> 
> > So I think we could use BUILD_BUG_ON to avoid it during building.
> > BUILD_BUG_ON(on);
> 
> Btw, this won't help, BUILD_BUG_ON is only effective on compile time, but
> "on" and "off" will be modified at the running time.
You are right.
This new function would be used frequently at running time.

Regards
Feng



--
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] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Liping Zhang

Hi Feng,

2017-04-13 10:42 GMT+08:00 Gao Feng :
[...]
>> +static void
>> +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
>> +   unsigned long off)
>> +{
>> + unsigned long mask;
>> + unsigned int bit;
>> +
>> + for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
>> + mask = 1 << bit;
>> + /* Ignore these unchangable bits */
>> + if (mask & IPS_UNCHANGEABLE_MASK)
>> + continue;
>
> How about clear the bits of on and off with IPS_UNCHANGEABLE_MASK before
> loop.
> Like "on &= ~ IPS_UNCHANGEABLE_MASK";
> Then the "if (mask & IPS_UNCHANGEABLE_MASK)" could be removed.

No, it's better to do this together, there are two invocations, it's not good to
copy these codes twice.

>
> BTW, when some bits are set both of on and off, the "on" would be effective,
> but "off" not.

This won't happen, see the invocation:
1. __ctnetlink_change_status(ct, status, 0);
2. __ctnetlink_change_status(ct, status, ~status);

> So I think we could use BUILD_BUG_ON to avoid it during building.
> BUILD_BUG_ON(on);

Btw, this won't help, BUILD_BUG_ON is only effective on compile time,
but "on" and "off" will be modified at the running time.
--
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] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Gao Feng

> -Original Message-
> From: Gao Feng [mailto:gfree.w...@foxmail.com]
> Sent: Thursday, April 13, 2017 10:42 AM
> To: 'Liping Zhang' ; 'pa...@netfilter.org'
> 
> Cc: 'netfilter-devel@vger.kernel.org' ;
> 'cerne...@chromium.org' ; 'Liping Zhang'
> 
> Subject: RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> ct->status
> 
> Hi Liping,
> 
> > -Original Message-
> > From: netfilter-devel-ow...@vger.kernel.org
> > [mailto:netfilter-devel-ow...@vger.kernel.org] On Behalf Of Liping
> > Zhang
> > Sent: Wednesday, April 12, 2017 11:57 PM
> > To: pa...@netfilter.org
> > Cc: netfilter-devel@vger.kernel.org; cerne...@chromium.org; Liping
> > Zhang 
> > Subject: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> > ct->status
> >
> > From: Liping Zhang 
> >
> > User can update the ct->status via nfnetlink, but using a non-atomic
> > operation "ct->status |= status;". This is unsafe, and may clear
> > IPS_DYING_BIT bit set by another CPU unexpectedly. For example:
> >  CPU0CPU1
> >   ctnetlink_change_status__nf_conntrack_find_get
> >   old = ct->status  nf_ct_gc_expired
> >   - nf_ct_kill
> >   -  test_and_set_bit(IPS_DYING_BIT
> >   - -
> >   ct->status = old | status;<-- oops, _DYING_ is cleared!
> >
> > So using a series of atomic bit operation to solve the above issue.
> >
> > Also note, user shouldn't set IPS_TEMPLATE, IPS_SEQ_ADJUST directly.
> >
> > If we set the IPS_TEMPLATE_BIT, ct will be freed by nf_ct_tmpl_free,
> > but actually it is alloced by nf_conntrack_alloc.
> >
> > If we set the IPS_SEQ_ADJUST_BIT, this may cause the NULL pointer
> > deference, as the nfct_seqadj(ct) maybe NULL.
> >
> > So make these two bits be unchangable too.
> >
> > Last, add some comments to describe the logic change due to the commit
> > a963d710f367 ("netfilter: ctnetlink: Fix regression in CTA_STATUS
> > processing"), which makes me feel a little confusing.
> >
> > Signed-off-by: Liping Zhang 
> > ---
> >  include/uapi/linux/netfilter/nf_conntrack_common.h | 13 +---
> >  net/netfilter/nf_conntrack_netlink.c   | 35
> > --
> >  2 files changed, 35 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h
> > b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > index 6a8e33d..38fc383 100644
> > --- a/include/uapi/linux/netfilter/nf_conntrack_common.h
> > +++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > @@ -82,10 +82,6 @@ enum ip_conntrack_status {
> > IPS_DYING_BIT = 9,
> > IPS_DYING = (1 << IPS_DYING_BIT),
> >
> > -   /* Bits that cannot be altered from userland. */
> > -   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> > -IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING),
> > -
> > /* Connection has fixed timeout. */
> > IPS_FIXED_TIMEOUT_BIT = 10,
> > IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT), @@ -101,6
> > +97,15 @@ enum ip_conntrack_status {
> > /* Conntrack got a helper explicitly attached via CT target. */
> > IPS_HELPER_BIT = 13,
> > IPS_HELPER = (1 << IPS_HELPER_BIT),
> > +
> > +   /* Be careful here, modifying these bits can make things messy,
> > +* so don't let users modify them directly.
> > +*/
> > +   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> > +IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING |
> > +IPS_SEQ_ADJUST | IPS_TEMPLATE),
> > +
> > +   __IPS_MAX_BIT = 14,
> >  };
> >
> >  /* Connection tracking event types */ diff --git
> > a/net/netfilter/nf_conntrack_netlink.c
> > b/net/netfilter/nf_conntrack_netlink.c
> > index 908d858..68efe1a 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -1419,6 +1419,26 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,
> > } #endif
> >
> > +static void
> > +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
> > + unsigned long off)
> > +{
> > +   unsigned long mask;
> > +   unsigned int bit;
> > +
> > +   for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
> > +   mask = 1 << bit;
> > +   /* Ignore these unchangable bits */
> > +   if (mask & IPS_UNCHANGEABLE_MASK)
> > +   continue;
> 
> How about clear the bits of on and off with IPS_UNCHANGEABLE_MASK before
> loop.
> Like "on &= ~ IPS_UNCHANGEABLE_MASK";
> Then the "if (mask & IPS_UNCHANGEABLE_MASK)" could be removed.
> 
> BTW, when some bits are set both of on and off, the "on" would be
effective,
> but "off" not.
> So I think we could use BUILD_BUG_ON to avoid it during building.
>

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Gao Feng

Hi Liping,

> -Original Message-
> From: netfilter-devel-ow...@vger.kernel.org
> [mailto:netfilter-devel-ow...@vger.kernel.org] On Behalf Of Liping Zhang
> Sent: Wednesday, April 12, 2017 11:57 PM
> To: pa...@netfilter.org
> Cc: netfilter-devel@vger.kernel.org; cerne...@chromium.org; Liping Zhang
> 
> Subject: [PATCH nf] netfilter: ctnetlink: make it safer when updating
ct->status
> 
> From: Liping Zhang 
> 
> User can update the ct->status via nfnetlink, but using a non-atomic
operation
> "ct->status |= status;". This is unsafe, and may clear IPS_DYING_BIT bit
set by
> another CPU unexpectedly. For example:
>  CPU0CPU1
>   ctnetlink_change_status__nf_conntrack_find_get
>   old = ct->status  nf_ct_gc_expired
>   - nf_ct_kill
>   -  test_and_set_bit(IPS_DYING_BIT
>   - -
>   ct->status = old | status;<-- oops, _DYING_ is cleared!
> 
> So using a series of atomic bit operation to solve the above issue.
> 
> Also note, user shouldn't set IPS_TEMPLATE, IPS_SEQ_ADJUST directly.
> 
> If we set the IPS_TEMPLATE_BIT, ct will be freed by nf_ct_tmpl_free, but
> actually it is alloced by nf_conntrack_alloc.
> 
> If we set the IPS_SEQ_ADJUST_BIT, this may cause the NULL pointer
deference,
> as the nfct_seqadj(ct) maybe NULL.
> 
> So make these two bits be unchangable too.
> 
> Last, add some comments to describe the logic change due to the commit
> a963d710f367 ("netfilter: ctnetlink: Fix regression in CTA_STATUS
processing"),
> which makes me feel a little confusing.
> 
> Signed-off-by: Liping Zhang 
> ---
>  include/uapi/linux/netfilter/nf_conntrack_common.h | 13 +---
>  net/netfilter/nf_conntrack_netlink.c   | 35
> --
>  2 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h
> b/include/uapi/linux/netfilter/nf_conntrack_common.h
> index 6a8e33d..38fc383 100644
> --- a/include/uapi/linux/netfilter/nf_conntrack_common.h
> +++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
> @@ -82,10 +82,6 @@ enum ip_conntrack_status {
>   IPS_DYING_BIT = 9,
>   IPS_DYING = (1 << IPS_DYING_BIT),
> 
> - /* Bits that cannot be altered from userland. */
> - IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> -  IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING),
> -
>   /* Connection has fixed timeout. */
>   IPS_FIXED_TIMEOUT_BIT = 10,
>   IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT), @@ -101,6
> +97,15 @@ enum ip_conntrack_status {
>   /* Conntrack got a helper explicitly attached via CT target. */
>   IPS_HELPER_BIT = 13,
>   IPS_HELPER = (1 << IPS_HELPER_BIT),
> +
> + /* Be careful here, modifying these bits can make things messy,
> +  * so don't let users modify them directly.
> +  */
> + IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> +  IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING |
> +  IPS_SEQ_ADJUST | IPS_TEMPLATE),
> +
> + __IPS_MAX_BIT = 14,
>  };
> 
>  /* Connection tracking event types */
> diff --git a/net/netfilter/nf_conntrack_netlink.c
> b/net/netfilter/nf_conntrack_netlink.c
> index 908d858..68efe1a 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1419,6 +1419,26 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,  }
> #endif
> 
> +static void
> +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
> +   unsigned long off)
> +{
> + unsigned long mask;
> + unsigned int bit;
> +
> + for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
> + mask = 1 << bit;
> + /* Ignore these unchangable bits */
> + if (mask & IPS_UNCHANGEABLE_MASK)
> + continue;

How about clear the bits of on and off with IPS_UNCHANGEABLE_MASK before
loop.
Like "on &= ~ IPS_UNCHANGEABLE_MASK";
Then the "if (mask & IPS_UNCHANGEABLE_MASK)" could be removed.

BTW, when some bits are set both of on and off, the "on" would be effective,
but "off" not.
So I think we could use BUILD_BUG_ON to avoid it during building.
BUILD_BUG_ON(on);

Best Regards
Feng

> +
> + if (on & mask)
> + set_bit(bit, >status);
> + else if (off & mask)
> + clear_bit(bit, >status);
> + }
> +}
> +
>  static int
>  ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const
cda[])
> { @@ -1438,10 +1458,7 @@ ctnetlink_change_status(struct nf_conn *ct, const
> struct nlattr * const cda[])
>   /* ASSURED bit can only be set */
>   return -EBUSY;
> 
> - /* Be careful here, modifying NAT bits can screw up things,
> -  * so don't let users modify them

Re: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

2017-04-12 Thread Laura García Liébana

On Wed, Apr 12, 2017 at 10:43 PM, Florian Westphal  wrote:
> Liping Zhang  wrote:
>> >> +++ b/net/netfilter/nft_hash.c
>> >> @@ -21,6 +21,7 @@ struct nft_hash {
>> >> enum nft_registers  sreg:8;
>> >> enum nft_registers  dreg:8;
>> >> u8  len;
>> >> +   boolautogen_seed:1;
>> >
>> > Hi Liping, I don't think that hiding the seed value would be useful, and
>> > even adding this attribute doesn't worth it just to hide the seed.
>> >
>>
>> If we don't do this thing, if the user inputting the following nft rules:
>>   # nft add rule x y ct mark set jhash ip saddr mod 2
>>
>> Then nft list ruleset will display something like this, where 0xd6ab633c
>> is very unpredictable, and the user doesn't care the seed at all:
>>   ct mark set jhash ip saddr mod 2 seed 0xd6ab633c
>>
>> This will cause annoying complain when running "nft-test.py ip/hash.t".
>>
>> But another problem is this, I remember that Pablo is implementing a new
>> delete rule syntax, something like this:
>> "nft del rule x y ct mark set jhash ip saddr mod 2"
>>
>> The unpredictable seed will cause the above rule failed, since the seed
>> is not the same, so we cannot find a matched nft rule.
>
> FWIW I agree with Liping, we should eat the extra bool and
> supress seed dump.
>
> I also think most users should not ever have to even know of seed arg
> existence.
>
> In fact is there a use case where it is needed?
> We might want to ditch it completely and always just
> generate it privately in kernel (symhash f.e. uses
> a private seed) already.

In the case that is always generated in the kernel side, then we can
discard the seed parameter in nft and there is no need for an
additional bool.

Another option could be to relax the check of the seed attribute in
the tests and in the rules deletion.
--
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] ip_vs_sync: change comparison on sync_refresh_period

2017-04-12 Thread Simon Horman

On Wed, Apr 12, 2017 at 04:38:12PM -0400, Aaron Conole wrote:
> The sync_refresh_period variable is unsigned, so it can never be < 0.
> 
> Signed-off-by: Aaron Conole 

Thanks Aaron,

I have applied this to ipvs-next after updating the prefix to "ipvs:".
--
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] netfilter: nft_hash: do not dump the auto generated seed

2017-04-12 Thread Florian Westphal

Liping Zhang  wrote:
> >> +++ b/net/netfilter/nft_hash.c
> >> @@ -21,6 +21,7 @@ struct nft_hash {
> >> enum nft_registers  sreg:8;
> >> enum nft_registers  dreg:8;
> >> u8  len;
> >> +   boolautogen_seed:1;
> >
> > Hi Liping, I don't think that hiding the seed value would be useful, and
> > even adding this attribute doesn't worth it just to hide the seed.
> >
> 
> If we don't do this thing, if the user inputting the following nft rules:
>   # nft add rule x y ct mark set jhash ip saddr mod 2
> 
> Then nft list ruleset will display something like this, where 0xd6ab633c
> is very unpredictable, and the user doesn't care the seed at all:
>   ct mark set jhash ip saddr mod 2 seed 0xd6ab633c
> 
> This will cause annoying complain when running "nft-test.py ip/hash.t".
> 
> But another problem is this, I remember that Pablo is implementing a new
> delete rule syntax, something like this:
> "nft del rule x y ct mark set jhash ip saddr mod 2"
> 
> The unpredictable seed will cause the above rule failed, since the seed
> is not the same, so we cannot find a matched nft rule.

FWIW I agree with Liping, we should eat the extra bool and
supress seed dump.

I also think most users should not ever have to even know of seed arg
existence.

In fact is there a use case where it is needed?
We might want to ditch it completely and always just
generate it privately in kernel (symhash f.e. uses
a private seed) already.
--
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 nf-next] ip_vs_sync: change comparison on sync_refresh_period

2017-04-12 Thread Aaron Conole

The sync_refresh_period variable is unsigned, so it can never be < 0.

Signed-off-by: Aaron Conole 
---
 net/netfilter/ipvs/ip_vs_sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index b03c280..123dc0f 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -520,7 +520,7 @@ static int ip_vs_sync_conn_needed(struct netns_ipvs *ipvs,
if (!(cp->flags & IP_VS_CONN_F_TEMPLATE) &&
pkts % sync_period != sysctl_sync_threshold(ipvs))
return 0;
-   } else if (sync_refresh_period <= 0 &&
+   } else if (!sync_refresh_period &&
   pkts != sysctl_sync_threshold(ipvs))
return 0;
 
-- 
2.9.3

--
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 nf-next] nf_conntrack: remove double assignment

2017-04-12 Thread Aaron Conole

The protonet pointer will unconditionally be rewritten, so just do the
needed assignment first.

Signed-off-by: Aaron Conole 
---
 net/netfilter/nf_conntrack_proto.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto.c 
b/net/netfilter/nf_conntrack_proto.c
index 2d6ee18..e6d2945 100644
--- a/net/netfilter/nf_conntrack_proto.c
+++ b/net/netfilter/nf_conntrack_proto.c
@@ -441,9 +441,8 @@ EXPORT_SYMBOL_GPL(nf_ct_l4proto_unregister_one);
 void nf_ct_l4proto_pernet_unregister_one(struct net *net,
 struct nf_conntrack_l4proto *l4proto)
 {
-   struct nf_proto_net *pn = NULL;
+   struct nf_proto_net *pn = nf_ct_l4proto_net(net, l4proto);
 
-   pn = nf_ct_l4proto_net(net, l4proto);
if (pn == NULL)
return;
 
-- 
2.9.3

--
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 nf-next] nf_tables: remove double return statement

2017-04-12 Thread Aaron Conole

Signed-off-by: Aaron Conole 
---
 net/netfilter/nf_tables_api.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 2d822d2..1452fb7 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4435,8 +4435,6 @@ static int nf_tables_getobj(struct net *net, struct sock 
*nlsk,
 err:
kfree_skb(skb2);
return err;
-
-   return 0;
 }
 
 static void nft_obj_destroy(struct nft_object *obj)
-- 
2.9.3

--
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 nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Liping Zhang

From: Liping Zhang 

User can update the ct->status via nfnetlink, but using a non-atomic
operation "ct->status |= status;". This is unsafe, and may clear
IPS_DYING_BIT bit set by another CPU unexpectedly. For example:
 CPU0CPU1
  ctnetlink_change_status__nf_conntrack_find_get
  old = ct->status  nf_ct_gc_expired
  - nf_ct_kill
  -  test_and_set_bit(IPS_DYING_BIT
  - -
  ct->status = old | status;<-- oops, _DYING_ is cleared!

So using a series of atomic bit operation to solve the above issue.

Also note, user shouldn't set IPS_TEMPLATE, IPS_SEQ_ADJUST directly.

If we set the IPS_TEMPLATE_BIT, ct will be freed by nf_ct_tmpl_free,
but actually it is alloced by nf_conntrack_alloc.

If we set the IPS_SEQ_ADJUST_BIT, this may cause the NULL pointer
deference, as the nfct_seqadj(ct) maybe NULL.

So make these two bits be unchangable too.

Last, add some comments to describe the logic change due to the
commit a963d710f367 ("netfilter: ctnetlink: Fix regression in CTA_STATUS
processing"), which makes me feel a little confusing.

Signed-off-by: Liping Zhang 
---
 include/uapi/linux/netfilter/nf_conntrack_common.h | 13 +---
 net/netfilter/nf_conntrack_netlink.c   | 35 --
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h 
b/include/uapi/linux/netfilter/nf_conntrack_common.h
index 6a8e33d..38fc383 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
@@ -82,10 +82,6 @@ enum ip_conntrack_status {
IPS_DYING_BIT = 9,
IPS_DYING = (1 << IPS_DYING_BIT),
 
-   /* Bits that cannot be altered from userland. */
-   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
-IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING),
-
/* Connection has fixed timeout. */
IPS_FIXED_TIMEOUT_BIT = 10,
IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT),
@@ -101,6 +97,15 @@ enum ip_conntrack_status {
/* Conntrack got a helper explicitly attached via CT target. */
IPS_HELPER_BIT = 13,
IPS_HELPER = (1 << IPS_HELPER_BIT),
+
+   /* Be careful here, modifying these bits can make things messy,
+* so don't let users modify them directly.
+*/
+   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
+IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING |
+IPS_SEQ_ADJUST | IPS_TEMPLATE),
+
+   __IPS_MAX_BIT = 14,
 };
 
 /* Connection tracking event types */
diff --git a/net/netfilter/nf_conntrack_netlink.c 
b/net/netfilter/nf_conntrack_netlink.c
index 908d858..68efe1a 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1419,6 +1419,26 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,
 }
 #endif
 
+static void
+__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
+ unsigned long off)
+{
+   unsigned long mask;
+   unsigned int bit;
+
+   for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
+   mask = 1 << bit;
+   /* Ignore these unchangable bits */
+   if (mask & IPS_UNCHANGEABLE_MASK)
+   continue;
+
+   if (on & mask)
+   set_bit(bit, >status);
+   else if (off & mask)
+   clear_bit(bit, >status);
+   }
+}
+
 static int
 ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
 {
@@ -1438,10 +1458,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct 
nlattr * const cda[])
/* ASSURED bit can only be set */
return -EBUSY;
 
-   /* Be careful here, modifying NAT bits can screw up things,
-* so don't let users modify them directly if they don't pass
-* nf_nat_range. */
-   ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+   __ctnetlink_change_status(ct, status, 0);
return 0;
 }
 
@@ -1632,7 +1649,7 @@ ctnetlink_change_seq_adj(struct nf_conn *ct,
if (ret < 0)
return ret;
 
-   ct->status |= IPS_SEQ_ADJUST;
+   set_bit(IPS_SEQ_ADJUST_BIT, >status);
}
 
if (cda[CTA_SEQ_ADJ_REPLY]) {
@@ -1641,7 +1658,7 @@ ctnetlink_change_seq_adj(struct nf_conn *ct,
if (ret < 0)
return ret;
 
-   ct->status |= IPS_SEQ_ADJUST;
+   set_bit(IPS_SEQ_ADJUST_BIT, >status);
}
 
return 0;
@@ -2295,10 +2312,10 @@ ctnetlink_update_status(struct nf_conn *ct, const 
struct nlattr * const cda[])
/* This check is less strict than ctnetlink_change_status()
 *

Re: [PATCH ulogd2 1/2] ulogd.conf: harmonize log file options with module default values

2017-04-12 Thread Kaarle Ritvanen

On Tue, 7 Mar 2017, Eric Leblond wrote:

> I really like the idea of getting an harmonized naming for the log
> files but I think we should do it reverse for values that are not
> commented in the configuration file. Most distributions and install are
> shipping with a copy of default configuration. 
> 
> Most users won't have changed the values that are uncommented. So they
> will have on disk the log file with name defined in the configuration
> file. And they will benefit from a new logrotate file. Thus we should
> use the file name defined in the conf as value of the default file name
> in code.
> 
> Could you resubmit a patchset ?

I preprared a new patch set according to your feedback:

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

Please take a look at them.

BR,
Kaarle
--
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 nf 1/1] netfilter: nf_nat: Fix return NF_DROP in nfnetlink_parse_nat_setup

2017-04-12 Thread gfree . wind

From: Gao Feng 

The __nf_nat_alloc_null_binding invokes nf_nat_setup_info which may
return NF_DROP when memory is exhausted, so convert NF_DROP to -ENOMEM
to make ctnetlink happy. Or ctnetlink_setup_nat treats it as a success
when one error NF_DROP happens actully.

Signed-off-by: Gao Feng 
---
 net/netfilter/nf_nat_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 82802e4..55746cd 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -819,7 +819,7 @@ static int nfnetlink_parse_nat_proto(struct nlattr *attr,
 
/* No NAT information has been passed, allocate the null-binding */
if (attr == NULL)
-   return __nf_nat_alloc_null_binding(ct, manip);
+   return __nf_nat_alloc_null_binding(ct, manip) == NF_DROP ? 
-ENOMEM : 0;
 
err = nfnetlink_parse_nat(attr, ct, , l3proto);
if (err < 0)
-- 
1.9.1




--
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] netfilter: ctnetlink: make it safer when updating ct->status

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

Re: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

Re: [PATCH nf-next] ip_vs_sync: change comparison on sync_refresh_period

Re: [PATCH nf] netfilter: nft_hash: do not dump the auto generated seed

[PATCH nf-next] ip_vs_sync: change comparison on sync_refresh_period

[PATCH nf-next] nf_conntrack: remove double assignment

[PATCH nf-next] nf_tables: remove double return statement

[PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

Re: [PATCH ulogd2 1/2] ulogd.conf: harmonize log file options with module default values

[PATCH nf 1/1] netfilter: nf_nat: Fix return NF_DROP in nfnetlink_parse_nat_setup

14 matches

Site Navigation

Mail list logo

Footer information