Re: [PATCH net] sctp: update dst pmtu with the correct daddr
From: Xin Long Date: Fri, 21 Sep 2018 15:55:34 +0800 > It's under the protection of the sock lock, I think any other places > that want to access the address also need to acquire this sock lock > first. Hash table lookups don't even have a socket context yet, so can't hold the sock lock, but look at the address for comparisons. Anything visible in a table lookup has to have stable keying information.
Re: [PATCH net] sctp: update dst pmtu with the correct daddr
On Fri, Sep 21, 2018 at 2:31 AM David Miller wrote:
>
> From: Xin Long
> Date: Thu, 20 Sep 2018 17:27:28 +0800
>
> > When processing pmtu update from an icmp packet, it calls .update_pmtu
> > with sk instead of skb in sctp_transport_update_pmtu.
> >
> > However for sctp, the daddr in the transport might be different from
> > inet_sock->inet_daddr or sk->sk_v6_daddr, which is used to update or
> > create the route cache. The incorrect daddr will cause a different
> > route cache created for the path.
> >
> > So before calling .update_pmtu, inet_sock->inet_daddr/sk->sk_v6_daddr
> > should be updated with the daddr in the transport, and update it back
> > after it's done.
> >
> > The issue has existed since route exceptions introduction.
> >
> > Fixes: 4895c771c7f0 ("ipv4: Add FIB nexthop exceptions.")
> > Reported-by: ian.per...@dialogic.com
> > Signed-off-by: Xin Long
>
> Applied and queued up for -stable.
>
> Although are you sure it's OK to temporarily change the sockets address
> like this? What if an asynchronous context looks at the socket state
> and sees the temporarily set address?
It's under the protection of the sock lock, I think any other places that
want to access the address also need to acquire this sock lock first.
Re: [PATCH net] sctp: update dst pmtu with the correct daddr
From: Xin Long
Date: Thu, 20 Sep 2018 17:27:28 +0800
> When processing pmtu update from an icmp packet, it calls .update_pmtu
> with sk instead of skb in sctp_transport_update_pmtu.
>
> However for sctp, the daddr in the transport might be different from
> inet_sock->inet_daddr or sk->sk_v6_daddr, which is used to update or
> create the route cache. The incorrect daddr will cause a different
> route cache created for the path.
>
> So before calling .update_pmtu, inet_sock->inet_daddr/sk->sk_v6_daddr
> should be updated with the daddr in the transport, and update it back
> after it's done.
>
> The issue has existed since route exceptions introduction.
>
> Fixes: 4895c771c7f0 ("ipv4: Add FIB nexthop exceptions.")
> Reported-by: ian.per...@dialogic.com
> Signed-off-by: Xin Long
Applied and queued up for -stable.
Although are you sure it's OK to temporarily change the sockets address
like this? What if an asynchronous context looks at the socket state
and sees the temporarily set address?
Thanks.
Re: [PATCH net] sctp: update dst pmtu with the correct daddr
On Thu, Sep 20, 2018 at 05:27:28PM +0800, Xin Long wrote:
> When processing pmtu update from an icmp packet, it calls .update_pmtu
> with sk instead of skb in sctp_transport_update_pmtu.
>
> However for sctp, the daddr in the transport might be different from
> inet_sock->inet_daddr or sk->sk_v6_daddr, which is used to update or
> create the route cache. The incorrect daddr will cause a different
> route cache created for the path.
>
> So before calling .update_pmtu, inet_sock->inet_daddr/sk->sk_v6_daddr
> should be updated with the daddr in the transport, and update it back
> after it's done.
>
> The issue has existed since route exceptions introduction.
>
> Fixes: 4895c771c7f0 ("ipv4: Add FIB nexthop exceptions.")
> Reported-by: ian.per...@dialogic.com
> Signed-off-by: Xin Long
Acked-by: Marcelo Ricardo Leitner
> ---
> net/sctp/transport.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 12cac85..033696e 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -260,6 +260,7 @@ void sctp_transport_pmtu(struct sctp_transport
> *transport, struct sock *sk)
> bool sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu)
> {
> struct dst_entry *dst = sctp_transport_dst_check(t);
> + struct sock *sk = t->asoc->base.sk;
> bool change = true;
>
> if (unlikely(pmtu < SCTP_DEFAULT_MINSEGMENT)) {
> @@ -271,12 +272,19 @@ bool sctp_transport_update_pmtu(struct sctp_transport
> *t, u32 pmtu)
> pmtu = SCTP_TRUNC4(pmtu);
>
> if (dst) {
> - dst->ops->update_pmtu(dst, t->asoc->base.sk, NULL, pmtu);
> + struct sctp_pf *pf = sctp_get_pf_specific(dst->ops->family);
> + union sctp_addr addr;
> +
> + pf->af->from_sk(, sk);
> + pf->to_sk_daddr(>ipaddr, sk);
> + dst->ops->update_pmtu(dst, sk, NULL, pmtu);
> + pf->to_sk_daddr(, sk);
> +
> dst = sctp_transport_dst_check(t);
> }
>
> if (!dst) {
> - t->af_specific->get_dst(t, >saddr, >fl, t->asoc->base.sk);
> + t->af_specific->get_dst(t, >saddr, >fl, sk);
> dst = t->dst;
> }
>
> --
> 2.1.0
>
[PATCH net] sctp: update dst pmtu with the correct daddr
When processing pmtu update from an icmp packet, it calls .update_pmtu
with sk instead of skb in sctp_transport_update_pmtu.
However for sctp, the daddr in the transport might be different from
inet_sock->inet_daddr or sk->sk_v6_daddr, which is used to update or
create the route cache. The incorrect daddr will cause a different
route cache created for the path.
So before calling .update_pmtu, inet_sock->inet_daddr/sk->sk_v6_daddr
should be updated with the daddr in the transport, and update it back
after it's done.
The issue has existed since route exceptions introduction.
Fixes: 4895c771c7f0 ("ipv4: Add FIB nexthop exceptions.")
Reported-by: ian.per...@dialogic.com
Signed-off-by: Xin Long
---
net/sctp/transport.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 12cac85..033696e 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -260,6 +260,7 @@ void sctp_transport_pmtu(struct sctp_transport *transport,
struct sock *sk)
bool sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu)
{
struct dst_entry *dst = sctp_transport_dst_check(t);
+ struct sock *sk = t->asoc->base.sk;
bool change = true;
if (unlikely(pmtu < SCTP_DEFAULT_MINSEGMENT)) {
@@ -271,12 +272,19 @@ bool sctp_transport_update_pmtu(struct sctp_transport *t,
u32 pmtu)
pmtu = SCTP_TRUNC4(pmtu);
if (dst) {
- dst->ops->update_pmtu(dst, t->asoc->base.sk, NULL, pmtu);
+ struct sctp_pf *pf = sctp_get_pf_specific(dst->ops->family);
+ union sctp_addr addr;
+
+ pf->af->from_sk(, sk);
+ pf->to_sk_daddr(>ipaddr, sk);
+ dst->ops->update_pmtu(dst, sk, NULL, pmtu);
+ pf->to_sk_daddr(, sk);
+
dst = sctp_transport_dst_check(t);
}
if (!dst) {
- t->af_specific->get_dst(t, >saddr, >fl, t->asoc->base.sk);
+ t->af_specific->get_dst(t, >saddr, >fl, sk);
dst = t->dst;
}
--
2.1.0
