All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC net-next] net: xfrm: support setting an output mark.
@ 2017-08-07  9:23 Lorenzo Colitti
  2017-08-07 11:16 ` Steffen Klassert
  0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Colitti @ 2017-08-07  9:23 UTC (permalink / raw)
  To: netdev
  Cc: davem, jhs, steffen.klassert, herbert, nharold, misterikkit,
	Lorenzo Colitti

On systems that use mark-based routing it may be necessary for
routing lookups to use marks in order for packets to be routed
correctly. An example of such a system is Android, which uses
socket marks to route packets via different networks.

Currently, routing lookups in tunnel mode always use a mark of
zero, making routing incorrect on such systems. This patch adds
a new output_mark parameter to the SA properties.  The mark will
be used in routing lookups, and if nonzero, will also set the
mark of the packets emitted by the SA, so it can be matched by
iptables.

Tested: https://android-review.googlesource.com/452776
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 include/net/xfrm.h        |  9 ++++++---
 include/uapi/linux/xfrm.h |  1 +
 net/ipv4/xfrm4_policy.c   | 14 +++++++++-----
 net/ipv6/xfrm6_policy.c   |  9 ++++++---
 net/xfrm/xfrm_output.c    |  3 +++
 net/xfrm/xfrm_policy.c    | 17 +++++++++--------
 net/xfrm/xfrm_user.c      | 11 +++++++++++
 7 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index afb4929d72..6b1ddb790d 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -163,6 +163,7 @@ struct xfrm_state {
 		int		header_len;
 		int		trailer_len;
 		u32		extra_flags;
+		u32		output_mark;
 	} props;
 
 	struct xfrm_lifetime_cfg lft;
@@ -296,10 +297,12 @@ struct xfrm_policy_afinfo {
 	struct dst_entry	*(*dst_lookup)(struct net *net,
 					       int tos, int oif,
 					       const xfrm_address_t *saddr,
-					       const xfrm_address_t *daddr);
+					       const xfrm_address_t *daddr,
+					       u32 mark);
 	int			(*get_saddr)(struct net *net, int oif,
 					     xfrm_address_t *saddr,
-					     xfrm_address_t *daddr);
+					     xfrm_address_t *daddr,
+					     u32 mark);
 	void			(*decode_session)(struct sk_buff *skb,
 						  struct flowi *fl,
 						  int reverse);
@@ -1638,7 +1641,7 @@ static inline int xfrm4_udp_encap_rcv(struct sock *sk, struct sk_buff *skb)
 struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, int oif,
 				    const xfrm_address_t *saddr,
 				    const xfrm_address_t *daddr,
-				    int family);
+				    int family, u32 mark);
 
 struct xfrm_policy *xfrm_policy_alloc(struct net *net, gfp_t gfp);
 
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 2b384ff09f..5fe7370a2b 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -304,6 +304,7 @@ enum xfrm_attr_type_t {
 	XFRMA_ADDRESS_FILTER,	/* struct xfrm_address_filter */
 	XFRMA_PAD,
 	XFRMA_OFFLOAD_DEV,	/* struct xfrm_state_offload */
+	XFRMA_OUTPUT_MARK,	/* __u32 */
 	__XFRMA_MAX
 
 #define XFRMA_MAX (__XFRMA_MAX - 1)
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 4aefb149fe..d7bf0b0418 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -20,7 +20,8 @@
 static struct dst_entry *__xfrm4_dst_lookup(struct net *net, struct flowi4 *fl4,
 					    int tos, int oif,
 					    const xfrm_address_t *saddr,
-					    const xfrm_address_t *daddr)
+					    const xfrm_address_t *daddr,
+					    u32 mark)
 {
 	struct rtable *rt;
 
@@ -28,6 +29,7 @@ static struct dst_entry *__xfrm4_dst_lookup(struct net *net, struct flowi4 *fl4,
 	fl4->daddr = daddr->a4;
 	fl4->flowi4_tos = tos;
 	fl4->flowi4_oif = l3mdev_master_ifindex_by_index(net, oif);
+	fl4->flowi4_mark = mark;
 	if (saddr)
 		fl4->saddr = saddr->a4;
 
@@ -42,20 +44,22 @@ static struct dst_entry *__xfrm4_dst_lookup(struct net *net, struct flowi4 *fl4,
 
 static struct dst_entry *xfrm4_dst_lookup(struct net *net, int tos, int oif,
 					  const xfrm_address_t *saddr,
-					  const xfrm_address_t *daddr)
+					  const xfrm_address_t *daddr,
+					  u32 mark)
 {
 	struct flowi4 fl4;
 
-	return __xfrm4_dst_lookup(net, &fl4, tos, oif, saddr, daddr);
+	return __xfrm4_dst_lookup(net, &fl4, tos, oif, saddr, daddr, mark);
 }
 
 static int xfrm4_get_saddr(struct net *net, int oif,
-			   xfrm_address_t *saddr, xfrm_address_t *daddr)
+			   xfrm_address_t *saddr, xfrm_address_t *daddr,
+			   u32 mark)
 {
 	struct dst_entry *dst;
 	struct flowi4 fl4;
 
-	dst = __xfrm4_dst_lookup(net, &fl4, 0, oif, NULL, daddr);
+	dst = __xfrm4_dst_lookup(net, &fl4, 0, oif, NULL, daddr, mark);
 	if (IS_ERR(dst))
 		return -EHOSTUNREACH;
 
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index f44b25a484..11d1314ab6 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -27,7 +27,8 @@
 
 static struct dst_entry *xfrm6_dst_lookup(struct net *net, int tos, int oif,
 					  const xfrm_address_t *saddr,
-					  const xfrm_address_t *daddr)
+					  const xfrm_address_t *daddr,
+					  u32 mark)
 {
 	struct flowi6 fl6;
 	struct dst_entry *dst;
@@ -36,6 +37,7 @@ static struct dst_entry *xfrm6_dst_lookup(struct net *net, int tos, int oif,
 	memset(&fl6, 0, sizeof(fl6));
 	fl6.flowi6_oif = l3mdev_master_ifindex_by_index(net, oif);
 	fl6.flowi6_flags = FLOWI_FLAG_SKIP_NH_OIF;
+	fl6.flowi6_mark = mark;
 	memcpy(&fl6.daddr, daddr, sizeof(fl6.daddr));
 	if (saddr)
 		memcpy(&fl6.saddr, saddr, sizeof(fl6.saddr));
@@ -52,12 +54,13 @@ static struct dst_entry *xfrm6_dst_lookup(struct net *net, int tos, int oif,
 }
 
 static int xfrm6_get_saddr(struct net *net, int oif,
-			   xfrm_address_t *saddr, xfrm_address_t *daddr)
+			   xfrm_address_t *saddr, xfrm_address_t *daddr,
+			   u32 mark)
 {
 	struct dst_entry *dst;
 	struct net_device *dev;
 
-	dst = xfrm6_dst_lookup(net, 0, oif, NULL, daddr);
+	dst = xfrm6_dst_lookup(net, 0, oif, NULL, daddr, mark);
 	if (IS_ERR(dst))
 		return -EHOSTUNREACH;
 
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 8c0b6722aa..31a2e6d34d 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -66,6 +66,9 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
 			goto error_nolock;
 		}
 
+		if (x->props.output_mark)
+			skb->mark = x->props.output_mark;
+
 		err = x->outer_mode->output(x, skb);
 		if (err) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTSTATEMODEERROR);
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 06c3bf7ab8..1de52f36ca 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -122,7 +122,7 @@ static const struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short fa
 struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, int oif,
 				    const xfrm_address_t *saddr,
 				    const xfrm_address_t *daddr,
-				    int family)
+				    int family, u32 mark)
 {
 	const struct xfrm_policy_afinfo *afinfo;
 	struct dst_entry *dst;
@@ -131,7 +131,7 @@ struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, int oif,
 	if (unlikely(afinfo == NULL))
 		return ERR_PTR(-EAFNOSUPPORT);
 
-	dst = afinfo->dst_lookup(net, tos, oif, saddr, daddr);
+	dst = afinfo->dst_lookup(net, tos, oif, saddr, daddr, mark);
 
 	rcu_read_unlock();
 
@@ -143,7 +143,7 @@ static inline struct dst_entry *xfrm_dst_lookup(struct xfrm_state *x,
 						int tos, int oif,
 						xfrm_address_t *prev_saddr,
 						xfrm_address_t *prev_daddr,
-						int family)
+						int family, u32 mark)
 {
 	struct net *net = xs_net(x);
 	xfrm_address_t *saddr = &x->props.saddr;
@@ -159,7 +159,7 @@ static inline struct dst_entry *xfrm_dst_lookup(struct xfrm_state *x,
 		daddr = x->coaddr;
 	}
 
-	dst = __xfrm_dst_lookup(net, tos, oif, saddr, daddr, family);
+	dst = __xfrm_dst_lookup(net, tos, oif, saddr, daddr, family, mark);
 
 	if (!IS_ERR(dst)) {
 		if (prev_saddr != saddr)
@@ -1340,14 +1340,14 @@ int __xfrm_sk_clone_policy(struct sock *sk, const struct sock *osk)
 
 static int
 xfrm_get_saddr(struct net *net, int oif, xfrm_address_t *local,
-	       xfrm_address_t *remote, unsigned short family)
+	       xfrm_address_t *remote, unsigned short family, u32 mark)
 {
 	int err;
 	const struct xfrm_policy_afinfo *afinfo = xfrm_policy_get_afinfo(family);
 
 	if (unlikely(afinfo == NULL))
 		return -EINVAL;
-	err = afinfo->get_saddr(net, oif, local, remote);
+	err = afinfo->get_saddr(net, oif, local, remote, mark);
 	rcu_read_unlock();
 	return err;
 }
@@ -1378,7 +1378,7 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
 			if (xfrm_addr_any(local, tmpl->encap_family)) {
 				error = xfrm_get_saddr(net, fl->flowi_oif,
 						       &tmp, remote,
-						       tmpl->encap_family);
+						       tmpl->encap_family, 0);
 				if (error)
 					goto fail;
 				local = &tmp;
@@ -1598,7 +1598,8 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 		if (xfrm[i]->props.mode != XFRM_MODE_TRANSPORT) {
 			family = xfrm[i]->props.family;
 			dst = xfrm_dst_lookup(xfrm[i], tos, fl->flowi_oif,
-					      &saddr, &daddr, family);
+					      &saddr, &daddr, family,
+					      xfrm[i]->props.output_mark);
 			err = PTR_ERR(dst);
 			if (IS_ERR(dst))
 				goto put_states;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 1b539b7dcf..15dd0023ed 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -584,6 +584,9 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 
 	xfrm_mark_get(attrs, &x->mark);
 
+	if (attrs[XFRMA_OUTPUT_MARK])
+		x->props.output_mark = nla_get_u32(attrs[XFRMA_OUTPUT_MARK]);
+
 	err = __xfrm_init_state(x, false);
 	if (err)
 		goto error;
@@ -899,6 +902,11 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 		goto out;
 	if (x->security)
 		ret = copy_sec_ctx(x->security, skb);
+	if (x->props.output_mark) {
+		ret = nla_put_u32(skb, XFRMA_OUTPUT_MARK, x->props.output_mark);
+		if (ret)
+			goto out;
+	}
 out:
 	return ret;
 }
@@ -2454,6 +2462,7 @@ static const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
 	[XFRMA_PROTO]		= { .type = NLA_U8 },
 	[XFRMA_ADDRESS_FILTER]	= { .len = sizeof(struct xfrm_address_filter) },
 	[XFRMA_OFFLOAD_DEV]	= { .len = sizeof(struct xfrm_user_offload) },
+	[XFRMA_OUTPUT_MARK]	= { .len = NLA_U32 },
 };
 
 static const struct nla_policy xfrma_spd_policy[XFRMA_SPD_MAX+1] = {
@@ -2673,6 +2682,8 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x)
 		l += nla_total_size(sizeof(x->props.extra_flags));
 	if (x->xso.dev)
 		 l += nla_total_size(sizeof(x->xso));
+	if (x->props.output_mark)
+		l += nla_total_size(sizeof(x->props.output_mark));
 
 	/* Must count x->lastused as it may become non-zero behind our back. */
 	l += nla_total_size_64bit(sizeof(u64));
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFC net-next] net: xfrm: support setting an output mark.
  2017-08-07  9:23 [RFC net-next] net: xfrm: support setting an output mark Lorenzo Colitti
@ 2017-08-07 11:16 ` Steffen Klassert
  2017-08-07 13:34   ` Lorenzo Colitti
  0 siblings, 1 reply; 6+ messages in thread
From: Steffen Klassert @ 2017-08-07 11:16 UTC (permalink / raw)
  To: Lorenzo Colitti; +Cc: netdev, davem, jhs, herbert, nharold, misterikkit

On Mon, Aug 07, 2017 at 06:23:26PM +0900, Lorenzo Colitti wrote:
> On systems that use mark-based routing it may be necessary for
> routing lookups to use marks in order for packets to be routed
> correctly. An example of such a system is Android, which uses
> socket marks to route packets via different networks.
> 
> Currently, routing lookups in tunnel mode always use a mark of
> zero, making routing incorrect on such systems. This patch adds
> a new output_mark parameter to the SA properties.  The mark will
> be used in routing lookups, and if nonzero, will also set the
> mark of the packets emitted by the SA, so it can be matched by
> iptables.
> 
> Tested: https://android-review.googlesource.com/452776
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> ---
>  include/net/xfrm.h        |  9 ++++++---
>  include/uapi/linux/xfrm.h |  1 +
>  net/ipv4/xfrm4_policy.c   | 14 +++++++++-----
>  net/ipv6/xfrm6_policy.c   |  9 ++++++---
>  net/xfrm/xfrm_output.c    |  3 +++
>  net/xfrm/xfrm_policy.c    | 17 +++++++++--------
>  net/xfrm/xfrm_user.c      | 11 +++++++++++
>  7 files changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index afb4929d72..6b1ddb790d 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -163,6 +163,7 @@ struct xfrm_state {
>  		int		header_len;
>  		int		trailer_len;
>  		u32		extra_flags;
> +		u32		output_mark;
>  	} props;
>  
>  	struct xfrm_lifetime_cfg lft;
> @@ -296,10 +297,12 @@ struct xfrm_policy_afinfo {
>  	struct dst_entry	*(*dst_lookup)(struct net *net,
>  					       int tos, int oif,
>  					       const xfrm_address_t *saddr,
> -					       const xfrm_address_t *daddr);
> +					       const xfrm_address_t *daddr,
> +					       u32 mark);
>  	int			(*get_saddr)(struct net *net, int oif,
>  					     xfrm_address_t *saddr,
> -					     xfrm_address_t *daddr);
> +					     xfrm_address_t *daddr,
> +					     u32 mark);
>  	void			(*decode_session)(struct sk_buff *skb,
>  						  struct flowi *fl,
>  						  int reverse);
> @@ -1638,7 +1641,7 @@ static inline int xfrm4_udp_encap_rcv(struct sock *sk, struct sk_buff *skb)
>  struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, int oif,
>  				    const xfrm_address_t *saddr,
>  				    const xfrm_address_t *daddr,
> -				    int family);
> +				    int family, u32 mark);
>  
>  struct xfrm_policy *xfrm_policy_alloc(struct net *net, gfp_t gfp);
>  
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index 2b384ff09f..5fe7370a2b 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -304,6 +304,7 @@ enum xfrm_attr_type_t {
>  	XFRMA_ADDRESS_FILTER,	/* struct xfrm_address_filter */
>  	XFRMA_PAD,
>  	XFRMA_OFFLOAD_DEV,	/* struct xfrm_state_offload */
> +	XFRMA_OUTPUT_MARK,	/* __u32 */
>  	__XFRMA_MAX

Hm, why don't you use the existing xfrm_mark for this?
Having two different marks on one SA seems to be a bit odd.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC net-next] net: xfrm: support setting an output mark.
  2017-08-07 11:16 ` Steffen Klassert
