netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
@ 2017-08-15 22:25 Jonathan Basseri
  2017-08-16  9:03 ` Lorenzo Colitti
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Jonathan Basseri @ 2017-08-15 22:25 UTC (permalink / raw)
  To: netdev; +Cc: Jonathan Basseri, jkbs, davem, steffen.klassert, lorenzo

If an IPv6 socket has a valid dst cache, then xfrm_lookup_route will get
skipped. However, the cache is not invalidated when applying policy to a
socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
sometimes ignored on those sockets.

This can be demonstrated like so,
1. Create UDPv6 socket.
2. connect() the socket.
3. Apply an outbound XFRM policy to the socket.
4. send() data on the socket.

Packets will continue to be sent in the clear instead of matching an
xfrm or returning a no-match error (EAGAIN). This affects calls to
send() and not sendto().

Note: Creating normal XFRM policies should have a similar effect on
sk_dst_cache entries that match the policy, but that is not fixed in
this patch.

Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
Tested: https://android-review.googlesource.com/418659
Signed-off-by: Jonathan Basseri <misterikkit@google.com>
---
 net/xfrm/xfrm_state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 6c0956d10db6..46294cc833f3 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2028,33 +2028,34 @@ EXPORT_SYMBOL(km_is_alive);
 int xfrm_user_policy(struct sock *sk, int optname, u8 __user *optval, int optlen)
 {
 	int err;
 	u8 *data;
 	struct xfrm_mgr *km;
 	struct xfrm_policy *pol = NULL;
 
 	if (optlen <= 0 || optlen > PAGE_SIZE)
 		return -EMSGSIZE;
 
 	data = memdup_user(optval, optlen);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
 	err = -EINVAL;
 	rcu_read_lock();
 	list_for_each_entry_rcu(km, &xfrm_km_list, list) {
 		pol = km->compile_policy(sk, optname, data,
 					 optlen, &err);
 		if (err >= 0)
 			break;
 	}
 	rcu_read_unlock();
 
 	if (err >= 0) {
 		xfrm_sk_policy_insert(sk, err, pol);
 		xfrm_pol_put(pol);
+		sk_dst_reset(sk);
 		err = 0;
 	}
 
 	kfree(data);
 	return err;
 }
-- 
2.14.1.480.gb18f417b89-goog

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
@ 2017-10-24  1:30 Jonathan Basseri 😶
  0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Basseri 😶 @ 2017-10-24  1:30 UTC (permalink / raw)
  To: netdev
  Cc: Jakub Sitnicki, David Miller, Steffen Klassert, Lorenzo Colitti,
	Eric Dumazet, Jonathan Basseri

On Mon, Oct 23, 2017 at 6:18 PM, Jonathan Basseri
<misterikkit@google.com> wrote:
> If a socket has a valid dst cache, then xfrm_lookup_route will get
> skipped. However, the cache is not invalidated when applying policy to a
> socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> sometimes ignored on those sockets. (Note: This was broken for IPv4 and
> IPv6 at different times.)
>
> This can be demonstrated like so,
> 1. Create UDP socket.
> 2. connect() the socket.
> 3. Apply an outbound XFRM policy to the socket.
> 4. send() data on the socket.
>
> Packets will continue to be sent in the clear instead of matching an
> xfrm or returning a no-match error (EAGAIN). This affects calls to
> send() and not sendto().
>
> Invalidating the sk_dst_cache is necessary to correctly apply xfrm
> policies. Since we do this in xfrm_user_policy(), the sk_lock was
> already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
> and we may call __sk_dst_reset().
>
> Performance impact should be negligible, since this code is only called
> when changing xfrm policy, and only affects the socket in question.
>
> Note: Creating normal XFRM policies should have a similar effect on
> sk_dst_cache entries that match the policy, but that is not fixed in
> this patch.
>
> Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
> Tested: https://android-review.googlesource.com/517555
> Tested: https://android-review.googlesource.com/418659
> Signed-off-by: Jonathan Basseri <misterikkit@google.com>
>
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 12213477cd3a..1f5cee2269af 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -2045,33 +2045,34 @@ EXPORT_SYMBOL(km_is_alive);
>  int xfrm_user_policy(struct sock *sk, int optname, u8 __user *optval, int optlen)
>  {
>         int err;
>         u8 *data;
>         struct xfrm_mgr *km;
>         struct xfrm_policy *pol = NULL;
>
>         if (optlen <= 0 || optlen > PAGE_SIZE)
>                 return -EMSGSIZE;
>
>         data = memdup_user(optval, optlen);
>         if (IS_ERR(data))
>                 return PTR_ERR(data);
>
>         err = -EINVAL;
>         rcu_read_lock();
>         list_for_each_entry_rcu(km, &xfrm_km_list, list) {
>                 pol = km->compile_policy(sk, optname, data,
>                                          optlen, &err);
>                 if (err >= 0)
>                         break;
>         }
>         rcu_read_unlock();
>
>         if (err >= 0) {
>                 xfrm_sk_policy_insert(sk, err, pol);
>                 xfrm_pol_put(pol);
> +               __sk_dst_reset(sk);
>                 err = 0;
>         }
>
>         kfree(data);
>         return err;
>  }
> --
> 2.15.0.rc0.271.g36b669edcc-goog
>

I discussed the concerns with Eric and I believe this addresses them.
(http://www.spinics.net/lists/netdev/msg449652.html)

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

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

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-15 22:25 [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy Jonathan Basseri
2017-08-16  9:03 ` Lorenzo Colitti
2017-08-16  9:03 ` Jakub Sitnicki
2017-08-16 10:43   ` Eric Dumazet
2017-08-16 11:19     ` Jakub Sitnicki
2017-10-24  1:18 ` Jonathan Basseri
2017-10-24  7:04   ` Steffen Klassert
2017-10-24 16:58     ` Jonathan Basseri 😶
2017-10-25  4:25       ` Steffen Klassert
2017-10-25 16:52         ` Jonathan Basseri
2017-10-25 16:52 ` [PATCH net v2] " Jonathan Basseri
2017-10-26 10:54   ` Steffen Klassert
  -- strict thread matches above, loose matches on Subject: below --
2017-10-24  1:30 [PATCH net] " Jonathan Basseri 😶

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).