Hi!
On Fri, May 20, 2011 at 03:54:03PM +0400, Vadim Zhukov wrote:
> This patch splits off IMSG_CFG_POLICY into four messages:
>
> IMSG_CFG_POLICY_BEGIN
> IMSG_CFG_POLICY_PROPOSAL
> IMSG_CFG_POLICY_FLOW
> IMSG_CFG_POLICY_COMMIT
>
> Each new policy should start with IMSG_CFG_POLICY_BEGIN, then proceed
> any number of proposals and flows, and then commit policy. I decided
> not to use ticket system because I see no race sources.
>
> Now we can have long, long policies in your iked.conf. Previously we were
> limited by maximum imsg packet size (MAX_IMSGSIZE) which is 16 kilobytes
> currently.
>
Thanks for you diff. See comments below. It is appreciated and I can
adjust it but you can see my comments for modifications below.
reyk
> Index: ikev2.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/ikev2.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 ikev2.c
> --- ikev2.c 9 May 2011 11:15:18 -0000 1.55
> +++ ikev2.c 20 May 2011 09:48:06 -0000
> @@ -133,8 +133,14 @@ ikev2_dispatch_parent(int fd, struct pri
> return (config_getsocket(env, imsg, ikev2_msg_cb));
> case IMSG_PFKEY_SOCKET:
> return (config_getpfkey(env, imsg));
> - case IMSG_CFG_POLICY:
> - return (config_getpolicy(env, imsg));
> + case IMSG_CFG_POLICY_BEGIN:
> + return (config_getpolicy_begin(env, imsg));
> + case IMSG_CFG_POLICY_PROPOSAL:
> + return (config_getpolicy_proposal(env, imsg));
> + case IMSG_CFG_POLICY_FLOW:
> + return (config_getpolicy_flow(env, imsg));
> + case IMSG_CFG_POLICY_COMMIT:
> + return (config_getpolicy_commit(env, imsg));
The naming should be simplified just to
IMSG_CFG_POLICY
IMSG_CFG_PROPOSAL
IMSG_CFG_FLOW
same for the functions
config_getpolicy
config_getproposal
config_getflow
The commit step is not required because the existing IMSG_COMPILE
finishes the policy configuration ones, calculates the skip steps and
starts operation.
See more comments below. There are some hints to make it fully async
below (not reviewing the code in detail).
> case IMSG_CFG_USER:
> return (config_getuser(env, imsg));
> case IMSG_COMPILE:
> Index: config.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/config.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 config.c
> --- config.c 9 May 2011 11:15:18 -0000 1.12
> +++ config.c 20 May 2011 09:48:06 -0000
> @@ -45,6 +45,10 @@
> #include "iked.h"
> #include "ikev2.h"
>
> +
> +static struct iked_policy *newpol = NULL;
> +
> +
Please don't use a global variable here. Just add the policy to the
global list in config_getpolicy() and reference to the policy by id
in the config_getproposal() and config_getflow() calls.
> struct iked_sa *
> config_new_sa(struct iked *env, int initiator)
> {
> @@ -584,122 +588,167 @@ config_setpolicy(struct iked *env, struc
> {
> struct iked_proposal *prop;
> struct iked_flow *flow;
> - struct iked_transform *xform;
> - size_t size, iovcnt, j, c = 0;
> struct iovec iov[IOV_MAX];
> + unsigned int i;
>
> - iovcnt = 1;
> - size = sizeof(*pol);
> - TAILQ_FOREACH(prop, &pol->pol_proposals, prop_entry) {
> - size += (prop->prop_nxforms * sizeof(*xform)) +
> - (sizeof(*prop));
> - iovcnt += prop->prop_nxforms + 1;
> + if (env->sc_opts & IKED_OPT_NOACTION) {
> + print_policy(pol);
> + return (0);
> }
>
> - size += pol->pol_nflows * sizeof(*flow);
> - iovcnt += pol->pol_nflows;
> -
> - if (iovcnt > IOV_MAX) {
> - log_warn("%s: too many proposals/flows", __func__);
> + if (proc_compose_imsg(env, id, IMSG_CFG_POLICY_BEGIN, -1,
> + pol, sizeof(*pol)) == -1)
> return (-1);
> - }
> -
> - iov[c].iov_base = pol;
> - iov[c++].iov_len = sizeof(*pol);
>
> TAILQ_FOREACH(prop, &pol->pol_proposals, prop_entry) {
> - iov[c].iov_base = prop;
> - iov[c++].iov_len = sizeof(*prop);
> -
> - for (j = 0; j < prop->prop_nxforms; j++) {
> - xform = prop->prop_xforms + j;
> + iov[0].iov_base = prop;
> + iov[0].iov_len = sizeof(*prop);
>
> - iov[c].iov_base = xform;
> - iov[c++].iov_len = sizeof(*xform);
> + for (i = 1; i <= prop->prop_nxforms; i++) {
> + if (i >= IOV_MAX) {
> + log_warn("%s: too many proposals", __func__);
> + return (-1);
> + }
> + iov[i].iov_base = prop->prop_xforms + (i - 1);
> + iov[i].iov_len = sizeof(struct iked_transform);
> }
> +
> + if (proc_composev_imsg(env, id, IMSG_CFG_POLICY_PROPOSAL, -1,
> + iov, i) == -1)
> + return (-1);
> }
>
> RB_FOREACH(flow, iked_flows, &pol->pol_flows) {
> - iov[c].iov_base = flow;
> - iov[c++].iov_len = sizeof(*flow);
> + if (proc_compose_imsg(env, id, IMSG_CFG_POLICY_FLOW, -1,
> + flow, sizeof(struct iked_flow)) == -1)
> + return (-1);
> }
>
> - if (env->sc_opts & IKED_OPT_NOACTION) {
> - print_policy(pol);
> - return (0);
> + if (proc_compose_imsg(env, id, IMSG_CFG_POLICY_COMMIT, -1,
> + NULL, 0) == -1)
> + return (-1);
> +
> + return (0);
> +}
> +
> +int
> +config_getpolicy_begin(struct iked *env, struct imsg *imsg)
> +{
> + u_int8_t *buf = (u_int8_t *)imsg->data;
> +
> + IMSG_SIZE_CHECK(imsg, newpol);
> + log_debug("%s: start receiving policy", __func__);
> +
> + if (newpol != NULL) {
> + log_warnx("%s: previous policy was not commited");
> + config_free_policy(env, newpol);
> }
instead of using the global "newpol" variable just see if a policy
with the same id is already there, eg.
if (policy_find(newpol->pol_id) != NULL) {
log_warnx("%s: policy already exists", __func__);
return (-1);
}
where policy_find() would go into policy.c and look like
struct policy *
policy_find(struct iked *env, u_int id)
{
struct policy *pol:
TAILQ_FOREACH(pol, &env->sc_policies, pol_entry)
if (pol->pol_id == id)
return (pol);
return (NULL);
Have a look at usr.sbin/relayd/config.c, the config_setproto() and
config_getproto() and config_getprotonode() follow a similar
approach. It even packs multiple protonodes into one imsg for speed
up - but this is probably not required in iked since relayd's relays
can have thousands to millions of protonodes (eg. URL filter entries).
More comments below.
>
> - if (proc_composev_imsg(env, id, IMSG_CFG_POLICY, -1,
> - iov, iovcnt) == -1)
> - return (-1);
> + if ((newpol = config_new_policy(NULL)) == NULL)
> + fatal("config_getpolicy_begin: new policy");
> +
> + memcpy(newpol, buf, sizeof(*newpol));
> +
> + TAILQ_INIT(&newpol->pol_proposals);
> + RB_INIT(&newpol->pol_flows);
> +
> + newpol->pol_nproposals = 0;
> + newpol->pol_nflows = 0;
>
> return (0);
> }
>
> int
> -config_getpolicy(struct iked *env, struct imsg *imsg)
> +config_getpolicy_proposal(struct iked *env, struct imsg *imsg)
> {
> - struct iked_policy *pol;
> + u_int8_t *buf = (u_int8_t *)imsg->data;
> + off_t offset = 0;
> struct iked_proposal pp, *prop;
> struct iked_transform xf, *xform;
> - struct iked_flow *flow;
> - off_t offset = 0;
> - u_int i, j;
> - u_int8_t *buf = (u_int8_t *)imsg->data;
> + u_int i;
>
> - IMSG_SIZE_CHECK(imsg, pol);
> - log_debug("%s: received policy", __func__);
> + IMSG_SIZE_CHECK(imsg, prop);
> + log_debug("%s: receiving proposal", __func__);
>
So in config_getproposal() I would lookup the parent policy by id, eg.
and set the new prop_polid value before in setpolicy or directly in
parse.y.
if ((pol = policy_find(prop->prop_polid)) == NULL) {
log_warnx("%s: policy does not exist", __func__);
return (-1);
}
> - if ((pol = config_new_policy(NULL)) == NULL)
> - fatal("config_getpolicy: new policy");
> + memcpy(&pp, buf + offset, sizeof(pp));
> + offset += sizeof(pp);
> + if (imsg->hdr.len < (pp.prop_nxforms * sizeof(xf)) + offset)
> + fatalx("bad length imsg received");
>
> - memcpy(pol, buf, sizeof(*pol));
> - offset += sizeof(*pol);
> + if ((prop = config_add_proposal(&newpol->pol_proposals,
> + pp.prop_id, pp.prop_protoid)) == NULL)
> + fatal("config_getpolicy: add proposal");
>
> - TAILQ_INIT(&pol->pol_proposals);
> - RB_INIT(&pol->pol_flows);
> + for (i = 0; i < pp.prop_nxforms; i++) {
> + memcpy(&xf, buf + offset, sizeof(xf));
> + offset += sizeof(xf);
>
> - for (i = 0; i < pol->pol_nproposals; i++) {
> - memcpy(&pp, buf + offset, sizeof(pp));
> - offset += sizeof(pp);
> -
> - if ((prop = config_add_proposal(&pol->pol_proposals,
> - pp.prop_id, pp.prop_protoid)) == NULL)
> - fatal("config_getpolicy: add proposal");
> -
> - for (j = 0; j < pp.prop_nxforms; j++) {
> - memcpy(&xf, buf + offset, sizeof(xf));
> - offset += sizeof(xf);
> -
> - if ((xform = config_add_transform(prop, xf.xform_type,
> - xf.xform_id, xf.xform_length,
> - xf.xform_keylength)) == NULL)
> - fatal("config_getpolicy: add transform");
> - }
> + if ((xform = config_add_transform(prop, xf.xform_type,
> + xf.xform_id, xf.xform_length,
> + xf.xform_keylength)) == NULL)
> + fatal("config_getpolicy: add transform");
> }
>
> - for (i = 0; i < pol->pol_nflows; i++) {
> - if ((flow = calloc(1, sizeof(*flow))) == NULL)
> - fatal("config_getpolicy: new flow");
> + newpol->pol_nproposals++;
> + return (0);
> +}
>
> - memcpy(flow, buf + offset, sizeof(*flow));
> - offset += sizeof(*flow);
> +int
> +config_getpolicy_flow(struct iked *env, struct imsg *imsg)
> +{
> + struct iked_flow *flow;
> + u_int8_t *buf = (u_int8_t *)imsg->data;
>
> - RB_INSERT(iked_flows, &pol->pol_flows, flow);
> + IMSG_SIZE_CHECK(imsg, flow);
> + log_debug("%s: receiving flow", __func__);
> +
same here
if ((pol = policy_find(flow->flow_polid)) == NULL) {
log_warnx("%s: policy does not exist", __func__);
return (-1);
}
> + if ((flow = calloc(1, sizeof(*flow))) == NULL)
> + fatal("config_getpolicy: new flow");
> +
> + memcpy(flow, buf, sizeof(*flow));
> + RB_INSERT(iked_flows, &newpol->pol_flows, flow);
> +
> + newpol->pol_nflows++;
> + return (0);
> +}
> +
> +int
> +config_getpolicy_commit(struct iked *env, struct imsg *imsg)
> +{
> + /* XXX no data used, how to cope? */
this function is not needed anymore, the policy is already added to
the list and the final compile imsg will do the final steps once.
> +
> + if (newpol == NULL) {
> + log_warnx("%s: no policy started", __func__);
> + goto fail;
> + }
> + if (newpol->pol_nproposals == 0) {
> + log_warnx("%s: no proposals in policy", __func__);
> + goto fail;
> + }
> + if (newpol->pol_nflows == 0) {
> + log_warnx("%s: no flows in policy", __func__);
> + goto fail;
> }
>
> - TAILQ_INSERT_TAIL(&env->sc_policies, pol, pol_entry);
> + TAILQ_INSERT_TAIL(&env->sc_policies, newpol, pol_entry);
>
> - if (pol->pol_flags & IKED_POLICY_DEFAULT) {
> + if (newpol->pol_flags & IKED_POLICY_DEFAULT) {
> /* Only one default policy, just free/unref the old one */
> if (env->sc_defaultcon != NULL)
> config_free_policy(env, env->sc_defaultcon);
> - env->sc_defaultcon = pol;
> + env->sc_defaultcon = newpol;
> }
>
> - print_policy(pol);
> -
> + print_policy(newpol);
> + newpol = NULL;
> return (0);
> +
> +fail:
> + if (newpol != NULL) {
> + config_free_policy(env, newpol);
> + newpol = NULL;
> + }
> + return (-1);
> }
>
> int
> Index: types.h
> ===================================================================
> RCS file: /cvs/src/sbin/iked/types.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 types.h
> --- types.h 5 May 2011 12:17:10 -0000 1.10
> +++ types.h 20 May 2011 09:48:06 -0000
> @@ -92,13 +92,16 @@ enum imsg_type {
> IMSG_UDP_SOCKET,
> IMSG_PFKEY_SOCKET,
> IMSG_IKE_MESSAGE,
> - IMSG_CFG_POLICY,
+ IMSG_CFG_POLICY,
+ IMSG_CFG_PROPOSAL,
+ IMSG_CFG_FLOW,
> IMSG_CFG_USER,
> IMSG_CERTREQ,
> IMSG_CERT,
> IMSG_CERTVALID,
> IMSG_CERTINVALID,
> - IMSG_AUTH
not this part...
> + IMSG_AUTH,
> + IMSG_CFG_POLICY_BEGIN,
> + IMSG_CFG_POLICY_PROPOSAL,
> + IMSG_CFG_POLICY_FLOW,
> + IMSG_CFG_POLICY_COMMIT
> };
>
> enum privsep_procid {
> Index: iked.h
> ===================================================================
> RCS file: /cvs/src/sbin/iked/iked.h,v
> retrieving revision 1.41
> diff -u -p -r1.41 iked.h
> --- iked.h 9 May 2011 11:15:18 -0000 1.41
> +++ iked.h 20 May 2011 09:51:26 -0000
> @@ -547,7 +547,10 @@ int config_setreset(struct iked *, u_in
> int config_getreset(struct iked *, struct imsg *);
> int config_setpolicy(struct iked *, struct iked_policy *,
> enum privsep_procid);
> -int config_getpolicy(struct iked *, struct imsg *);
> +int config_getpolicy_begin(struct iked *, struct imsg *);
> +int config_getpolicy_proposal(struct iked *, struct imsg *);
> +int config_getpolicy_flow(struct iked *, struct imsg *);
> +int config_getpolicy_commit(struct iked *, struct imsg *);
accordingly...
> int config_setsocket(struct iked *, struct sockaddr_storage *, in_port_t,
> enum privsep_procid);
> int config_getsocket(struct iked *env, struct imsg *,
