On Wed, Jun 21, 2017 at 09:46:44AM +0200, Patrick Wildt wrote:
> On Wed, May 31, 2017 at 02:23:12PM +0200, Patrick Wildt wrote:
> > Hi,
> >
> > I'd like to add MOBIKE support to iked, specifically first of all making
> > iked as server react to mobile clients changing their IP addresses. One
> > thing for that is the kernel part.
> >
> > Having MOBIKE means that we need to be able to change the destination
> > address in an existing SA so that the already established SA points to
> > the correct client's IP address. Now we cannot call SADB_UPDATE with
> > the new DST address in SADB_EXT_ADDRESS_DST since that attribute is
> > used to find and retrieve the SA. For this we need a different attr.
> > Since PROXY is unused we can make re-use it. Maybe it might make sense
> > to rename it to something else, but I won't propose it as part of this
> > diff. In the case of our server changing the IP, we would be able to
> > use the SADB_EXT_ADDRESS_SRC attribute for that.
> >
> > Since we are changing a valid SA, we need to remove it from the tree,
> > modify it, and then put it back in. We do not yet have that functio-
> > nality, we can only free them. For this, split a part of tdb_delete()
> > into a new function tdb_unlink() which is the equivalent of puttdb(),
> > but the other way around.
> >
> > Opinions?
> >
> > Patrick
> >
> > diff --git a/sys/net/pfkeyv2.c b/sys/net/pfkeyv2.c
> > index 5acb747f9f3..a50adba7431 100644
> > --- a/sys/net/pfkeyv2.c
> > +++ b/sys/net/pfkeyv2.c
> > @@ -1214,6 +1214,15 @@ pfkeyv2_send(struct socket *socket, void *message,
> > int len)
> > import_tag(sa2, headers[SADB_X_EXT_TAG]);
> > import_tap(sa2, headers[SADB_X_EXT_TAP]);
> > #endif
> > + if (headers[SADB_EXT_ADDRESS_SRC] ||
> > + headers[SADB_EXT_ADDRESS_PROXY]) {
> > + tdb_unlink(sa2);
> > + import_address((struct sockaddr *)&sa2->tdb_src,
> > + headers[SADB_EXT_ADDRESS_SRC]);
> > + import_address((struct sockaddr *)&sa2->tdb_dst,
> > + headers[SADB_EXT_ADDRESS_PROXY]);
> > + puttdb(sa2);
> > + }
> > }
> >
> > break;
> > diff --git a/sys/net/pfkeyv2_parsemessage.c b/sys/net/pfkeyv2_parsemessage.c
> > index 547532fa7b4..3a8b48441da 100644
> > --- a/sys/net/pfkeyv2_parsemessage.c
> > +++ b/sys/net/pfkeyv2_parsemessage.c
> > @@ -96,6 +96,7 @@
> > #define BITMAP_LIFETIME_SOFT (1LL << SADB_EXT_LIFETIME_SOFT)
> > #define BITMAP_ADDRESS_SRC (1LL << SADB_EXT_ADDRESS_SRC)
> > #define BITMAP_ADDRESS_DST (1LL << SADB_EXT_ADDRESS_DST)
> > +#define BITMAP_ADDRESS_PROXY (1LL << SADB_EXT_ADDRESS_PROXY)
> > #define BITMAP_KEY_AUTH (1LL << SADB_EXT_KEY_AUTH)
> > #define BITMAP_KEY_ENCRYPT (1LL << SADB_EXT_KEY_ENCRYPT)
> > #define BITMAP_IDENTITY_SRC (1LL << SADB_EXT_IDENTITY_SRC)
> > @@ -134,7 +135,7 @@ uint64_t sadb_exts_allowed_in[SADB_MAX+1] =
> > /* GETSPI */
> > BITMAP_ADDRESS_SRC | BITMAP_ADDRESS_DST | BITMAP_SPIRANGE,
> > /* UPDATE */
> > - BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_KEY |
> > BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG |
> > BITMAP_X_TAP,
> > + BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_ADDRESS_PROXY |
> > BITMAP_KEY | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP |
> > BITMAP_X_TAG | BITMAP_X_TAP,
> > /* ADD */
> > BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_KEY |
> > BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP |
> > BITMAP_X_LIFETIME_LASTUSE | BITMAP_X_TAG | BITMAP_X_TAP,
> > /* DELETE */
> > @@ -206,7 +207,7 @@ uint64_t sadb_exts_allowed_out[SADB_MAX+1] =
> > /* GETSPI */
> > BITMAP_SA | BITMAP_ADDRESS_SRC | BITMAP_ADDRESS_DST,
> > /* UPDATE */
> > - BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_IDENTITY |
> > BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG | BITMAP_X_TAP,
> > + BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_ADDRESS_PROXY |
> > BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG |
> > BITMAP_X_TAP,
> > /* ADD */
> > BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_IDENTITY |
> > BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG | BITMAP_X_TAP,
> > /* DELETE */
> > @@ -463,6 +464,7 @@ pfkeyv2_parsemessage(void *p, int len, void **headers)
> > break;
> > case SADB_EXT_ADDRESS_SRC:
> > case SADB_EXT_ADDRESS_DST:
> > + case SADB_EXT_ADDRESS_PROXY:
> > case SADB_X_EXT_SRC_MASK:
> > case SADB_X_EXT_DST_MASK:
> > case SADB_X_EXT_SRC_FLOW:
> > diff --git a/sys/netinet/ip_ipsp.c b/sys/netinet/ip_ipsp.c
> > index 39daa184c51..89f9fb89164 100644
> > --- a/sys/netinet/ip_ipsp.c
> > +++ b/sys/netinet/ip_ipsp.c
> > @@ -714,7 +714,7 @@ puttdb(struct tdb *tdbp)
> > }
> >
> > void
> > -tdb_delete(struct tdb *tdbp)
> > +tdb_unlink(struct tdb *tdbp)
> > {
> > struct tdb *tdbpp;
> > u_int32_t hashval;
> > @@ -775,10 +775,18 @@ tdb_delete(struct tdb *tdbp)
> > }
> >
> > tdbp->tdb_snext = NULL;
> > - tdb_free(tdbp);
> > tdb_count--;
> > }
> >
> > +void
> > +tdb_delete(struct tdb *tdbp)
> > +{
> > + NET_ASSERT_LOCKED();
> > +
> > + tdb_unlink(tdbp);
> > + tdb_free(tdbp);
> > +}
> > +
> > /*
> > * Allocate a TDB and initialize a few basic fields.
> > */
> > diff --git a/sys/netinet/ip_ipsp.h b/sys/netinet/ip_ipsp.h
> > index 43bf0f8b66a..d7bbacc46c1 100644
> > --- a/sys/netinet/ip_ipsp.h
> > +++ b/sys/netinet/ip_ipsp.h
> > @@ -468,6 +468,7 @@ void tdb_delete(struct tdb *);
> > struct tdb *tdb_alloc(u_int);
> > void tdb_free(struct tdb *);
> > int tdb_init(struct tdb *, u_int16_t, struct ipsecinit *);
> > +void tdb_unlink(struct tdb *);
> > int tdb_walk(u_int, int (*)(struct tdb *, void *, int), void *);
> >
> > /* XF_IP4 */
>
> Anyone?
No objections from my side. The tdb_unlink/tdb_free change is OK. I think
SADB_EXT_ADDRESS_PROXY should be renamed maybe to _MOB_DST or similar but
that is bikeshedding and can happen in the tree later.
The protocol handling seems sensible but honestly pfkey is a beast.
OK claudio@
--
:wq Claudio
