[PATCH net v2] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[PATCH net v2] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[Daniel Borkmann]

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>
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. ]

 v1 -> v2:
   - Plain resend, adding more sunrpc folks to Cc

 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.
-- 
2.21.0

Re: [PATCH net v2] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[Chuck Lever]

On Fri, Jun 28, 2024 at 06:31:23PM +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>
> 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. ]
> 
>  v1 -> v2:
>    - Plain resend, adding more sunrpc folks to Cc
> 
>  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.
> -- 
> 2.21.0
> 

Hi Daniel -

I know this is not documented in MAINTAINERS, but changes to
net/sunrpc/xprtsock.c go to Anna Schumaker and Trond Myklebust,
cc: linux-nfs@vger.


-- 
Chuck Lever

[PATCH net v3] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[Daniel Borkmann]

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. ]

 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.
-- 
2.21.0

Re: [PATCH net v2] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[Simon Horman]

On Fri, Jun 28, 2024 at 01:19:49PM -0400, Chuck Lever wrote:
> On Fri, Jun 28, 2024 at 06:31:23PM +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>
> > 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. ]
> > 
> >  v1 -> v2:
> >    - Plain resend, adding more sunrpc folks to Cc
> > 
> >  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.
> > -- 
> > 2.21.0
> > 
> 
> Hi Daniel -
> 
> I know this is not documented in MAINTAINERS, but changes to
> net/sunrpc/xprtsock.c go to Anna Schumaker and Trond Myklebust,
> cc: linux-nfs@vger.

Would it be possible to update MAINTAINERS accordingly?

Re: [PATCH net v2] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[Chuck Lever III]

> On Jul 1, 2024, at 6:57 AM, Simon Horman <horms@kernel.org> wrote:
> 
> On Fri, Jun 28, 2024 at 01:19:49PM -0400, Chuck Lever wrote:
>> On Fri, Jun 28, 2024 at 06:31:23PM +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>
>>> 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. ]
>>> 
>>> v1 -> v2:
>>>   - Plain resend, adding more sunrpc folks to Cc
>>> 
>>> 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.
>>> -- 
>>> 2.21.0
>>> 
>> 
>> Hi Daniel -
>> 
>> I know this is not documented in MAINTAINERS, but changes to
>> net/sunrpc/xprtsock.c go to Anna Schumaker and Trond Myklebust,
>> cc: linux-nfs@vger.
> 
> Would it be possible to update MAINTAINERS accordingly?

I can do it, of course, but I'd like to discuss this
with the NFS client maintainers to ensure they agree
on how the files are divided between the trees.


--
Chuck Lever

Re: [PATCH net v2] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[Simon Horman]

On Mon, Jul 01, 2024 at 02:04:22PM +0000, Chuck Lever III wrote:
> > On Jul 1, 2024, at 6:57 AM, Simon Horman <horms@kernel.org> wrote:
> > On Fri, Jun 28, 2024 at 01:19:49PM -0400, Chuck Lever wrote:
> >> On Fri, Jun 28, 2024 at 06:31:23PM +0200, Daniel Borkmann wrote:

...

> >> Hi Daniel -
> >> 
> >> I know this is not documented in MAINTAINERS, but changes to
> >> net/sunrpc/xprtsock.c go to Anna Schumaker and Trond Myklebust,
> >> cc: linux-nfs@vger.
> > 
> > Would it be possible to update MAINTAINERS accordingly?
> 
> I can do it, of course, but I'd like to discuss this
> with the NFS client maintainers to ensure they agree
> on how the files are divided between the trees.

Hi Chuck,

Thanks in advance for raising this with the NFS client maintainers.
I do think it would be nice to resolve.

[PATCH net v3] net, sunrpc: Remap EPERM in case of connection failure in xs_tcp_setup_socket

[Daniel Borkmann]

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. ]

 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.
-- 
2.21.0