@ 2017-08-07 13:34   ` Lorenzo Colitti
  2017-08-08  7:51     ` Steffen Klassert
  0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Colitti @ 2017-08-07 13:34 UTC (permalink / raw)
  To: Steffen Klassert
  Cc: netdev, David Miller, Jamal Hadi Salim, herbert, Nathan Harold,
	Jonathan Basseri

On Mon, Aug 7, 2017 at 8:16 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> >       XFRMA_OFFLOAD_DEV,      /* struct xfrm_state_offload */
> > +     XFRMA_OUTPUT_MARK,      /* __u32 */
> >       __XFRMA_MAX
>
> Hm, why don't you use the existing xfrm_mark for this?
> Having two different marks on one SA seems to be a bit odd.

I think using a separate mark is conceptually cleaner and much more flexible.

The existing xfrm_mark is constrained to match the mark of the socket
that sends the packet (or the unencrypted packet that is transformed).
Specifically (at least if I'm reading the code correctly),
xfrm_policy_match requires:

    (fl->flowi_mark & pol->mark.m) == pol->mark.v

and xfrm_state_find requires:

    (mark & x->mark.m) == x->mark.v

so basically the SA mark is a subset of the bits of the policy mark,
which is a subset of the bits of sk_mark.

On systems using mark-based routing (or at least on Android [1]), the
mark decides how a packet is routed, and in order for things to work
properly the routing of an IPsec tunnel should depend only on the
tunnel configuration and not on the mark of any socket or any packet
that is routed through that tunnel. The mark might select which tunnel
to go through, and the output mark would decide how the tunnel packets
are routed.

A real use case is IWLAN [2], which is used to connect to 3GPP
networks over wifi. The phone has the same IP addresses on the
cellular interface and on an IPsec tunnel to a carrier gateway (PDG).
On Android, a socket connected on the carrier network through a native
interface such as LTE will have the socket mark that's specific to the
carrier network. If the device then hands over the carrier connection
to the IPsec tunnel over wifi, the socket will still have the mark of
the LTE interface. If we used xfrm_mark, then the tunneled packets
themselves will be routed on LTE instead of being routed on wifi. That
obviously defeats the purpose.

Using XFRM_OUTPUT_MARK makes this much cleaner: the socket can
continue to be marked with the carrier network's mark, and it's the
IPsec tunnel itself that is bound to the wifi network using the mark
in the SA.

Another use case is if there are multiple SAs that apply to a given
socket/packet (e.g., a transport mode SA for an individual TCP
connection, wrapped in a tunnel mode SA that applies to a whole remote
site or network). This is required when running SIP over IWLAN (i.e.,
"wifi calling") - the SIP connection is always encrypted in transport
mode regardless of whether the carrier connection is native or
tunneled over wifi, and when on wifi, the mark that is used to route
the packet must be the tunnel mode SA, not the socket mark or the
transport mode SA mark.


[1] http://www.netdevconf.org/1.1/proceedings/slides/colitti-kline-linux-networking-android-devices.pdf
[2] http://www.etsi.org/deliver/etsi_ts/124300_124399/124327/12.00.00_60/ts_124327v120000p.pdf

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC net-next] net: xfrm: support setting an output mark.
  2017-08-07 13:34   ` Lorenzo Colitti
@ 2017-08-08  7:51     ` Steffen Klassert
  2017-08-09 17:13       ` Lorenzo Colitti
  0 siblings, 1 reply; 6+ messages in thread
From: Steffen Klassert @ 2017-08-08  7:51 UTC (permalink / raw)
  To: Lorenzo Colitti
  Cc: netdev, David Miller, Jamal Hadi Salim, herbert, Nathan Harold,
	Jonathan Basseri

On Mon, Aug 07, 2017 at 10:34:37PM +0900, Lorenzo Colitti wrote:
> On Mon, Aug 7, 2017 at 8:16 PM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> > >       XFRMA_OFFLOAD_DEV,      /* struct xfrm_state_offload */
> > > +     XFRMA_OUTPUT_MARK,      /* __u32 */
> > >       __XFRMA_MAX
> >
> > Hm, why don't you use the existing xfrm_mark for this?
> > Having two different marks on one SA seems to be a bit odd.
> 
> I think using a separate mark is conceptually cleaner and much more flexible.
> 
> The existing xfrm_mark is constrained to match the mark of the socket
> that sends the packet (or the unencrypted packet that is transformed).
> Specifically (at least if I'm reading the code correctly),
> xfrm_policy_match requires:
> 
>     (fl->flowi_mark & pol->mark.m) == pol->mark.v
> 
> and xfrm_state_find requires:
> 
>     (mark & x->mark.m) == x->mark.v
> 
> so basically the SA mark is a subset of the bits of the policy mark,
> which is a subset of the bits of sk_mark.
> 
> On systems using mark-based routing (or at least on Android [1]), the
> mark decides how a packet is routed, and in order for things to work
> properly the routing of an IPsec tunnel should depend only on the
> tunnel configuration and not on the mark of any socket or any packet
> that is routed through that tunnel. The mark might select which tunnel
> to go through, and the output mark would decide how the tunnel packets
> are routed.

I thought you can just split the 32 bit mark into two 16 bit marks
by setting an appropriate mask at the xfrm and the routing mark.
But this has the drawback that the socket needs to know how possibly
tunneled packets should be routed.

> 
> A real use case is IWLAN [2], which is used to connect to 3GPP
> networks over wifi. The phone has the same IP addresses on the
> cellular interface and on an IPsec tunnel to a carrier gateway (PDG).
> On Android, a socket connected on the carrier network through a native
> interface such as LTE will have the socket mark that's specific to the
> carrier network. If the device then hands over the carrier connection
> to the IPsec tunnel over wifi, the socket will still have the mark of
> the LTE interface. If we used xfrm_mark, then the tunneled packets
> themselves will be routed on LTE instead of being routed on wifi. That
> obviously defeats the purpose.
> 
> Using XFRM_OUTPUT_MARK makes this much cleaner: the socket can
> continue to be marked with the carrier network's mark, and it's the
> IPsec tunnel itself that is bound to the wifi network using the mark
> in the SA.

So we transform the packet and may 'transform' the mark on the packet
too. This could make sense, but we have to point out the differences
between the xfrm_mark and the output_mark on the SA very explicit.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC net-next] net: xfrm: support setting an output mark.
  2017-08-08  7:51     ` Steffen Klassert
