Re: [4/4] [IPSEC] Store MTU at each xfrm_dst

[Patrick McHardy <kaber@xxxxxxxxx>]

Hi Herbert,

Herbert Xu wrote:
> Finally this is the patch that sets the MTU values of each xfrm_dst
> and keeps it up-to-date.
while looking for ways to exchange the xfrm pointer of a live
dst_entry I noticed what looks like an ABBA deadlock introduced
by the call to xfrm_state_mtu() in xfrm_bundle_ok().

spin_lock x->lock                    (xfrm_state_delete)
        read_lock xfrm_policy_lock      (xfrm_prune_bundles)
                write_lock policy->lock      (xfrm_prune_bundles)

read/write_lock policy->lock         (__xfrm46_find_acq/xfrm_lookup)
        spin_lock x->lock            (xfrm_state_mtu)

I haven't though of a way to avoid this yet. It would be nice though
if we could keep the rule that xfrm_policy_lock and policy->lock nest
with x->lock.

Something unrelated I was also wondering about, from xfrm_find_state():

        list_for_each_entry(x, xfrm_state_bydst+h, bydst) {
                if (x->props.family == family &&
                    x->props.reqid == tmpl->reqid &&
                    xfrm_state_addr_check(x, daddr, saddr, family) &&
                    tmpl->mode == x->props.mode &&
                    tmpl->id.proto == x->id.proto) {

Shouldn't we check for (tmpl->id.spi == x->id.spi || !tmpl->id.spi) ?

Regards
Patrick

>   
> -int xfrm_bundle_ok(struct xfrm_dst *xdst, struct flowi *fl, int family)
> +int xfrm_bundle_ok(struct xfrm_dst *first, struct flowi *fl, int family)
>  {
> -       struct dst_entry *dst = &xdst->u.dst;
> +       struct dst_entry *dst = &first->u.dst;
> +       struct xfrm_dst *last;
> +       u32 mtu;
>  
>  	if (dst->path->obsolete > 0 ||
>             (dst->dev && !netif_running(dst->dev)))
>                 return 0;
>  
> +	last = NULL;
> +
>         do {
> +               struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
> +
>                 if (fl && !xfrm_selector_match(&dst->xfrm->sel, fl, family))
>                         return 0;
>                 if (dst->xfrm->km.state != XFRM_STATE_VALID)
>                         return 0;
> +
> +               mtu = dst_pmtu(dst->child);
> +               if (xdst->child_mtu_cached != mtu) {
> +                       last = xdst;
> +                       xdst->child_mtu_cached = mtu;
> +               }
> +
> +               mtu = dst_pmtu(xdst->route);
> +               if (xdst->child_mtu_cached != mtu) {
> +                       last = xdst;
> +                       xdst->route_mtu_cached = mtu;
> +               }
> +
>                 dst = dst->child;
>         } while (dst->xfrm);
> +
> +       if (likely(!last))
> +               return 1;
> +
> +       mtu = last->child_mtu_cached;
> +       for (;;) {
> +               dst = &last->u.dst;
> +
> +               mtu = xfrm_state_mtu(dst->xfrm, mtu);
> +               if (mtu > last->route_mtu_cached)
> +                       mtu = last->route_mtu_cached;
> +               dst->metrics[RTAX_MTU-1] = mtu;
> +
> +               if (last == first)
> +                       break;
> +
> +               last = last->u.next;
> +               last->child_mtu_cached = mtu;
> +       }
>  
>  	return 1;
>  }