LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net/tls: Fix return values for setsockopt
@ 2019-12-03 22:44 Valentin Vidic
  2019-12-03 22:55 ` Jakub Kicinski
  0 siblings, 1 reply; 17+ messages in thread
From: Valentin Vidic @ 2019-12-03 22:44 UTC (permalink / raw)
  To: Boris Pismenny, Aviad Yehezkel, John Fastabend, Daniel Borkmann,
	Jakub Kicinski
  Cc: David S. Miller, netdev, linux-kernel, Valentin Vidic

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(-)

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);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] net/tls: Fix return values for setsockopt
  2019-12-03 22:44 [PATCH] net/tls: Fix return values for setsockopt Valentin Vidic
@ 2019-12-03 22:55 ` Jakub Kicinski
  2019-12-04 18:29   ` [PATCH v2] " Valentin Vidic
  2019-12-04 19:22   ` [PATCH] " Willem de Bruijn
  0 siblings, 2 replies; 17+ messages in thread
From: Jakub Kicinski @ 2019-12-03 22:55 UTC (permalink / raw)
  To: Valentin Vidic
  Cc: Boris Pismenny, Aviad Yehezkel, John Fastabend, Daniel Borkmann,
	David S. Miller, netdev, linux-kernel

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

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

> 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);


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] net/tls: Fix return values for setsockopt
  2019-12-03 22:55 ` Jakub Kicinski
@ 2019-12-04 18:29   ` " Valentin Vidic
  2019-12-04 19:22   ` [PATCH] " Willem de Bruijn
  1 sibling, 0 replies; 17+ messages in thread
From: Valentin Vidic @ 2019-12-04 18:29 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Boris Pismenny, Aviad Yehezkel, John Fastabend, Daniel Borkmann,
	David S. Miller, netdev, linux-kernel, Valentin Vidic

ENOTSUPP is not available in userspace:

  setsockopt failed, 524, Unknown error 524

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
v2: update error code in selftest

 net/tls/tls_main.c                | 4 ++--
 tools/testing/selftests/net/tls.c | 8 ++------
 2 files changed, 4 insertions(+), 8 deletions(-)

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);
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 1c8f194d6556..97c056ab43d9 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -25,10 +25,6 @@
 #define TLS_PAYLOAD_MAX_LEN 16384
 #define SOL_TLS 282
 
-#ifndef ENOTSUPP
-#define ENOTSUPP 524
-#endif
-
 FIXTURE(tls_basic)
 {
 	int fd, cfd;
@@ -1145,11 +1141,11 @@ TEST(non_established) {
 	/* TLS ULP not supported */
 	if (errno == ENOENT)
 		return;
-	EXPECT_EQ(errno, ENOTSUPP);
+	EXPECT_EQ(errno, ENOTCONN);
 
 	ret = setsockopt(sfd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
 	EXPECT_EQ(ret, -1);
-	EXPECT_EQ(errno, ENOTSUPP);
+	EXPECT_EQ(errno, ENOTCONN);
 
 	ret = getsockname(sfd, &addr, &len);
 	ASSERT_EQ(ret, 0);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] net/tls: Fix return values for setsockopt
  2019-12-03 22:55 ` Jakub Kicinski
  2019-12-04 18:29   ` [PATCH v2] " Valentin Vidic
@ 2019-12-04 19:22   ` " Willem de Bruijn
  2019-12-04 19:35     ` Jakub Kicinski
  1 sibling, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2019-12-04 19:22 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Valentin Vidic, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann, David S. Miller, Network Development,
	linux-kernel

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] net/tls: Fix return values for setsockopt
  2019-12-04 19:22   ` [PATCH] " Willem de Bruijn
@ 2019-12-04 19:35     ` Jakub Kicinski
  2019-12-04 20:43       ` Willem de Bruijn
  0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2019-12-04 19:35 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Valentin Vidic, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann, David S. Miller, Network Development,
	linux-kernel