@ 2017-08-09 17:13       ` Lorenzo Colitti
  2017-08-10 10:53         ` Steffen Klassert
  0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Colitti @ 2017-08-09 17:13 UTC (permalink / raw)
  To: Steffen Klassert
  Cc: netdev, David Miller, Jamal Hadi Salim, Herbert Xu,
	Nathan Harold, Jonathan Basseri

On Tue, Aug 8, 2017 at 4:51 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> I thought you can just split the 32 bit mark into two 16 bit marks
> by setting an appropriate mask at the xfrm and the routing mark.
> But this has the drawback that the socket needs to know how possibly
> tunneled packets should be routed.

Right. And if those bits are already used for something else (e.g.,
Android uses something like 20 bits for marks) then that's not
possible.

Also - the other approach of using the SA mark for routing the
tunneled packet, that has backwards compatibility issues. If someone
is using mark-based routing, and has configured an SA with a mark,
then making the mark influence the routing lookup would change how
those tunnels are routed and possibly break them.

> So we transform the packet and may 'transform' the mark on the packet
> too. This could make sense, but we have to point out the differences
> between the xfrm_mark and the output_mark on the SA very explicit.

Ack. Where should this be pointed out? I've sent out a non-RFC version
to netdev, mostly unchanged but including a fair bit more rationale in
the commit message:

