| Message ID | 9069ec1d59e4b2129fc23433349fd5580ad43921.1720075070.git.daniel@iogearbox.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v3,resend] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket | expand |
On 7/4/24 8:41 AM, Daniel Borkmann wrote: > When using a BPF program on kernel_connect(), the call can return -EPERM. This > causes xs_tcp_setup_socket() to loop forever, filling up the syslog and causing > the kernel to potentially freeze up. > > Neil suggested: > > This will propagate -EPERM up into other layers which might not be ready > to handle it. It might be safer to map EPERM to an error we would be more > likely to expect from the network system - such as ECONNREFUSED or ENETDOWN. > > ECONNREFUSED as error seems reasonable. For programs setting a different error > can be out of reach (see handling in 4fbac77d2d09) in particular on kernels > which do not have f10d05966196 ("bpf: Make BPF_PROG_RUN_ARRAY return -err > instead of allow boolean"), thus given that it is better to simply remap for > consistent behavior. UDP does handle EPERM in xs_udp_send_request(). > > Fixes: d74bad4e74ee ("bpf: Hooks for sys_connect") > Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind") > Co-developed-by: Lex Siegel <usiegl00@gmail.com> > Signed-off-by: Lex Siegel <usiegl00@gmail.com> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Neil Brown <neilb@suse.de> > Cc: Trond Myklebust <trondmy@kernel.org> > Cc: Anna Schumaker <anna@kernel.org> > Link: https://github.com/cilium/cilium/issues/33395 > Link: https://lore.kernel.org/bpf/171374175513.12877.8993642908082014881@noble.neil.brown.name > --- > [ Fixes tags are set to the orig connect commit so that stable team > can pick this up. > > Resend as it turns out that patchwork did not pick up the earlier > resends likely due to the message id being the same. ] > > v1 -> v2 -> v3: > - Plain resend, adding correct sunrpc folks to Cc > https://lore.kernel.org/bpf/Zn7wtStV+iafWRXj@tissot.1015granger.net/ (gentle ping)
On Thu, 2024-07-04 at 08:41 +0200, Daniel Borkmann wrote: > When using a BPF program on kernel_connect(), the call can return -EPERM. This > causes xs_tcp_setup_socket() to loop forever, filling up the syslog and causing > the kernel to potentially freeze up. > > Neil suggested: > > This will propagate -EPERM up into other layers which might not be ready > to handle it. It might be safer to map EPERM to an error we would be more > likely to expect from the network system - such as ECONNREFUSED or ENETDOWN. > > ECONNREFUSED as error seems reasonable. For programs setting a different error > can be out of reach (see handling in 4fbac77d2d09) in particular on kernels > which do not have f10d05966196 ("bpf: Make BPF_PROG_RUN_ARRAY return -err > instead of allow boolean"), thus given that it is better to simply remap for > consistent behavior. UDP does handle EPERM in xs_udp_send_request(). > > Fixes: d74bad4e74ee ("bpf: Hooks for sys_connect") > Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind") > Co-developed-by: Lex Siegel <usiegl00@gmail.com> > Signed-off-by: Lex Siegel <usiegl00@gmail.com> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Neil Brown <neilb@suse.de> > Cc: Trond Myklebust <trondmy@kernel.org> > Cc: Anna Schumaker <anna@kernel.org> > Link: https://github.com/cilium/cilium/issues/33395 > Link: https://lore.kernel.org/bpf/171374175513.12877.8993642908082014881@noble.neil.brown.name > --- > [ Fixes tags are set to the orig connect commit so that stable team > can pick this up. > > Resend as it turns out that patchwork did not pick up the earlier > resends likely due to the message id being the same. ] > > v1 -> v2 -> v3: > - Plain resend, adding correct sunrpc folks to Cc > https://lore.kernel.org/bpf/Zn7wtStV+iafWRXj@tissot.1015granger.net/ > > net/sunrpc/xprtsock.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > index dfc353eea8ed..0e1691316f42 100644 > --- a/net/sunrpc/xprtsock.c > +++ b/net/sunrpc/xprtsock.c > @@ -2441,6 +2441,13 @@ static void xs_tcp_setup_socket(struct work_struct *work) > transport->srcport = 0; > status = -EAGAIN; > break; > + case -EPERM: > + /* Happens, for instance, if a BPF program is preventing > + * the connect. Remap the error so upper layers can better > + * deal with it. > + */ > + status = -ECONNREFUSED; > + fallthrough; > case -EINVAL: > /* Happens, for instance, if the user specified a link > * local IPv6 address without a scope-id. The patch looks sane to me. @Trond, @Anna, are you ok for this to go directly into the net tree? Thanks! Paolo
On Tue, 2024-07-09 at 10:22 +0200, Paolo Abeni wrote: > On Thu, 2024-07-04 at 08:41 +0200, Daniel Borkmann wrote: > > When using a BPF program on kernel_connect(), the call can return -EPERM. This > > causes xs_tcp_setup_socket() to loop forever, filling up the syslog and causing > > the kernel to potentially freeze up. > > > > Neil suggested: > > > > This will propagate -EPERM up into other layers which might not be ready > > to handle it. It might be safer to map EPERM to an error we would be more > > likely to expect from the network system - such as ECONNREFUSED or ENETDOWN. > > > > ECONNREFUSED as error seems reasonable. For programs setting a different error > > can be out of reach (see handling in 4fbac77d2d09) in particular on kernels > > which do not have f10d05966196 ("bpf: Make BPF_PROG_RUN_ARRAY return -err > > instead of allow boolean"), thus given that it is better to simply remap for > > consistent behavior. UDP does handle EPERM in xs_udp_send_request(). > > > > Fixes: d74bad4e74ee ("bpf: Hooks for sys_connect") > > Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind") > > Co-developed-by: Lex Siegel <usiegl00@gmail.com> > > Signed-off-by: Lex Siegel <usiegl00@gmail.com> > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > > Cc: Neil Brown <neilb@suse.de> > > Cc: Trond Myklebust <trondmy@kernel.org> > > Cc: Anna Schumaker <anna@kernel.org> > > Link: https://github.com/cilium/cilium/issues/33395 > > Link: https://lore.kernel.org/bpf/171374175513.12877.8993642908082014881@noble.neil.brown.name > > --- > > [ Fixes tags are set to the orig connect commit so that stable team > > can pick this up. > > > > Resend as it turns out that patchwork did not pick up the earlier > > resends likely due to the message id being the same. ] > > > > v1 -> v2 -> v3: > > - Plain resend, adding correct sunrpc folks to Cc > > https://lore.kernel.org/bpf/Zn7wtStV+iafWRXj@tissot.1015granger.net/ > > > > net/sunrpc/xprtsock.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > > index dfc353eea8ed..0e1691316f42 100644 > > --- a/net/sunrpc/xprtsock.c > > +++ b/net/sunrpc/xprtsock.c > > @@ -2441,6 +2441,13 @@ static void xs_tcp_setup_socket(struct work_struct *work) > > transport->srcport = 0; > > status = -EAGAIN; > > break; > > + case -EPERM: > > + /* Happens, for instance, if a BPF program is preventing > > + * the connect. Remap the error so upper layers can better > > + * deal with it. > > + */ > > + status = -ECONNREFUSED; > > + fallthrough; > > case -EINVAL: > > /* Happens, for instance, if the user specified a link > > * local IPv6 address without a scope-id. > > The patch looks sane to me. @Trond, @Anna, are you ok for this to go > directly into the net tree? FTR, I think it's better to have it in today's net PR, so unless someone scream very loudly very soon, I'm going to merge it. Thanks, Paolo
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Thu, 4 Jul 2024 08:41:57 +0200 you wrote: > When using a BPF program on kernel_connect(), the call can return -EPERM. This > causes xs_tcp_setup_socket() to loop forever, filling up the syslog and causing > the kernel to potentially freeze up. > > Neil suggested: > > This will propagate -EPERM up into other layers which might not be ready > to handle it. It might be safer to map EPERM to an error we would be more > likely to expect from the network system - such as ECONNREFUSED or ENETDOWN. > > [...] Here is the summary with links: - [net,v3,resend] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket https://git.kernel.org/netdev/net/c/626dfed5fa3b You are awesome, thank you!
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index dfc353eea8ed..0e1691316f42 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c @@ -2441,6 +2441,13 @@ static void xs_tcp_setup_socket(struct work_struct *work) transport->srcport = 0; status = -EAGAIN; break; + case -EPERM: + /* Happens, for instance, if a BPF program is preventing + * the connect. Remap the error so upper layers can better + * deal with it. + */ + status = -ECONNREFUSED; + fallthrough; case -EINVAL: /* Happens, for instance, if the user specified a link * local IPv6 address without a scope-id.
When using a BPF program on kernel_connect(), the call can return -EPERM. This causes xs_tcp_setup_socket() to loop forever, filling up the syslog and causing the kernel to potentially freeze up. Neil suggested: This will propagate -EPERM up into other layers which might not be ready to handle it. It might be safer to map EPERM to an error we would be more likely to expect from the network system - such as ECONNREFUSED or ENETDOWN. ECONNREFUSED as error seems reasonable. For programs setting a different error can be out of reach (see handling in 4fbac77d2d09) in particular on kernels which do not have f10d05966196 ("bpf: Make BPF_PROG_RUN_ARRAY return -err instead of allow boolean"), thus given that it is better to simply remap for consistent behavior. UDP does handle EPERM in xs_udp_send_request(). Fixes: d74bad4e74ee ("bpf: Hooks for sys_connect") Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind") Co-developed-by: Lex Siegel <usiegl00@gmail.com> Signed-off-by: Lex Siegel <usiegl00@gmail.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Cc: Neil Brown <neilb@suse.de> Cc: Trond Myklebust <trondmy@kernel.org> Cc: Anna Schumaker <anna@kernel.org> Link: https://github.com/cilium/cilium/issues/33395 Link: https://lore.kernel.org/bpf/171374175513.12877.8993642908082014881@noble.neil.brown.name --- [ Fixes tags are set to the orig connect commit so that stable team can pick this up. Resend as it turns out that patchwork did not pick up the earlier resends likely due to the message id being the same. ] v1 -> v2 -> v3: - Plain resend, adding correct sunrpc folks to Cc https://lore.kernel.org/bpf/Zn7wtStV+iafWRXj@tissot.1015granger.net/ net/sunrpc/xprtsock.c | 7 +++++++ 1 file changed, 7 insertions(+)