Re: [PATCH v6] netfilter: nf_conntrack_sip: fix expectation clash

[Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>]

On Tue, Jun 11, 2019 at 01:20:59PM +0800, xiao ruizhu wrote:
> > On Tue, Jun 11, 2019 at 01:45AM, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > 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!