| Message ID | 20191205064118.8299-1-vvidic@valentin-vidic.from.hr |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3] net/tls: Fix return values to avoid ENOTSUPP | expand |
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; > } >
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).
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.
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>
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.
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 :)
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.
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);
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(-)