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

["Gao Feng" <gfree.wind@foxmail.com>]

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