(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?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] net/tls: Fix return values for setsockopt
  2019-12-04 19:35     ` Jakub Kicinski
@ 2019-12-04 20:43       ` Willem de Bruijn
  2019-12-04 20:51         ` David Miller
  0 siblings, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2019-12-04 20:43 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Willem de Bruijn, Valentin Vidic, Boris Pismenny, Aviad Yehezkel,
	John Fastabend, Daniel Borkmann, David S. Miller,
	Network Development, linux-kernel

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] net/tls: Fix return values for setsockopt
  2019-12-04 20:43       ` Willem de Bruijn
@ 2019-12-04 20:51         ` David Miller
  2019-12-04 23:01           ` Jakub Kicinski
  0 siblings, 1 reply; 17+ messages in thread
From: David Miller @ 2019-12-04 20:51 UTC (permalink / raw)
  To: willemdebruijn.kernel
  Cc: jakub.kicinski, vvidic, borisp, aviadye, john.fastabend, daniel,
	netdev, linux-kernel

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?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] net/tls: Fix return values for setsockopt
  2019-12-04 20:51         ` David Miller
@ 2019-12-04 23:01           ` Jakub Kicinski
  2019-12-05  0:55             ` David Miller
  0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2019-12-04 23:01 UTC (permalink / raw)
  To: David Miller
  Cc: willemdebruijn.kernel, vvidic, borisp, aviadye, john.fastabend,
	daniel, netdev, linux-kernel

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?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] net/tls: Fix return values for setsockopt
  2019-12-04 23:01           ` Jakub Kicinski
@ 2019-12-05  0:55             ` David Miller
  2019-12-05  6:41               ` [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP Valentin Vidic
  0 siblings, 1 reply; 17+ messages in thread
From: David Miller @ 2019-12-05  0:55 UTC (permalink / raw)
  To: jakub.kicinski
  Cc: willemdebruijn.kernel, vvidic, borisp, aviadye, john.fastabend,
	daniel, netdev, linux-kernel

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?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05  0:55             ` David Miller
@ 2019-12-05  6:41               ` Valentin Vidic
  2019-12-05 19:34                 ` Jakub Kicinski
  2019-12-07  4:17                 ` David Miller
  0 siblings, 2 replies; 17+ messages in thread
From: Valentin Vidic @ 2019-12-05  6:41 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Willem de Bruijn, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann, David S. Miller, netdev, linux-kernel,
	Valentin Vidic

ENOTSUPP is not available in userspace, for example:

  setsockopt failed, 524, Unknown error 524

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
v3: replace ENOTSUPP in other functions
v2: update error code in selftest

 net/tls/tls_device.c              | 8 ++++----
 net/tls/tls_main.c                | 4 ++--
 net/tls/tls_sw.c                  | 8 ++++----
 tools/testing/selftests/net/tls.c | 8 ++------
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 0683788bbef0..cd91ad812291 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -429,7 +429,7 @@ static int tls_push_data(struct sock *sk,
 
 	if (flags &
 	    ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST))
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	if (unlikely(sk->sk_err))
 		return -sk->sk_err;
@@ -571,7 +571,7 @@ int tls_device_sendpage(struct sock *sk, struct page *page,
 	lock_sock(sk);
 
 	if (flags & MSG_OOB) {
-		rc = -ENOTSUPP;
+		rc = -EOPNOTSUPP;
 		goto out;
 	}
 
@@ -1023,7 +1023,7 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx)
 	}
 
 	if (!(netdev->features & NETIF_F_HW_TLS_TX)) {
-		rc = -ENOTSUPP;
+		rc = -EOPNOTSUPP;
 		goto release_netdev;
 	}
 
@@ -1098,7 +1098,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
 	}
 
 	if (!(netdev->features & NETIF_F_HW_TLS_RX)) {
-		rc = -ENOTSUPP;
+		rc = -EOPNOTSUPP;
 		goto release_netdev;
 	}
 
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);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index da9f9ce51e7b..2969dc30e4e0 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -900,7 +900,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	int ret = 0;
 
 	if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL))
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	mutex_lock(&tls_ctx->tx_lock);
 	lock_sock(sk);
