net/tls: Fix return values for setsockopt
diff mbox series

Message ID 20191203224458.24338-1-vvidic@valentin-vidic.from.hr
State Superseded
Delegated to: David Miller
Headers show
Series
  • net/tls: Fix return values for setsockopt
Related show

Commit Message

Valentin Vidić Dec. 3, 2019, 10:44 p.m. UTC
ENOTSUPP is not available in userspace:

  setsockopt failed, 524, Unknown error 524

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
 net/tls/tls_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Willem de Bruijn Dec. 4, 2019, 7:22 p.m. UTC | #1
On Tue, Dec 3, 2019 at 6:08 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Tue,  3 Dec 2019 23:44:58 +0100, Valentin Vidic wrote:
> > ENOTSUPP is not available in userspace:
> >
> >   setsockopt failed, 524, Unknown error 524
> >
> > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
>
> I'm not 100% clear on whether we can change the return codes after they
> had been exposed to user space for numerous releases..

This has also come up in the context of SO_ZEROCOPY in the past. In my
opinion the answer is no. A quick grep | wc -l in net/ shows 99
matches for this error code. Only a fraction of those probably make it
to userspace, but definitely more than this single case.

If anything, it may be time to define it in uapi?

>
>
> But if we can - please fix the tools/testing/selftests/net/tls.c test
> as well, because it expects ENOTSUPP.

Even if changing the error code, EOPNOTSUPP is arguably a better
replacement. The request itself is valid. Also considering forward
compatibility.
Jakub Kicinski Dec. 4, 2019, 7:35 p.m. UTC | #2
(there is a v2, in case you missed)

On Wed, 4 Dec 2019 14:22:55 -0500, Willem de Bruijn wrote:
> On Tue, Dec 3, 2019 at 6:08 PM Jakub Kicinski wrote:
> > On Tue,  3 Dec 2019 23:44:58 +0100, Valentin Vidic wrote:  
> > > ENOTSUPP is not available in userspace:
> > >
> > >   setsockopt failed, 524, Unknown error 524
> > >
> > > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>  
> >
> > I'm not 100% clear on whether we can change the return codes after they
> > had been exposed to user space for numerous releases..  
> 
> This has also come up in the context of SO_ZEROCOPY in the past. In my
> opinion the answer is no. A quick grep | wc -l in net/ shows 99
> matches for this error code. Only a fraction of those probably make it
> to userspace, but definitely more than this single case.
> 
> If anything, it may be time to define it in uapi?

No opinion but FWIW I'm toying with some CI for netdev, I've added a
check for use of ENOTSUPP, apparently checkpatch already sniffs out
uses of ENOSYS, so seems appropriate to add this one.

> > But if we can - please fix the tools/testing/selftests/net/tls.c test
> > as well, because it expects ENOTSUPP.  
> 
> Even if changing the error code, EOPNOTSUPP is arguably a better
> replacement. The request itself is valid. Also considering forward
> compatibility.

For the case TLS version case?
Willem de Bruijn Dec. 4, 2019, 8:43 p.m. UTC | #3
On Wed, Dec 4, 2019 at 2:36 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> (there is a v2, in case you missed)

Thanks. I meant to respond to your comment. (but should have done sooner :)

> On Wed, 4 Dec 2019 14:22:55 -0500, Willem de Bruijn wrote:
> > On Tue, Dec 3, 2019 at 6:08 PM Jakub Kicinski wrote:
> > > On Tue,  3 Dec 2019 23:44:58 +0100, Valentin Vidic wrote:
> > > > ENOTSUPP is not available in userspace:
> > > >
> > > >   setsockopt failed, 524, Unknown error 524
> > > >
> > > > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
> > >
> > > I'm not 100% clear on whether we can change the return codes after they
> > > had been exposed to user space for numerous releases..
> >
> > This has also come up in the context of SO_ZEROCOPY in the past. In my
> > opinion the answer is no. A quick grep | wc -l in net/ shows 99
> > matches for this error code. Only a fraction of those probably make it
> > to userspace, but definitely more than this single case.
> >
> > If anything, it may be time to define it in uapi?
>
> No opinion but FWIW I'm toying with some CI for netdev, I've added a
> check for use of ENOTSUPP, apparently checkpatch already sniffs out
> uses of ENOSYS, so seems appropriate to add this one.

Good idea if not exposing this in UAPI.

> > > But if we can - please fix the tools/testing/selftests/net/tls.c test
> > > as well, because it expects ENOTSUPP.
> >
> > Even if changing the error code, EOPNOTSUPP is arguably a better
> > replacement. The request itself is valid. Also considering forward
> > compatibility.
>
> For the case TLS version case?

Yes. It's a more specific signal. Quite a few error paths already return EINVAL.
David Miller Dec. 4, 2019, 8:51 p.m. UTC | #4
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 4 Dec 2019 15:43:00 -0500

