All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH lnfct 0/2] fix - src: add support for IPv6 NAT
@ 2017-02-28  4:53 Ken-ichirou MATSUZAWA
  2017-02-28  4:55 ` [PATCH lnfct 1/2] conntrack: fix missing break Ken-ichirou MATSUZAWA
  2017-02-28  5:00 ` [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition Ken-ichirou MATSUZAWA
  0 siblings, 2 replies; 10+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2017-02-28  4:53 UTC (permalink / raw)
  To: The netfilter developer mailinglist

 Hi,

This patch series fixes commit:

    73ad642ba462d0992e1903012eee4ebfec89ed69
    f5e51ad64d9e5597e8880b652abe261585c2563d

Conditions in getobjopt_is_snat() / getobjopt_is_dnat() has
been changed in the commit above and fixed by this, but I do
not understand proper way to check a nfct content is correct.
Please review it.

Thanks,

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH lnfct 1/2] conntrack: fix missing break
  2017-02-28  4:53 [PATCH lnfct 0/2] fix - src: add support for IPv6 NAT Ken-ichirou MATSUZAWA
@ 2017-02-28  4:55 ` Ken-ichirou MATSUZAWA
  2017-02-28 10:47   ` Pablo Neira Ayuso
  2017-02-28  5:00 ` [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition Ken-ichirou MATSUZAWA
  1 sibling, 1 reply; 10+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2017-02-28  4:55 UTC (permalink / raw)
  To: The netfilter developer mailinglist

Signed-off-by Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 src/conntrack/objopt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conntrack/objopt.c b/src/conntrack/objopt.c
index 119a83a..fb43d6c 100644
--- a/src/conntrack/objopt.c
+++ b/src/conntrack/objopt.c
@@ -81,6 +81,7 @@ static void setobjopt_undo_dnat(struct nf_conntrack *ct)
 		ct->dnat.max_ip.v4 = ct->dnat.min_ip.v4;
 		ct->repl.src.v4 = ct->head.orig.dst.v4;
 		set_bit(ATTR_DNAT_IPV4, ct->head.set);
+		break;
 	case AF_INET6:
 		memcpy(&ct->dnat.min_ip.v6, &ct->repl.src.v6,
 		       sizeof(struct in6_addr));
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition
  2017-02-28  4:53 [PATCH lnfct 0/2] fix - src: add support for IPv6 NAT Ken-ichirou MATSUZAWA
  2017-02-28  4:55 ` [PATCH lnfct 1/2] conntrack: fix missing break Ken-ichirou MATSUZAWA
