Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Tue, Oct 24, 2017 at 9:25 PM, Steffen Klassert wrote: > On Tue, Oct 24, 2017 at 09:58:48AM -0700, Jonathan Basseri 😶 wrote: >> On Tue, Oct 24, 2017 at 12:04 AM, Steffen Klassert >> wrote: >> > >> > On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri 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. >> > >> > I think we don't have this problem with 'normal' policies. When >> > inserting such a policy, we bump the IPv4/IPv6 genid. This should >> > invalidate all cached dst entries, no? >> > >> That sounds reasonable to me. I had not confirmed the behavior for >> normal policies, so I was trying to point out that this fix is only >> for socket policies. Should I modify the commit message? > > Yes, please do so. This comment may lead people to the wrong direction. > > Thanks! Thank you for the feedback. Sending a v2 patch with updated message.
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Tue, Oct 24, 2017 at 09:58:48AM -0700, Jonathan Basseri 😶 wrote: > On Tue, Oct 24, 2017 at 12:04 AM, Steffen Klassert > wrote: > > > > On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri 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. > > > > I think we don't have this problem with 'normal' policies. When > > inserting such a policy, we bump the IPv4/IPv6 genid. This should > > invalidate all cached dst entries, no? > > > That sounds reasonable to me. I had not confirmed the behavior for > normal policies, so I was trying to point out that this fix is only > for socket policies. Should I modify the commit message? Yes, please do so. This comment may lead people to the wrong direction. Thanks!
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Tue, Oct 24, 2017 at 12:04 AM, Steffen Klassert wrote: > > On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri 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. > > I think we don't have this problem with 'normal' policies. When > inserting such a policy, we bump the IPv4/IPv6 genid. This should > invalidate all cached dst entries, no? > That sounds reasonable to me. I had not confirmed the behavior for normal policies, so I was trying to point out that this fix is only for socket policies. Should I modify the commit message?
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri 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. I think we don't have this problem with 'normal' policies. When inserting such a policy, we bump the IPv4/IPv6 genid. This should invalidate all cached dst entries, no?
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Mon, Oct 23, 2017 at 6:18 PM, Jonathan Basseri 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 > > 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)
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Wed, 16 Aug 2017 03:43:54 -0700 Eric Dumazet wrote: > On Wed, 2017-08-16 at 11:03 +0200, Jakub Sitnicki wrote: > > On Tue, 15 Aug 2017 15:25:10 -0700 > > Jonathan Basseri wrote: > > > > > 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 > > > --- > > > > Thank you for the fix. > > > > Acked-by: Jakub Sitnicki > > I do not believe this fix is correct. > > What happens if the socket is TCP ? > > sk_dst_reset(sk) is not safe for them. > > This might add use-after-free, and eventually crash. You are right. I see that RCU-variant __sk_dst_reset() is used throughout TCP stack. Thank you for pointing it out. Please disregard my earlier ACK. -Jakub
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Wed, 2017-08-16 at 11:03 +0200, Jakub Sitnicki wrote: > On Tue, 15 Aug 2017 15:25:10 -0700 > Jonathan Basseri wrote: > > > 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 > > --- > > Thank you for the fix. > > Acked-by: Jakub Sitnicki I do not believe this fix is correct. What happens if the socket is TCP ? sk_dst_reset(sk) is not safe for them. This might add use-after-free, and eventually crash.
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Tue, 15 Aug 2017 15:25:10 -0700 Jonathan Basseri wrote: > 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 > --- Thank you for the fix. Acked-by: Jakub Sitnicki
Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.
On Wed, Aug 16, 2017 at 7:25 AM, Jonathan Basseri wrote: > If an IPv6 socket has a valid dst cache Did you look into why IPv4 does not suffer from this problem? That said, clearing the dst cache entry does seem prudent in general.