| Message ID | 20210712005554.26948-2-vfedorenko@novek.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix PMTU for ESP-in-UDP encapsulation | expand |
Hello, On Mon, 2021-07-12 at 03:55 +0300, Vadim Fedorenko wrote: > Usage of encap_type as a flag to determine encapsulation of udp > socket is not correct because there is special encap_enable field > for this check. Many network drivers use this field as a flag > instead of correctly indicate type of encapsulation. Remove such > usage and update all checks to use encap_enable field Uhmm... this looks quite dangerous to me. Apparently l2tp and gtp clear 'encap_type' to prevent the rx path pushing pkts into the encap on shutdown. Will such tunnel's shutdown be safe with the above? > Fixes: 60fb9567bf30 ("udp: implement complete book-keeping for encap_needed") IMHO this not fix. Which bug are you observing that is addressed here? Thanks! Paolo
On 12.07.2021 09:37, Paolo Abeni wrote: > Hello, > > On Mon, 2021-07-12 at 03:55 +0300, Vadim Fedorenko wrote: >> Usage of encap_type as a flag to determine encapsulation of udp >> socket is not correct because there is special encap_enable field >> for this check. Many network drivers use this field as a flag >> instead of correctly indicate type of encapsulation. Remove such >> usage and update all checks to use encap_enable field > > Uhmm... this looks quite dangerous to me. Apparently l2tp and gtp clear > 'encap_type' to prevent the rx path pushing pkts into the encap on > shutdown. Will such tunnel's shutdown be safe with the above? > I think it's safe because all the code that checks for encap_enabled checks for encap_rcv before invoking it and l2tp clears this handler. A bit different situation with gtp where no clearing is done while encap destroy, so I think the same approach could be done to clear the receive handler. I also realised that there could be some imbalance on udp_encap_needed_key in case of l2tp and gtp. I will try to investigate it a bit more. >> Fixes: 60fb9567bf30 ("udp: implement complete book-keeping for encap_needed") > > IMHO this not fix. Which bug are you observing that is addressed here? > I thought that introduction of encap_enabled should go further to switch the code to check this particular flag and leave encap_type as a description of specific type (or subtype) of used encapsulation. That's why I added Fixes tag. Correct me if I'm wrong on this assumption
On Mon, 2021-07-12 at 13:32 +0100, Vadim Fedorenko wrote: > On 12.07.2021 09:37, Paolo Abeni wrote: > > > Fixes: 60fb9567bf30 ("udp: implement complete book-keeping for encap_needed") > > > > IMHO this not fix. Which bug are you observing that is addressed here? > > > I thought that introduction of encap_enabled should go further to switch the > code to check this particular flag and leave encap_type as a description of > specific type (or subtype) of used encapsulation. Than to me it looks more like a refactor than a fix. Is this strictly needed by the following patch? if not, I suggest to consider net-next as a target for this patch, or even better, drop it altogether. Cheers, Paolo
On 12.07.2021 15:05, Paolo Abeni wrote: > On Mon, 2021-07-12 at 13:32 +0100, Vadim Fedorenko wrote: >> On 12.07.2021 09:37, Paolo Abeni wrote: >>>> Fixes: 60fb9567bf30 ("udp: implement complete book-keeping for encap_needed") >>> >>> IMHO this not fix. Which bug are you observing that is addressed here? >>> >> I thought that introduction of encap_enabled should go further to switch the >> code to check this particular flag and leave encap_type as a description of >> specific type (or subtype) of used encapsulation. > > Than to me it looks more like a refactor than a fix. Is this strictly > needed by the following patch? if not, I suggest to consider net-next > as a target for this patch, or even better, drop it altogether. > Looks like it isn't strictly needed for the following patch. Do you think that such refactor would lead to more harm than benefits provided by clearness of usage of encap_enable and encap_type fields? I mean why do you think that it's better to drop it? Thanks!
On Mon, 2021-07-12 at 15:13 +0100, Vadim Fedorenko wrote: > On 12.07.2021 15:05, Paolo Abeni wrote: > > On Mon, 2021-07-12 at 13:32 +0100, Vadim Fedorenko wrote: > > > On 12.07.2021 09:37, Paolo Abeni wrote: > > > > > Fixes: 60fb9567bf30 ("udp: implement complete book-keeping for encap_needed") > > > > > > > > IMHO this not fix. Which bug are you observing that is addressed here? > > > > > > > I thought that introduction of encap_enabled should go further to switch the > > > code to check this particular flag and leave encap_type as a description of > > > specific type (or subtype) of used encapsulation. > > > > Than to me it looks more like a refactor than a fix. Is this strictly > > needed by the following patch? if not, I suggest to consider net-next > > as a target for this patch, or even better, drop it altogether. > > > Looks like it isn't strictly needed for the following patch. Do you think that > such refactor would lead to more harm than benefits provided by clearness of > usage of encap_enable and encap_type fields? Yes. That patch is invasive and the clarification is quite subjective IMHO. Cheers, Paolo
On 12.07.2021 15:33, Paolo Abeni wrote: > On Mon, 2021-07-12 at 15:13 +0100, Vadim Fedorenko wrote: >> On 12.07.2021 15:05, Paolo Abeni wrote: >>> On Mon, 2021-07-12 at 13:32 +0100, Vadim Fedorenko wrote: >>>> On 12.07.2021 09:37, Paolo Abeni wrote: >>>>>> Fixes: 60fb9567bf30 ("udp: implement complete book-keeping for encap_needed") >>>>> >>>>> IMHO this not fix. Which bug are you observing that is addressed here? >>>>> >>>> I thought that introduction of encap_enabled should go further to switch the >>>> code to check this particular flag and leave encap_type as a description of >>>> specific type (or subtype) of used encapsulation. >>> >>> Than to me it looks more like a refactor than a fix. Is this strictly >>> needed by the following patch? if not, I suggest to consider net-next >>> as a target for this patch, or even better, drop it altogether. >>> >> Looks like it isn't strictly needed for the following patch. Do you think that >> such refactor would lead to more harm than benefits provided by clearness of >> usage of encap_enable and encap_type fields? > > Yes. That patch is invasive and the clarification is quite subjective > IMHO. > Ok, no problem, will drop it in next iteration. Thanks
diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c index 01662727dca0..f6515208e5af 100644 --- a/drivers/infiniband/sw/rxe/rxe_net.c +++ b/drivers/infiniband/sw/rxe/rxe_net.c @@ -212,7 +212,6 @@ static struct socket *rxe_setup_udp_tunnel(struct net *net, __be16 port, return ERR_PTR(err); } - tnl_cfg.encap_type = 1; tnl_cfg.encap_rcv = rxe_udp_encap_recv; /* Setup UDP tunnel */ diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c index a7ee0af1af90..4c84b327bba0 100644 --- a/drivers/net/bareudp.c +++ b/drivers/net/bareudp.c @@ -236,7 +236,6 @@ static int bareudp_socket_create(struct bareudp_dev *bareudp, __be16 port) /* Mark socket as an encapsulation socket */ memset(&tunnel_cfg, 0, sizeof(tunnel_cfg)); tunnel_cfg.sk_user_data = bareudp; - tunnel_cfg.encap_type = 1; tunnel_cfg.encap_rcv = bareudp_udp_encap_recv; tunnel_cfg.encap_err_lookup = bareudp_err_lookup; tunnel_cfg.encap_destroy = NULL; diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 1ab94b5f9bbf..953a9306af98 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -590,7 +590,6 @@ static struct geneve_sock *geneve_socket_create(struct net *net, __be16 port, /* Mark socket as an encapsulation socket */ memset(&tunnel_cfg, 0, sizeof(tunnel_cfg)); tunnel_cfg.sk_user_data = gs; - tunnel_cfg.encap_type = 1; tunnel_cfg.gro_receive = geneve_gro_receive; tunnel_cfg.gro_complete = geneve_gro_complete; tunnel_cfg.encap_rcv = geneve_udp_encap_recv; diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 5a8df5a195cb..4eba44b1120e 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -3539,7 +3539,6 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, bool ipv6, /* Mark socket as an encapsulation socket. */ memset(&tunnel_cfg, 0, sizeof(tunnel_cfg)); tunnel_cfg.sk_user_data = vs; - tunnel_cfg.encap_type = 1; tunnel_cfg.encap_rcv = vxlan_rcv; tunnel_cfg.encap_err_lookup = vxlan_err_lookup; tunnel_cfg.encap_destroy = NULL; diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c index 8c496b747108..81669dd0e6cd 100644 --- a/drivers/net/wireguard/socket.c +++ b/drivers/net/wireguard/socket.c @@ -351,7 +351,6 @@ int wg_socket_init(struct wg_device *wg, u16 port) int ret; struct udp_tunnel_sock_cfg cfg = { .sk_user_data = wg, - .encap_type = 1, .encap_rcv = wg_receive }; struct socket *new4 = NULL, *new6 = NULL; diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c index e5f69b0bf3df..b80f56594e0a 100644 --- a/net/ipv4/fou.c +++ b/net/ipv4/fou.c @@ -590,7 +590,6 @@ static int fou_create(struct net *net, struct fou_cfg *cfg, fou->sock = sock; memset(&tunnel_cfg, 0, sizeof(tunnel_cfg)); - tunnel_cfg.encap_type = 1; tunnel_cfg.sk_user_data = fou; tunnel_cfg.encap_destroy = NULL; diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 62cd4cd52e84..e5cb7fedfbcd 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -707,7 +707,7 @@ int __udp4_lib_err(struct sk_buff *skb, u32 info, struct udp_table *udptable) sk = __udp4_lib_lookup(net, iph->daddr, uh->dest, iph->saddr, uh->source, skb->dev->ifindex, inet_sdif(skb), udptable, NULL); - if (!sk || udp_sk(sk)->encap_type) { + if (!sk || udp_sk(sk)->encap_enabled) { /* No socket for error: try tunnels before discarding */ sk = ERR_PTR(-ENOENT); if (static_branch_unlikely(&udp_encap_needed_key)) { @@ -2105,7 +2105,7 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb) goto drop; nf_reset_ct(skb); - if (static_branch_unlikely(&udp_encap_needed_key) && up->encap_type) { + if (static_branch_unlikely(&udp_encap_needed_key) && up->encap_enabled) { int (*encap_rcv)(struct sock *sk, struct sk_buff *skb); /* @@ -2615,14 +2615,13 @@ void udp_destroy_sock(struct sock *sk) udp_flush_pending_frames(sk); unlock_sock_fast(sk, slow); if (static_branch_unlikely(&udp_encap_needed_key)) { - if (up->encap_type) { + if (up->encap_enabled) { void (*encap_destroy)(struct sock *sk); encap_destroy = READ_ONCE(up->encap_destroy); if (encap_destroy) encap_destroy(sk); - } - if (up->encap_enabled) static_branch_dec(&udp_encap_needed_key); + } } } diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index 0cc7ba531b34..798916d2e722 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -558,7 +558,7 @@ int __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, sk = __udp6_lib_lookup(net, daddr, uh->dest, saddr, uh->source, inet6_iif(skb), inet6_sdif(skb), udptable, NULL); - if (!sk || udp_sk(sk)->encap_type) { + if (!sk || udp_sk(sk)->encap_enabled) { /* No socket for error: try tunnels before discarding */ sk = ERR_PTR(-ENOENT); if (static_branch_unlikely(&udpv6_encap_needed_key)) { @@ -661,7 +661,7 @@ static int udpv6_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb) if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb)) goto drop; - if (static_branch_unlikely(&udpv6_encap_needed_key) && up->encap_type) { + if (static_branch_unlikely(&udpv6_encap_needed_key) && up->encap_enabled) { int (*encap_rcv)(struct sock *sk, struct sk_buff *skb); /* @@ -1605,13 +1605,11 @@ void udpv6_destroy_sock(struct sock *sk) release_sock(sk); if (static_branch_unlikely(&udpv6_encap_needed_key)) { - if (up->encap_type) { + if (up->encap_enabled) { void (*encap_destroy)(struct sock *sk); encap_destroy = READ_ONCE(up->encap_destroy); if (encap_destroy) encap_destroy(sk); - } - if (up->encap_enabled) { static_branch_dec(&udpv6_encap_needed_key); udp_encap_disable(); } diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c index ec0f52567c16..2eccb3f9122b 100644 --- a/net/sctp/protocol.c +++ b/net/sctp/protocol.c @@ -872,7 +872,6 @@ int sctp_udp_sock_start(struct net *net) return err; } - tuncfg.encap_type = 1; tuncfg.encap_rcv = sctp_udp_rcv; tuncfg.encap_err_lookup = sctp_udp_v4_err; setup_udp_tunnel_sock(net, sock, &tuncfg); @@ -894,7 +893,6 @@ int sctp_udp_sock_start(struct net *net) return err; } - tuncfg.encap_type = 1; tuncfg.encap_rcv = sctp_udp_rcv; tuncfg.encap_err_lookup = sctp_udp_v6_err; setup_udp_tunnel_sock(net, sock, &tuncfg); diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c index c2bb818704c8..da0c679dae37 100644 --- a/net/tipc/udp_media.c +++ b/net/tipc/udp_media.c @@ -771,7 +771,6 @@ static int tipc_udp_enable(struct net *net, struct tipc_bearer *b, if (err) goto err; tuncfg.sk_user_data = ub; - tuncfg.encap_type = 1; tuncfg.encap_rcv = tipc_udp_recv; tuncfg.encap_destroy = NULL; setup_udp_tunnel_sock(net, ub->ubsock, &tuncfg);
Usage of encap_type as a flag to determine encapsulation of udp socket is not correct because there is special encap_enable field for this check. Many network drivers use this field as a flag instead of correctly indicate type of encapsulation. Remove such usage and update all checks to use encap_enable field Fixes: 60fb9567bf30 ("udp: implement complete book-keeping for encap_needed") Signed-off-by: Vadim Fedorenko <vfedorenko@novek.ru> --- drivers/infiniband/sw/rxe/rxe_net.c | 1 - drivers/net/bareudp.c | 1 - drivers/net/geneve.c | 1 - drivers/net/vxlan.c | 1 - drivers/net/wireguard/socket.c | 1 - net/ipv4/fou.c | 1 - net/ipv4/udp.c | 9 ++++----- net/ipv6/udp.c | 8 +++----- net/sctp/protocol.c | 2 -- net/tipc/udp_media.c | 1 - 10 files changed, 7 insertions(+), 19 deletions(-)