On Wed, 2017-11-01 at 00:08 +0100, Florian Westphal wrote: > Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley <sds@xxxxxxxxxxxx > > v> wrote: > > > matching before (as in this patch) or after calling > > > xfrm_bundle_ok()? > > > > I would probably make the LSM call the last check, as you've done; > > but > > I have to say that is just so it is consistent with the "LSM last" > > philosophy and not because of any performance related argument. > > > > > ... Also, > > > do we need to test xfrm->sel.family before calling > > > xfrm_selector_match > > > (as in this patch) or not - xfrm_state_look_at() does so when the > > > state is XFRM_STATE_VALID but not when it is _ERROR or _EXPIRED? > > > > Speaking purely from a SELinux perspective, I'm not sure it > > matters: > > as long as the labels match we are happy. However, from a general > > IPsec perspective it does seem like a reasonable thing. > > > > Granted I'm probably missing something, but it seems a little odd > > that > > the code isn't already checking that the selectors match (... what > > am > > I missing?). It does check the policies, maybe that is enough in > > the > > normal IPsec case? > > The assumption was that identical policies would yield the same SAs, > but thats not correct. > > > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > > > index 2746b62..171818b 100644 > > > --- a/net/xfrm/xfrm_policy.c > > > +++ b/net/xfrm/xfrm_policy.c > > > @@ -1820,6 +1820,11 @@ xfrm_resolve_and_create_bundle(struct > > > xfrm_policy **pols, int num_pols, > > > !xfrm_pol_dead(xdst) && > > > memcmp(xdst->pols, pols, > > > sizeof(struct xfrm_policy *) * num_pols) == 0 > > > && > > > + (!xdst->u.dst.xfrm->sel.family || > > > + xfrm_selector_match(&xdst->u.dst.xfrm->sel, fl, > > > + xdst->u.dst.xfrm->sel.family)) > > > && > > > + security_xfrm_state_pol_flow_match(xdst->u.dst.xfrm, > > > + xdst->pols[0], fl) > > > && > > ... so this needs to walk the bundle and validate each selector. > > Alternatively we could always do template resolution and then check > that all states found match those of the old pcpu xdst: With your patch below, the selinux-testsuite passes, and I couldn't trigger any failures even running the inet_socket tests repeatedly. Tested-by: Stephen Smalley <sds@xxxxxxxxxxxxx> > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -1786,19 +1786,23 @@ void xfrm_policy_cache_flush(void) > put_online_cpus(); > } > > -static bool xfrm_pol_dead(struct xfrm_dst *xdst) > +static bool xfrm_xdst_can_reuse(struct xfrm_dst *xdst, > + struct xfrm_state * const xfrm[], > + int num) > { > - unsigned int num_pols = xdst->num_pols; > - unsigned int pol_dead = 0, i; > + const struct dst_entry *dst = &xdst->u.dst; > + int i; > > - for (i = 0; i < num_pols; i++) > - pol_dead |= xdst->pols[i]->walk.dead; > + if (xdst->num_xfrms != num) > + return false; > > - /* Mark DST_OBSOLETE_DEAD to fail the next xfrm_dst_check() > */ > - if (pol_dead) > - xdst->u.dst.obsolete = DST_OBSOLETE_DEAD; > + for (i = 0; i < num; i++) { > + if (!dst || dst->xfrm != xfrm[i]) > + return false; > + dst = dst->child; > + } > > - return pol_dead; > + return xfrm_bundle_ok(xdst); > } > > static struct xfrm_dst * > @@ -1812,26 +1816,28 @@ xfrm_resolve_and_create_bundle(struct > xfrm_policy **pols, int num_pols, > struct dst_entry *dst; > int err; > > + /* Try to instantiate a bundle */ > + err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family); > + if (err <= 0) { > + if (err != 0 && err != -EAGAIN) > + XFRM_INC_STATS(net, > LINUX_MIB_XFRMOUTPOLERROR); > + return ERR_PTR(err); > + } > + > xdst = this_cpu_read(xfrm_last_dst); > if (xdst && > xdst->u.dst.dev == dst_orig->dev && > xdst->num_pols == num_pols && > - !xfrm_pol_dead(xdst) && > memcmp(xdst->pols, pols, > sizeof(struct xfrm_policy *) * num_pols) == 0 && > - xfrm_bundle_ok(xdst)) { > + xfrm_xdst_can_reuse(xdst, xfrm, err)) { > dst_hold(&xdst->u.dst); > + while (err > 0) > + xfrm_state_put(xfrm[--err]); > return xdst; > } > > old = xdst; > - /* Try to instantiate a bundle */ > - err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family); > - if (err <= 0) { > - if (err != 0 && err != -EAGAIN) > - XFRM_INC_STATS(net, > LINUX_MIB_XFRMOUTPOLERROR); > - return ERR_PTR(err); > - } > > dst = xfrm_bundle_create(pols[0], xfrm, err, fl, dst_orig); > if (IS_ERR(dst)) {