On Tue, Mar 21, 2017 at 10:35:43PM +0800, Liping Zhang wrote:
> Hi Pablo,
>
> 2017-03-21 18:27 GMT+08:00 Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>:
> [...]
> >> + class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
> >> + if (class_max == 0)
> >> + return -EINVAL;
> >
> > I think this patch is just fixing up this case. We should always
> > provide a policy for the expectation.
>
> No, see comments below.
>
> >
> >> + if (class_max > NF_CT_MAX_EXPECT_CLASSES)
> >> return -EOVERFLOW;
> >>
> >> expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
> >> - helper->expect_class_max, GFP_KERNEL);
> >> + class_max, GFP_KERNEL);
> >> if (expect_policy == NULL)
> >> return -ENOMEM;
> >>
> >> - for (i=0; i<helper->expect_class_max; i++) {
> >> + for (i = 0; i < class_max; i++) {
> >> if (!tb[NFCTH_POLICY_SET+i])
> >> goto err;
> >>
> >> @@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
> >> if (ret < 0)
> >> goto err;
> >> }
> >> +
> >> + helper->expect_class_max = class_max - 1;
> >
> > Why - 1 ?
> >
> > class_max represents the array size, you can just keep this code the
> > way it is.
>
> Actually, expect_class_max represents the array size minus 1.
>
> For sip, expect_class_max is set to SIP_EXPECT_MAX(3). For ftp,
> expect_class_max is set to 0.
>
> Also there's a "BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);"
> in nf_conntrack_helper_register, so if we supply 4 expect_policys,
> i.e. expect_class_max is 4, BUG_ON will happen.
Right, from the kernel side, all helpers assume that
me->expect_class_max represents the array size - 1.
While cthelper takes this as the array size.
This expect_class_max semantics is counterintuitive to me.
Applied, thanks.
P.S: I'm going to send a new version of:
http://patchwork.ozlabs.org/patch/741528/
that applies on top of this one.
--
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
