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

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

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

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:
         CPU0                            CPU1
  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 <zlpnobody@gmail.com>
---
 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, &ct->status);
+		else if (off & mask)
+			clear_bit(bit, &ct->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, &ct->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, &ct->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()
 	 * because callers often flip IPS_EXPECTED bits when sending
 	 * an NFQA_CT attribute to the kernel.  So ignore the
-	 * unchangeable bits but do not error out.
+	 * unchangeable bits but do not error out. Also user programs
+	 * are allowed to clear the bits that they are allowed to change.
 	 */
-	ct->status = (status & ~IPS_UNCHANGEABLE_MASK) |
-		     (ct->status & IPS_UNCHANGEABLE_MASK);
+	__ctnetlink_change_status(ct, status, ~status);
 	return 0;
 }
 
-- 
2.5.5

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

[Gao Feng]

Hi Liping,

> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org
> [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of Liping Zhang
> Sent: Wednesday, April 12, 2017 11:57 PM
> To: pablo@netfilter.org
> Cc: netfilter-devel@vger.kernel.org; cernekee@chromium.org; Liping Zhang
> <zlpnobody@gmail.com>
> Subject: [PATCH nf] netfilter: ctnetlink: make it safer when updating
ct->status
> 
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> 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:
>          CPU0                            CPU1
>   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 <zlpnobody@gmail.com>
> ---
>  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&mask);

Best Regards
Feng

> +
> +		if (on & mask)
> +			set_bit(bit, &ct->status);
> +		else if (off & mask)
> +			clear_bit(bit, &ct->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, &ct->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, &ct->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()
>  	 * because callers often flip IPS_EXPECTED bits when sending
>  	 * an NFQA_CT attribute to the kernel.  So ignore the
> -	 * unchangeable bits but do not error out.
> +	 * unchangeable bits but do not error out. Also user programs
> +	 * are allowed to clear the bits that they are allowed to change.
>  	 */
> -	ct->status = (status & ~IPS_UNCHANGEABLE_MASK) |
> -		     (ct->status & IPS_UNCHANGEABLE_MASK);
> +	__ctnetlink_change_status(ct, status, ~status);
>  	return 0;
>  }
> 
> --
> 2.5.5
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel"
in
> the body of a message to majordomo@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

[Gao Feng]

> -----Original Message-----
> From: Gao Feng [mailto:gfree.wind@foxmail.com]
> Sent: Thursday, April 13, 2017 10:42 AM
> To: 'Liping Zhang' <zlpnobody@163.com>; 'pablo@netfilter.org'
> <pablo@netfilter.org>
> Cc: 'netfilter-devel@vger.kernel.org' <netfilter-devel@vger.kernel.org>;
> 'cernekee@chromium.org' <cernekee@chromium.org>; 'Liping Zhang'
> <zlpnobody@gmail.com>
> Subject: RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> ct->status
> 
> Hi Liping,
> 
> > -----Original Message-----
> > From: netfilter-devel-owner@vger.kernel.org
> > [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of Liping
> > Zhang
> > Sent: Wednesday, April 12, 2017 11:57 PM
> > To: pablo@netfilter.org
> > Cc: netfilter-devel@vger.kernel.org; cernekee@chromium.org; Liping
> > Zhang <zlpnobody@gmail.com>
> > Subject: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> > ct->status
> >
> > From: Liping Zhang <zlpnobody@gmail.com>
> >
> > 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:
> >          CPU0                            CPU1
> >   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 <zlpnobody@gmail.com>
> > ---
> >  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&mask);

It is typo. Should be BUILD_BUG_ON(on&off).

Regards
Feng
> 
> Best Regards
> Feng
> 
> > +
> > +		if (on & mask)
> > +			set_bit(bit, &ct->status);
> > +		else if (off & mask)
> > +			clear_bit(bit, &ct->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, &ct->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, &ct->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()
> >  	 * because callers often flip IPS_EXPECTED bits when sending
> >  	 * an NFQA_CT attribute to the kernel.  So ignore the
> > -	 * unchangeable bits but do not error out.
> > +	 * unchangeable bits but do not error out. Also user programs
> > +	 * are allowed to clear the bits that they are allowed to change.
> >  	 */
> > -	ct->status = (status & ~IPS_UNCHANGEABLE_MASK) |
> > -		     (ct->status & IPS_UNCHANGEABLE_MASK);
> > +	__ctnetlink_change_status(ct, status, ~status);
> >  	return 0;
> >  }
> >
> > --
> > 2.5.5
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > netfilter-devel" in the body of a message to majordomo@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

[Liping Zhang]

Hi Feng,

2017-04-13 10:42 GMT+08:00 Gao Feng <gfree.wind@foxmail.com>:
[...]
>> +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&mask);

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.

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

[Gao Feng]

Hi Liping,

> -----Original Message-----
> From: Liping Zhang [mailto:zlpnobody@gmail.com]
> Sent: Thursday, April 13, 2017 11:15 AM
> To: Gao Feng <gfree.wind@foxmail.com>
> Cc: Liping Zhang <zlpnobody@163.com>; Pablo Neira Ayuso
> <pablo@netfilter.org>; Netfilter Developer Mailing List
> <netfilter-devel@vger.kernel.org>; cernekee@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 <gfree.wind@foxmail.com>:
> [...]
> >> +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&mask);
> 
> 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

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

[Liping Zhang]

Hi Feng,

2017-04-13 11:22 GMT+08:00 Gao Feng <gfree.wind@foxmail.com>:
[...]
>> 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 :)