Hi Liping, > -----Original Message----- > From: Liping Zhang [mailto:zlpnobody@xxxxxxxxx] > Sent: Thursday, April 13, 2017 11:15 AM > To: Gao Feng <gfree.wind@xxxxxxxxxxx> > Cc: Liping Zhang <zlpnobody@xxxxxxx>; Pablo Neira Ayuso > <pablo@xxxxxxxxxxxxx>; Netfilter Developer Mailing List > <netfilter-devel@xxxxxxxxxxxxxxx>; cernekee@xxxxxxxxxxxx > 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@xxxxxxxxxxx>: > [...] > >> +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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