> On Wed, Dec 4, 2019 at 2:36 PM Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>>
>> (there is a v2, in case you missed)
> 
> Thanks. I meant to respond to your comment. (but should have done sooner :)
> 
>> On Wed, 4 Dec 2019 14:22:55 -0500, Willem de Bruijn wrote:
>> > On Tue, Dec 3, 2019 at 6:08 PM Jakub Kicinski wrote:
>> > > On Tue,  3 Dec 2019 23:44:58 +0100, Valentin Vidic wrote:
>> > > > ENOTSUPP is not available in userspace:
>> > > >
>> > > >   setsockopt failed, 524, Unknown error 524
>> > > >
>> > > > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
>> > >
>> > > I'm not 100% clear on whether we can change the return codes after they
>> > > had been exposed to user space for numerous releases..
>> >
>> > This has also come up in the context of SO_ZEROCOPY in the past. In my
>> > opinion the answer is no. A quick grep | wc -l in net/ shows 99
>> > matches for this error code. Only a fraction of those probably make it
>> > to userspace, but definitely more than this single case.
>> >
>> > If anything, it may be time to define it in uapi?
>>
>> No opinion but FWIW I'm toying with some CI for netdev, I've added a
>> check for use of ENOTSUPP, apparently checkpatch already sniffs out
>> uses of ENOSYS, so seems appropriate to add this one.
> 
> Good idea if not exposing this in UAPI.

I'm trying to understand this part of the discussion.

If we have been returning a non-valid error code, this 524 internal
kernel thing, it is _NOT_ an exposed UAPI.

It is a kernel bug and we should fix it.

If userspace anywhere is checking for 524, that is what needs to be fixed.

Do we agree on this point?
Jakub Kicinski Dec. 4, 2019, 11:01 p.m. UTC | #5
On Wed, 04 Dec 2019 12:51:35 -0800 (PST), David Miller wrote:
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Wed, 4 Dec 2019 15:43:00 -0500
> > On Wed, Dec 4, 2019 at 2:36 PM Jakub Kicinski wrote:  
> >> On Wed, 4 Dec 2019 14:22:55 -0500, Willem de Bruijn wrote:  
> >> > On Tue, Dec 3, 2019 at 6:08 PM Jakub Kicinski wrote:  
> >> > > On Tue,  3 Dec 2019 23:44:58 +0100, Valentin Vidic wrote:  
> >> > > > ENOTSUPP is not available in userspace:
> >> > > >
> >> > > >   setsockopt failed, 524, Unknown error 524
> >> > > >
> >> > > > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>  
> >> > >
> >> > > I'm not 100% clear on whether we can change the return codes after they
> >> > > had been exposed to user space for numerous releases..  
> >> >
> >> > This has also come up in the context of SO_ZEROCOPY in the past. In my
> >> > opinion the answer is no. A quick grep | wc -l in net/ shows 99
> >> > matches for this error code. Only a fraction of those probably make it
> >> > to userspace, but definitely more than this single case.
> >> >
> >> > If anything, it may be time to define it in uapi?  
> >>
> >> No opinion but FWIW I'm toying with some CI for netdev, I've added a
> >> check for use of ENOTSUPP, apparently checkpatch already sniffs out
> >> uses of ENOSYS, so seems appropriate to add this one.  
> > 
> > Good idea if not exposing this in UAPI.  
> 
> I'm trying to understand this part of the discussion.
> 
> If we have been returning a non-valid error code, this 524 internal
> kernel thing, it is _NOT_ an exposed UAPI.
> 
> It is a kernel bug and we should fix it.

I agree. We should just fix this.

As Willem points out the use of this error code has spread, but in
theory I'm a co-maintainer of the TLS code now, and my maintainer 
gut says "just fix it" :)

> If userspace anywhere is checking for 524, that is what needs to be fixed.

FWIW I did a quick grep through openssl and gnutls and fbthrift and I
see no references to ENOTSUPP or 524.

Valentin, what's the strategy you're using for this fix? There's a
bunch of ENOTSUPP in net/tls/tls_sw.c as well, could you convert those,
too?
David Miller Dec. 5, 2019, 12:55 a.m. UTC | #6
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Wed, 4 Dec 2019 15:01:36 -0800

> Valentin, what's the strategy you're using for this fix? There's a
> bunch of ENOTSUPP in net/tls/tls_sw.c as well, could you convert those,
> too?

Yes I see those as well, let's get them all in one patch ok?

Patch
diff mbox series

diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index bdca31ffe6da..5830b8e02a36 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -496,7 +496,7 @@  static int do_tls_setsockopt_conf(struct sock *sk, char __user *optval,
 	/* check version */
 	if (crypto_info->version != TLS_1_2_VERSION &&
 	    crypto_info->version != TLS_1_3_VERSION) {
-		rc = -ENOTSUPP;
+		rc = -EINVAL;
 		goto err_crypto_info;
 	}
 
@@ -723,7 +723,7 @@  static int tls_init(struct sock *sk)
 	 * share the ulp context.
 	 */
 	if (sk->sk_state != TCP_ESTABLISHED)
-		return -ENOTSUPP;
+		return -ENOTCONN;
 
 	/* allocate tls context */
 	write_lock_bh(&sk->sk_callback_lock);