@@ -1215,7 +1215,7 @@ int tls_sw_sendpage_locked(struct sock *sk, struct page *page,
 	if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
 		      MSG_SENDPAGE_NOTLAST | MSG_SENDPAGE_NOPOLICY |
 		      MSG_NO_SHARED_FRAGS))
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	return tls_sw_do_sendpage(sk, page, offset, size, flags);
 }
@@ -1228,7 +1228,7 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
 
 	if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
 		      MSG_SENDPAGE_NOTLAST | MSG_SENDPAGE_NOPOLICY))
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	mutex_lock(&tls_ctx->tx_lock);
 	lock_sock(sk);
@@ -1927,7 +1927,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 
 		/* splice does not support reading control messages */
 		if (ctx->control != TLS_RECORD_TYPE_DATA) {
-			err = -ENOTSUPP;
+			err = -EINVAL;
 			goto splice_read_end;
 		}
 
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 1c8f194d6556..97c056ab43d9 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -25,10 +25,6 @@
 #define TLS_PAYLOAD_MAX_LEN 16384
 #define SOL_TLS 282
 
-#ifndef ENOTSUPP
-#define ENOTSUPP 524
-#endif
-
 FIXTURE(tls_basic)
 {
 	int fd, cfd;
@@ -1145,11 +1141,11 @@ TEST(non_established) {
 	/* TLS ULP not supported */
 	if (errno == ENOENT)
 		return;
-	EXPECT_EQ(errno, ENOTSUPP);
+	EXPECT_EQ(errno, ENOTCONN);
 
 	ret = setsockopt(sfd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
 	EXPECT_EQ(ret, -1);
-	EXPECT_EQ(errno, ENOTSUPP);
+	EXPECT_EQ(errno, ENOTCONN);
 
 	ret = getsockname(sfd, &addr, &len);
 	ASSERT_EQ(ret, 0);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05  6:41               ` [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP Valentin Vidic
@ 2019-12-05 19:34                 ` Jakub Kicinski
  2019-12-05 20:06                   ` Willem de Bruijn
  2019-12-07  4:17                 ` David Miller
  1 sibling, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2019-12-05 19:34 UTC (permalink / raw)
  To: Valentin Vidic
  Cc: Willem de Bruijn, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann, David S. Miller, netdev, linux-kernel

On Thu,  5 Dec 2019 07:41:18 +0100, Valentin Vidic wrote:
> ENOTSUPP is not available in userspace, for example:
> 
>   setsockopt failed, 524, Unknown error 524
> 
> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 0683788bbef0..cd91ad812291 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -429,7 +429,7 @@ static int tls_push_data(struct sock *sk,
>  
>  	if (flags &
>  	    ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST))
> -		return -ENOTSUPP;
> +		return -EOPNOTSUPP;
>  
>  	if (unlikely(sk->sk_err))
>  		return -sk->sk_err;
> @@ -571,7 +571,7 @@ int tls_device_sendpage(struct sock *sk, struct page *page,
>  	lock_sock(sk);
>  
>  	if (flags & MSG_OOB) {
> -		rc = -ENOTSUPP;
> +		rc = -EOPNOTSUPP;

Perhaps the flag checks should return EINVAL? Willem any opinions?

>  		goto out;
>  	}
>  
> @@ -1023,7 +1023,7 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx)
>  	}
>  
>  	if (!(netdev->features & NETIF_F_HW_TLS_TX)) {
> -		rc = -ENOTSUPP;
> +		rc = -EOPNOTSUPP;
>  		goto release_netdev;
>  	}
>  
> @@ -1098,7 +1098,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
>  	}
>  
>  	if (!(netdev->features & NETIF_F_HW_TLS_RX)) {
> -		rc = -ENOTSUPP;
> +		rc = -EOPNOTSUPP;
>  		goto release_netdev;
>  	}
>  
> 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;

This one I think Willem asked to be EOPNOTSUPP OTOH.

>  		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);
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index da9f9ce51e7b..2969dc30e4e0 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -900,7 +900,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  	int ret = 0;
>  
>  	if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL))
> -		return -ENOTSUPP;
> +		return -EOPNOTSUPP;
>  
>  	mutex_lock(&tls_ctx->tx_lock);
>  	lock_sock(sk);
> @@ -1215,7 +1215,7 @@ int tls_sw_sendpage_locked(struct sock *sk, struct page *page,
>  	if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
>  		      MSG_SENDPAGE_NOTLAST | MSG_SENDPAGE_NOPOLICY |
>  		      MSG_NO_SHARED_FRAGS))
> -		return -ENOTSUPP;
> +		return -EOPNOTSUPP;
>  
>  	return tls_sw_do_sendpage(sk, page, offset, size, flags);
>  }
> @@ -1228,7 +1228,7 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
>  
>  	if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
>  		      MSG_SENDPAGE_NOTLAST | MSG_SENDPAGE_NOPOLICY))
> -		return -ENOTSUPP;
> +		return -EOPNOTSUPP;

