On Tue, Jun 11, 2019 at 01:20:59PM +0800, xiao ruizhu wrote:
> > On Tue, Jun 11, 2019 at 01:45AM, Pablo Neira Ayuso <[email protected]>
> > wrote:
>
> > Looks good, only one more little change and we go.
>
> >> On Tue, Jun 04, 2019 at 04:34:23PM +0800, xiao ruizhu wrote:
> >> [...]
> >> @@ -420,8 +421,10 @@ static inline int __nf_ct_expect_check(struct
> >> nf_conntrack_expect *expect)
> >> }
> >> h = nf_ct_expect_dst_hash(net, &expect->tuple);
> >> hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
> >> - if (expect_matches(i, expect)) {
> >> - if (i->class != expect->class)
> >> + if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
> >> + expect->master) && expect_matches(i, expect)) {
> >
> > Could you add a function for this? eg.
> >
> > static bool nf_ct_check_master(struct nf_conntrack_expect *a,
> > struct nf_conntrack_expect *b)
> > {
> > if (flags & NF_CT_EXP_F_CHECK_MASTER)
> > return true;
> >
> > return i->master == expect->master &&
> > expect_matches(i, expect);
> > }
>
> > Was that the intention?
>
> > I'm a bit confused with the use of the single statement branch above.
>
> Hi Pablo,
>
> Thanks for your notice.
> Sorry, I made a mistake here. I meant to move the checking of master from
> expect_matches() to __nf_ct_expect_check(), where we will use the flag
> NF_CT_EXP_F_CHECK_MASTER to decide whether masters also need to be checked
> or not for matching.
> That is, the intention is to change expect_matches() from the original
> {
> return a->master == b->master &&
> nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
> nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
> net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
> nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
> }
> to
> {
> return nf_ct_tuple_equal(&a->tuple, &b->tuple) &&
> nf_ct_tuple_mask_equal(&a->mask, &b->mask) &&
> net_eq(nf_ct_net(a->master), nf_ct_net(b->master)) &&
> nf_ct_zone_equal_any(a->master, nf_ct_zone(b->master));
> }
> And in __nf_ct_expect_check(), if the expect is for SIP helper (i.e. with
> NF_CT_EXP_F_CHECK_MASTER set), the master will not be checked, only the
> items in new expect_matches() will be used for matching check; otherwise,
> masters will also be checked. That's the purpose of (flags &
> NF_CT_EXP_F_CHECK_MASTER ? true : i->master == expect->master).
[...]
> @@ -420,8 +420,10 @@ static inline int __nf_ct_expect_check(struct
> nf_conntrack_expect *expect)
> }
> h = nf_ct_expect_dst_hash(net, &expect->tuple);
> hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
> - if (expect_matches(i, expect)) {
> - if (i->class != expect->class)
> + if ((flags & NF_CT_EXP_F_CHECK_MASTER ? true : i->master ==
> + expect->master) && expect_matches(i, expect)) {
This part is slightly hard to read. Could you add a function? For
example:
static bool master_matches(...)
{
if (flags & NF_CT_EXP_F_CHECK_MASTER)
return true;
return i->master == expect->master;
}
Then use it:
if (master_matches(i, expect) &&
expect_matches(i, expect)) {
> + if (i->class != expect->class ||
> + i->master != expect->master)
> return -EALREADY;
>
> if (nf_ct_remove_expect(i))
Thanks!
