netdev - Re: [PATCH net v3 resend] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <056562ebdab1add954ad19b3d78d9d5736a81190.camel@redhat.com>
Date: Thu, 11 Jul 2024 10:10:21 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Trond Myklebust <trondmy@...nel.org>, Anna Schumaker <anna@...nel.org>
Cc: netdev@...r.kernel.org, linux-nfs@...r.kernel.org, Lex Siegel
	 <usiegl00@...il.com>, Neil Brown <neilb@...e.de>, Daniel Borkmann
	 <daniel@...earbox.net>, kuba@...nel.org
Subject: Re: [PATCH net v3 resend] net, sunrpc: Remap EPERM in case of
 connection failure in xs_tcp_setup_socket

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@...il.com>
> > Signed-off-by: Lex Siegel <usiegl00@...il.com>
> > Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> > Cc: Neil Brown <neilb@...e.de>
> > Cc: Trond Myklebust <trondmy@...nel.org>
> > Cc: Anna Schumaker <anna@...nel.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