Same suggestion for flags checks in tls_sw.c

>  
>  	mutex_lock(&tls_ctx->tx_lock);
>  	lock_sock(sk);
> @@ -1927,7 +1927,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  
>  		/* splice does not support reading control messages */
>  		if (ctx->control != TLS_RECORD_TYPE_DATA) {
> -			err = -ENOTSUPP;
> +			err = -EINVAL;
>  			goto splice_read_end;
>  		}
>  


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05 19:34                 ` Jakub Kicinski
@ 2019-12-05 20:06                   ` Willem de Bruijn
  2019-12-05 20:43                     ` Valentin Vidić
  0 siblings, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2019-12-05 20:06 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Valentin Vidic, Willem de Bruijn, Boris Pismenny, Aviad Yehezkel,
	John Fastabend, Daniel Borkmann, David S. Miller,
	Network Development, linux-kernel

On Thu, Dec 5, 2019 at 2:34 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu,  5 Dec 2019 07:41:18 +0100, Valentin Vidic wrote:
> > ENOTSUPP is not available in userspace, for example:
> >
> >   setsockopt failed, 524, Unknown error 524
> >
> > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
>
> > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > index 0683788bbef0..cd91ad812291 100644
> > --- a/net/tls/tls_device.c
> > +++ b/net/tls/tls_device.c
> > @@ -429,7 +429,7 @@ static int tls_push_data(struct sock *sk,
> >
> >       if (flags &
> >           ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST))
> > -             return -ENOTSUPP;
> > +             return -EOPNOTSUPP;
> >
> >       if (unlikely(sk->sk_err))
> >               return -sk->sk_err;
> > @@ -571,7 +571,7 @@ int tls_device_sendpage(struct sock *sk, struct page *page,
> >       lock_sock(sk);
> >
> >       if (flags & MSG_OOB) {
> > -             rc = -ENOTSUPP;
> > +             rc = -EOPNOTSUPP;
>
> Perhaps the flag checks should return EINVAL? Willem any opinions?

No strong opinion. Judging from do_tcp_sendpages MSG_OOB is a
supported flag in general for sendpage, so signaling that the TLS
variant cannot support that otherwise valid request sounds fine to me.

>
> >               goto out;
> >       }
> >
> > @@ -1023,7 +1023,7 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx)
> >       }
> >
> >       if (!(netdev->features & NETIF_F_HW_TLS_TX)) {
> > -             rc = -ENOTSUPP;
> > +             rc = -EOPNOTSUPP;
> >               goto release_netdev;
> >       }
> >
> > @@ -1098,7 +1098,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
> >       }
> >
> >       if (!(netdev->features & NETIF_F_HW_TLS_RX)) {
> > -             rc = -ENOTSUPP;
> > +             rc = -EOPNOTSUPP;
> >               goto release_netdev;
> >       }
> >
> > 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;
>
> This one I think Willem asked to be EOPNOTSUPP OTOH.

Indeed (assuming no one disagrees). Based on the same rationale: the
request may be valid, it just cannot be accommodated (yet).

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05 20:06                   ` Willem de Bruijn
@ 2019-12-05 20:43                     ` Valentin Vidić
  2019-12-05 20:45                       ` Jakub Kicinski
  2019-12-05 21:26                       ` Willem de Bruijn
  0 siblings, 2 replies; 17+ messages in thread
From: Valentin Vidić @ 2019-12-05 20:43 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Jakub Kicinski, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann, David S. Miller, Network Development,
	linux-kernel

On Thu, Dec 05, 2019 at 03:06:55PM -0500, Willem de Bruijn wrote:
> On Thu, Dec 5, 2019 at 2:34 PM Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> >
> > On Thu,  5 Dec 2019 07:41:18 +0100, Valentin Vidic wrote:
> > > ENOTSUPP is not available in userspace, for example:
> > >
> > >   setsockopt failed, 524, Unknown error 524
> > >
> > > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
> >
> > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > > index 0683788bbef0..cd91ad812291 100644
> > > --- a/net/tls/tls_device.c
> > > +++ b/net/tls/tls_device.c
> > > @@ -429,7 +429,7 @@ static int tls_push_data(struct sock *sk,
> > >
> > >       if (flags &
> > >           ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST))
> > > -             return -ENOTSUPP;
> > > +             return -EOPNOTSUPP;
> > >
> > >       if (unlikely(sk->sk_err))
> > >               return -sk->sk_err;
> > > @@ -571,7 +571,7 @@ int tls_device_sendpage(struct sock *sk, struct page *page,
> > >       lock_sock(sk);
> > >
> > >       if (flags & MSG_OOB) {
> > > -             rc = -ENOTSUPP;
> > > +             rc = -EOPNOTSUPP;
> >
> > Perhaps the flag checks should return EINVAL? Willem any opinions?
> 
> No strong opinion. Judging from do_tcp_sendpages MSG_OOB is a
> supported flag in general for sendpage, so signaling that the TLS
> variant cannot support that otherwise valid request sounds fine to me.

I based these on the description from the sendmsg manpage, but you decide:

EOPNOTSUPP
    Some bit in the flags argument is inappropriate for the socket type.

> > > 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;
> >
> > This one I think Willem asked to be EOPNOTSUPP OTOH.
> 
> Indeed (assuming no one disagrees). Based on the same rationale: the
> request may be valid, it just cannot be accommodated (yet).

In this case other checks in the same function like crypto_info->cipher_type
return EINVAL, so I used the same here.

-- 
Valentin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05 20:43                     ` Valentin Vidić
@ 2019-12-05 20:45                       ` Jakub Kicinski
  2019-12-05 21:26                       ` Willem de Bruijn
  1 sibling, 0 replies; 17+ messages in thread