@ 2017-02-28  5:00 ` Ken-ichirou MATSUZAWA
  2017-02-28 10:47   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 10+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2017-02-28  5:00 UTC (permalink / raw)
  To: The netfilter developer mailinglist

>From 9e8aa4ed079b526faf190b69a2c1032f22776602 Mon Sep 17 00:00:00 2001
From: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
Date: Tue, 28 Feb 2017 11:34:29 +0900
Subject: [PATCH 2/2] conntrack: revert getobjopt_is_nat condition

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 src/conntrack/objopt.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/conntrack/objopt.c b/src/conntrack/objopt.c
index fb43d6c..1581480 100644
--- a/src/conntrack/objopt.c
+++ b/src/conntrack/objopt.c
@@ -144,10 +144,8 @@ int __setobjopt(struct nf_conntrack *ct, unsigned int option)
 
 static int getobjopt_is_snat(const struct nf_conntrack *ct)
 {
-	if (!(test_bit(ATTR_STATUS, ct->head.set)))
-		return 0;
-
-	if (!(ct->status & IPS_SRC_NAT_DONE))
+	if (test_bit(ATTR_STATUS, ct->head.set) &&
+	    !(ct->status & IPS_SRC_NAT_DONE))
 		return 0;
 
 	switch (ct->head.orig.l3protonum) {
@@ -166,10 +164,8 @@ static int getobjopt_is_snat(const struct nf_conntrack *ct)
 
 static int getobjopt_is_dnat(const struct nf_conntrack *ct)
 {
-	if (!(test_bit(ATTR_STATUS, ct->head.set)))
-		return 0;
-
-	if (!(ct->status & IPS_DST_NAT_DONE))
+	if (test_bit(ATTR_STATUS, ct->head.set) &&
+	    !(ct->status & IPS_DST_NAT_DONE))
 		return 0;
 
 	switch (ct->head.orig.l3protonum) {
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition
  2017-02-28  5:00 ` [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition Ken-ichirou MATSUZAWA
@ 2017-02-28 10:47   ` Pablo Neira Ayuso
  2017-02-28 11:44     ` Ken-ichirou MATSUZAWA
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-28 10:47 UTC (permalink / raw)
  To: Ken-ichirou MATSUZAWA; +Cc: The netfilter developer mailinglist

Hi Ken-ichirou,

On Tue, Feb 28, 2017 at 02:00:41PM +0900, Ken-ichirou MATSUZAWA wrote:
> From 9e8aa4ed079b526faf190b69a2c1032f22776602 Mon Sep 17 00:00:00 2001
> From: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
> Date: Tue, 28 Feb 2017 11:34:29 +0900
> Subject: [PATCH 2/2] conntrack: revert getobjopt_is_nat condition
> 
> Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
> ---
>  src/conntrack/objopt.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/src/conntrack/objopt.c b/src/conntrack/objopt.c
> index fb43d6c..1581480 100644
> --- a/src/conntrack/objopt.c
> +++ b/src/conntrack/objopt.c
> @@ -144,10 +144,8 @@ int __setobjopt(struct nf_conntrack *ct, unsigned int option)
>  
>  static int getobjopt_is_snat(const struct nf_conntrack *ct)
>  {
> -	if (!(test_bit(ATTR_STATUS, ct->head.set)))
> -		return 0;
> -
> -	if (!(ct->status & IPS_SRC_NAT_DONE))
> +	if (test_bit(ATTR_STATUS, ct->head.set) &&
> +	    !(ct->status & IPS_SRC_NAT_DONE))

However, if ATTR_STATUS is not set, we keep checking ahead. What are
you trying to fix?

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH lnfct 1/2] conntrack: fix missing break
  2017-02-28  4:55 ` [PATCH lnfct 1/2] conntrack: fix missing break Ken-ichirou MATSUZAWA
@ 2017-02-28 10:47   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-28 10:47 UTC (permalink / raw)
  To: Ken-ichirou MATSUZAWA; +Cc: The netfilter developer mailinglist

Applied, thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition
  2017-02-28 10:47   ` Pablo Neira Ayuso
@ 2017-02-28 11:44     ` Ken-ichirou MATSUZAWA
  2017-02-28 11:48       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2017-02-28 11:44 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: The netfilter developer mailinglist

 Hi, Pablo

On Tue, Feb 28, 2017 at 11:47:25AM +0100, Pablo Neira Ayuso wrote:
> > diff --git a/src/conntrack/objopt.c b/src/conntrack/objopt.c
> > index fb43d6c..1581480 100644
> > --- a/src/conntrack/objopt.c
> > +++ b/src/conntrack/objopt.c
> > @@ -144,10 +144,8 @@ int __setobjopt(struct nf_conntrack *ct, unsigned int option)
> >  
> >  static int getobjopt_is_snat(const struct nf_conntrack *ct)
> >  {
> > -	if (!(test_bit(ATTR_STATUS, ct->head.set)))
> > -		return 0;
> > -
> > -	if (!(ct->status & IPS_SRC_NAT_DONE))
> > +	if (test_bit(ATTR_STATUS, ct->head.set) &&
> > +	    !(ct->status & IPS_SRC_NAT_DONE))
> 
> However, if ATTR_STATUS is not set, we keep checking ahead. What are
> you trying to fix?

It was:

-       return ((test_bit(ATTR_STATUS, ct->head.set) ?
-               ct->status & IPS_SRC_NAT_DONE : 1) &&
-               ct->repl.dst.v4 !=
-               ct->head.orig.src.v4);

I thought it keeps checking even ATTR_STATUS is not set.
But it's ok not to apply, returning false in case of
ATTR_STATUS is not set.

Thanks,

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition
  2017-02-28 11:44     ` Ken-ichirou MATSUZAWA
@ 2017-02-28 11:48       ` Pablo Neira Ayuso
  2017-02-28 22:29         ` Ken-ichirou MATSUZAWA
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-28 11:48 UTC (permalink / raw)
  To: Ken-ichirou MATSUZAWA; +Cc: The netfilter developer mailinglist

On Tue, Feb 28, 2017 at 08:44:53PM +0900, Ken-ichirou MATSUZAWA wrote:
>  Hi, Pablo
> 
> On Tue, Feb 28, 2017 at 11:47:25AM +0100, Pablo Neira Ayuso wrote:
> > > diff --git a/src/conntrack/objopt.c b/src/conntrack/objopt.c
> > > index fb43d6c..1581480 100644
> > > --- a/src/conntrack/objopt.c
> > > +++ b/src/conntrack/objopt.c
> > > @@ -144,10 +144,8 @@ int __setobjopt(struct nf_conntrack *ct, unsigned int option)
> > >  
> > >  static int getobjopt_is_snat(const struct nf_conntrack *ct)
> > >  {
> > > -	if (!(test_bit(ATTR_STATUS, ct->head.set)))
> > > -		return 0;
> > > -
> > > -	if (!(ct->status & IPS_SRC_NAT_DONE))
> > > +	if (test_bit(ATTR_STATUS, ct->head.set) &&
> > > +	    !(ct->status & IPS_SRC_NAT_DONE))
> > 
> > However, if ATTR_STATUS is not set, we keep checking ahead. What are
> > you trying to fix?
> 
> It was:
> 
> -       return ((test_bit(ATTR_STATUS, ct->head.set) ?
> -               ct->status & IPS_SRC_NAT_DONE : 1) &&
> -               ct->repl.dst.v4 !=
> -               ct->head.orig.src.v4);
> 
> I thought it keeps checking even ATTR_STATUS is not set.
> But it's ok not to apply, returning false in case of
> ATTR_STATUS is not set.

Ah, I see.

static int getobjopt_is_snat(const struct nf_conntrack *ct)
{
        if (!(test_bit(ATTR_STATUS, ct->head.set)))
                return 0;

        if (!(ct->status & IPS_SRC_NAT_DONE))
                return 0;

        switch (ct->head.orig.l3protonum) {
        case AF_INET:
                return ct->repl.dst.v4 != ct->head.orig.src.v4;
        case AF_INET6:
                if (memcmp(&ct->repl.dst.v6, &ct->head.orig.src.v6,
                           sizeof(struct in6_addr)) != 0)
                        return 1;
                else
                        return 0;
        default:
                return 0;
        }
}

So you want to check if the addresses mismatch, so we infer from there
if there is NAT or not when status bits are not available.

Are you trying to catch up some case in netlink event specifically?

Thanks for explaining.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition
  2017-02-28 11:48       ` Pablo Neira Ayuso
@ 2017-02-28 22:29         ` Ken-ichirou MATSUZAWA
  2017-03-01 16:28           ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: Ken-ichirou MATSUZAWA @ 2017-02-28 22:29 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: The netfilter developer mailinglist

 Hi, Pablo
 
On Tue, Feb 28, 2017 at 12:48:09PM +0100, Pablo Neira Ayuso wrote:
> So you want to check if the addresses mismatch, so we infer from there
> if there is NAT or not when status bits are not available.
> 
> Are you trying to catch up some case in netlink event specifically?

It's nothing. My skimping test for lnfct binding for another languate
which set only ATTR_REPL_IPV4DST then get NFCT_GOPT_IS_NAT was success,
but it fails now.

Thanks,

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition
  2017-02-28 22:29         ` Ken-ichirou MATSUZAWA
@ 2017-03-01 16:28           ` Pablo Neira Ayuso
  2017-03-03 12:18             ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2017-03-01 16:28 UTC (permalink / raw)
  To: Ken-ichirou MATSUZAWA; +Cc: The netfilter developer mailinglist

On Wed, Mar 01, 2017 at 07:29:33AM +0900, Ken-ichirou MATSUZAWA wrote:
>  Hi, Pablo
>  
> On Tue, Feb 28, 2017 at 12:48:09PM +0100, Pablo Neira Ayuso wrote:
> > So you want to check if the addresses mismatch, so we infer from there
> > if there is NAT or not when status bits are not available.
> > 
> > Are you trying to catch up some case in netlink event specifically?
> 
> It's nothing. My skimping test for lnfct binding for another languate
> which set only ATTR_REPL_IPV4DST then get NFCT_GOPT_IS_NAT was success,
> but it fails now.

I see, we should restore the original behaviour, I think your patch is
correct then.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition
  2017-03-01 16:28           ` Pablo Neira Ayuso
@ 2017-03-03 12:18             ` Pablo Neira Ayuso
  0 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2017-03-03 12:18 UTC (permalink / raw)
  To: Ken-ichirou MATSUZAWA; +Cc: The netfilter developer mailinglist

On Wed, Mar 01, 2017 at 05:28:02PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Mar 01, 2017 at 07:29:33AM +0900, Ken-ichirou MATSUZAWA wrote:
> >  Hi, Pablo
> >  
> > On Tue, Feb 28, 2017 at 12:48:09PM +0100, Pablo Neira Ayuso wrote:
> > > So you want to check if the addresses mismatch, so we infer from there
> > > if there is NAT or not when status bits are not available.
> > > 
> > > Are you trying to catch up some case in netlink event specifically?
> > 
> > It's nothing. My skimping test for lnfct binding for another languate
> > which set only ATTR_REPL_IPV4DST then get NFCT_GOPT_IS_NAT was success,
> > but it fails now.
> 
> I see, we should restore the original behaviour, I think your patch is
> correct then.

JFYI: I have applied this patch, thanks for explaining.

BTW, it would be great if you can add some short description to your
follow up patch. If English is a concern, don't bother much about it,
we're non-native speaker mostly :)

Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-03 15:24 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-28  4:53 [PATCH lnfct 0/2] fix - src: add support for IPv6 NAT Ken-ichirou MATSUZAWA
2017-02-28  4:55 ` [PATCH lnfct 1/2] conntrack: fix missing break Ken-ichirou MATSUZAWA
2017-02-28 10:47   ` Pablo Neira Ayuso
2017-02-28  5:00 ` [PATCH lnfct 2/2] conntrack: revert getobjopt_is_nat condition Ken-ichirou MATSUZAWA
2017-02-28 10:47   ` Pablo Neira Ayuso
2017-02-28 11:44     ` Ken-ichirou MATSUZAWA
2017-02-28 11:48       ` Pablo Neira Ayuso
2017-02-28 22:29         ` Ken-ichirou MATSUZAWA
2017-03-01 16:28           ` Pablo Neira Ayuso
2017-03-03 12:18             ` Pablo Neira Ayuso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.