https://patchwork.ozlabs.org/patch/799891/

Or did you mean it should be be documented this in the ip-xfrm man page, or...?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC net-next] net: xfrm: support setting an output mark.
  2017-08-09 17:13       ` Lorenzo Colitti
@ 2017-08-10 10:53         ` Steffen Klassert
  0 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2017-08-10 10:53 UTC (permalink / raw)
  To: Lorenzo Colitti
  Cc: netdev, David Miller, Jamal Hadi Salim, Herbert Xu,
	Nathan Harold, Jonathan Basseri

On Thu, Aug 10, 2017 at 02:13:15AM +0900, Lorenzo Colitti wrote:
> On Tue, Aug 8, 2017 at 4:51 PM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> > I thought you can just split the 32 bit mark into two 16 bit marks
> > by setting an appropriate mask at the xfrm and the routing mark.
> > But this has the drawback that the socket needs to know how possibly
> > tunneled packets should be routed.
> 
> Right. And if those bits are already used for something else (e.g.,
> Android uses something like 20 bits for marks) then that's not
> possible.
> 
> Also - the other approach of using the SA mark for routing the
> tunneled packet, that has backwards compatibility issues. If someone
> is using mark-based routing, and has configured an SA with a mark,
> then making the mark influence the routing lookup would change how
> those tunnels are routed and possibly break them.

Right, good point.

> 
> > So we transform the packet and may 'transform' the mark on the packet
> > too. This could make sense, but we have to point out the differences
> > between the xfrm_mark and the output_mark on the SA very explicit.
> 
> Ack. Where should this be pointed out? I've sent out a non-RFC version
> to netdev, mostly unchanged but including a fair bit more rationale in
> the commit message:
> 
> https://patchwork.ozlabs.org/patch/799891/
> 
> Or did you mean it should be be documented this in the ip-xfrm man page, or...?

A detailed commit message is what I meant, this could be also
the base for the manpage once iproute2 gets support for this.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-08-10 10:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-07  9:23 [RFC net-next] net: xfrm: support setting an output mark Lorenzo Colitti
2017-08-07 11:16 ` Steffen Klassert
2017-08-07 13:34   ` Lorenzo Colitti
2017-08-08  7:51     ` Steffen Klassert
2017-08-09 17:13       ` Lorenzo Colitti
2017-08-10 10:53         ` Steffen Klassert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.