From: Jakub Kicinski @ 2019-12-05 20:45 UTC (permalink / raw)
  To: Valentin Vidić
  Cc: Willem de Bruijn, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann, David S. Miller, Network Development,
	linux-kernel

On Thu, 5 Dec 2019 21:43:43 +0100, Valentin Vidić wrote:
> > > On Thu,  5 Dec 2019 07:41:18 +0100, Valentin Vidic wrote:  
> > > > ENOTSUPP is not available in userspace, for example:
> > > >
> > > >   setsockopt failed, 524, Unknown error 524
> > > >
> > > > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>  
> > >  
> > > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > > > index 0683788bbef0..cd91ad812291 100644
> > > > --- a/net/tls/tls_device.c
> > > > +++ b/net/tls/tls_device.c
> > > > @@ -429,7 +429,7 @@ static int tls_push_data(struct sock *sk,
> > > >
> > > >       if (flags &
> > > >           ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST))
> > > > -             return -ENOTSUPP;
> > > > +             return -EOPNOTSUPP;
> > > >
> > > >       if (unlikely(sk->sk_err))
> > > >               return -sk->sk_err;
> > > > @@ -571,7 +571,7 @@ int tls_device_sendpage(struct sock *sk, struct page *page,
> > > >       lock_sock(sk);
> > > >
> > > >       if (flags & MSG_OOB) {
> > > > -             rc = -ENOTSUPP;
> > > > +             rc = -EOPNOTSUPP;  
> > >
> > > Perhaps the flag checks should return EINVAL? Willem any opinions?  
> > 
> > No strong opinion. Judging from do_tcp_sendpages MSG_OOB is a
> > supported flag in general for sendpage, so signaling that the TLS
> > variant cannot support that otherwise valid request sounds fine to me.  
> 
> I based these on the description from the sendmsg manpage, but you decide:
> 
> EOPNOTSUPP
>     Some bit in the flags argument is inappropriate for the socket type.
> 
> > > > 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;  
> > >
> > > This one I think Willem asked to be EOPNOTSUPP OTOH.  
> > 
> > Indeed (assuming no one disagrees). Based on the same rationale: the
> > request may be valid, it just cannot be accommodated (yet).  
> 
> In this case other checks in the same function like crypto_info->cipher_type
> return EINVAL, so I used the same here.

Thanks for explaining, in that case:

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05 20:43                     ` Valentin Vidić
  2019-12-05 20:45                       ` Jakub Kicinski
@ 2019-12-05 21:26                       ` Willem de Bruijn
  2019-12-05 23:08                         ` Valentin Vidić
  1 sibling, 1 reply; 17+ messages in thread
From: Willem de Bruijn @ 2019-12-05 21:26 UTC (permalink / raw)
  To: Valentin Vidić
  Cc: Willem de Bruijn, Jakub Kicinski, Boris Pismenny, Aviad Yehezkel,
	John Fastabend, Daniel Borkmann, David S. Miller,
	Network Development, linux-kernel

On Thu, Dec 5, 2019 at 3:44 PM Valentin Vidić
<vvidic@valentin-vidic.from.hr> wrote:
>
> On Thu, Dec 05, 2019 at 03:06:55PM -0500, Willem de Bruijn wrote:
> > On Thu, Dec 5, 2019 at 2:34 PM Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > >
> > > On Thu,  5 Dec 2019 07:41:18 +0100, Valentin Vidic wrote:
> > > > ENOTSUPP is not available in userspace, for example:
> > > >
> > > >   setsockopt failed, 524, Unknown error 524
> > > >
> > > > Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
> > >
> > > > diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> > > > index 0683788bbef0..cd91ad812291 100644
> > > > --- a/net/tls/tls_device.c
> > > > +++ b/net/tls/tls_device.c
> > > > @@ -429,7 +429,7 @@ static int tls_push_data(struct sock *sk,
> > > >
> > > >       if (flags &
> > > >           ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL | MSG_SENDPAGE_NOTLAST))
> > > > -             return -ENOTSUPP;
> > > > +             return -EOPNOTSUPP;
> > > >
> > > >       if (unlikely(sk->sk_err))
> > > >               return -sk->sk_err;
> > > > @@ -571,7 +571,7 @@ int tls_device_sendpage(struct sock *sk, struct page *page,
> > > >       lock_sock(sk);
> > > >
> > > >       if (flags & MSG_OOB) {
> > > > -             rc = -ENOTSUPP;
> > > > +             rc = -EOPNOTSUPP;
> > >
> > > Perhaps the flag checks should return EINVAL? Willem any opinions?
> >
> > No strong opinion. Judging from do_tcp_sendpages MSG_OOB is a
> > supported flag in general for sendpage, so signaling that the TLS
> > variant cannot support that otherwise valid request sounds fine to me.
>
> I based these on the description from the sendmsg manpage, but you decide:
>
> EOPNOTSUPP
>     Some bit in the flags argument is inappropriate for the socket type.

Interesting. That is a narrower interpretation than asm-generic/errno.h

  #define EOPNOTSUPP      95      /* Operation not supported on
transport endpoint */

which is also the string that strerror() generates.

>
> > > > 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;
> > >
> > > This one I think Willem asked to be EOPNOTSUPP OTOH.
> >
> > Indeed (assuming no one disagrees). Based on the same rationale: the
> > request may be valid, it just cannot be accommodated (yet).
>
> In this case other checks in the same function like crypto_info->cipher_type
> return EINVAL, so I used the same here.

That makes sense.

I think there is a fundamental difference between, say, passing an
argument of incorrect length (optlen < sizeof(..)) and asking for a
possibly unsupported cipher mode. But consistency trumps that.

I don't mean to drag this out by bike-shedding.

Happy to defer to maintainers on whether the errno on published code
can and should be changed, which is the more fundamental issue than
the exact errno.

FWIW, I also did not see existing openssl and gnutls callers test the
specific errno. The calls just fail on any setsockopt return value -1.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05 21:26                       ` Willem de Bruijn
@ 2019-12-05 23:08                         ` Valentin Vidić
  0 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidić @ 2019-12-05 23:08 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Jakub Kicinski, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann, David S. Miller, Network Development,
	linux-kernel

On Thu, Dec 05, 2019 at 04:26:14PM -0500, Willem de Bruijn wrote:
> > I based these on the description from the sendmsg manpage, but you decide:
> >
> > EOPNOTSUPP
> >     Some bit in the flags argument is inappropriate for the socket type.
> 
> Interesting. That is a narrower interpretation than asm-generic/errno.h
> 
>   #define EOPNOTSUPP      95      /* Operation not supported on
> transport endpoint */
> 
> which is also the string that strerror() generates.

Found one interesting example where EINVAL seems to mean "this value will never work"
and EOPNOTSUPP means "this value will not work in the current state/type of socket":

        case TCP_FASTOPEN_CONNECT:
                if (val > 1 || val < 0) {
                        err = -EINVAL;
                } else if (net->ipv4.sysctl_tcp_fastopen & TFO_CLIENT_ENABLE) {
                        if (sk->sk_state == TCP_CLOSE)
                                tp->fastopen_connect = val;
                        else
                                err = -EINVAL;
                } else {
                        err = -EOPNOTSUPP;
                }
                break;

> I think there is a fundamental difference between, say, passing an
> argument of incorrect length (optlen < sizeof(..)) and asking for a
> possibly unsupported cipher mode. But consistency trumps that.
> 
> I don't mean to drag this out by bike-shedding.
> 
> Happy to defer to maintainers on whether the errno on published code
> can and should be changed, which is the more fundamental issue than
> the exact errno.
> 
> FWIW, I also did not see existing openssl and gnutls callers test the
> specific errno. The calls just fail on any setsockopt return value -1.

Right, I'm also fine with whatever the maintainer decides to take :)

-- 
Valentin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP
  2019-12-05  6:41               ` [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP Valentin Vidic
  2019-12-05 19:34                 ` Jakub Kicinski
@ 2019-12-07  4:17                 ` David Miller
  1 sibling, 0 replies; 17+ messages in thread
From: David Miller @ 2019-12-07  4:17 UTC (permalink / raw)
  To: vvidic
  Cc: jakub.kicinski, willemdebruijn.kernel, borisp, aviadye,
	john.fastabend, daniel, netdev, linux-kernel

From: Valentin Vidic <vvidic@valentin-vidic.from.hr>
Date: Thu,  5 Dec 2019 07:41:18 +0100

> ENOTSUPP is not available in userspace, for example:
> 
>   setsockopt failed, 524, Unknown error 524
> 
> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>

Applied and queued up for -stable, thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 22:44 [PATCH] net/tls: Fix return values for setsockopt Valentin Vidic
2019-12-03 22:55 ` Jakub Kicinski
2019-12-04 18:29   ` [PATCH v2] " Valentin Vidic
2019-12-04 19:22   ` [PATCH] " Willem de Bruijn
2019-12-04 19:35     ` Jakub Kicinski
2019-12-04 20:43       ` Willem de Bruijn
2019-12-04 20:51         ` David Miller
2019-12-04 23:01           ` Jakub Kicinski
2019-12-05  0:55             ` David Miller
2019-12-05  6:41               ` [PATCH v3] net/tls: Fix return values to avoid ENOTSUPP Valentin Vidic
2019-12-05 19:34                 ` Jakub Kicinski
2019-12-05 20:06                   ` Willem de Bruijn
2019-12-05 20:43                     ` Valentin Vidić
2019-12-05 20:45                       ` Jakub Kicinski
2019-12-05 21:26                       ` Willem de Bruijn
2019-12-05 23:08                         ` Valentin Vidić
2019-12-07  4:17                 ` David Miller

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git