[PATCH] tlshd: Return errnos from QUIC helper functions

[PATCH] tlshd: Return errnos from QUIC helper functions

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Static analysis tools have noticed that
tlshd_quic_serverhello_handshake() expects its helper functions to
return errno values, but two of them return GNUTLS_E_* values in
most cases. Convert those functions to always return errno values
or zero (on success).

Cc: Xin Long <lucien.xin@gmail.com>
Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/src/tlshd/server.c b/src/tlshd/server.c
index 6531f0819d2b..8c8295caedf1 100644
--- a/src/tlshd/server.c
+++ b/src/tlshd/server.c
@@ -575,45 +575,47 @@ static int tlshd_quic_server_set_x509_session(struct tlshd_quic_conn *conn)
 		return ret;
 	}
 
-	ret = gnutls_certificate_allocate_credentials(&cred);
-	if (ret)
+	ret = -ENOMEM;
+	if (gnutls_certificate_allocate_credentials(&cred) != GNUTLS_E_SUCCESS)
 		goto err;
-	ret = tlshd_server_get_truststore(cred);
-	if (ret)
+	ret = -EINVAL;
+	if (tlshd_server_get_truststore(cred) != GNUTLS_E_SUCCESS)
 		goto err;
 
 	gnutls_certificate_set_retrieve_function2(cred, tlshd_x509_retrieve_key_cb);
 
 	gnutls_certificate_set_verify_function(cred, tlshd_quic_server_x509_verify_function);
 
-	ret = gnutls_init(&session, GNUTLS_SERVER | GNUTLS_NO_AUTO_SEND_TICKET |
-				    GNUTLS_ENABLE_EARLY_DATA | GNUTLS_NO_END_OF_EARLY_DATA);
-	if (ret)
+	ret = -ENOMEM;
+	if (gnutls_init(&session, GNUTLS_SERVER | GNUTLS_NO_AUTO_SEND_TICKET |
+			GNUTLS_ENABLE_EARLY_DATA |
+			GNUTLS_NO_END_OF_EARLY_DATA) != GNUTLS_E_SUCCESS)
 		goto err_cred;
 
 	if (!tlshd_quic_server_anti_replay) {
-		ret = gnutls_anti_replay_init(&tlshd_quic_server_anti_replay);
-		if (ret)
+		if (gnutls_anti_replay_init(&tlshd_quic_server_anti_replay) != GNUTLS_E_SUCCESS)
 			goto err_session;
 		gnutls_anti_replay_set_add_function(tlshd_quic_server_anti_replay,
 						    tlshd_quic_server_anti_replay_db_add_func);
 		gnutls_anti_replay_set_ptr(tlshd_quic_server_anti_replay, NULL);
 	}
 	gnutls_anti_replay_enable(session, tlshd_quic_server_anti_replay);
-	ret = gnutls_record_set_max_early_data_size(session, 0xffffffffu);
-	if (ret)
+	ret = -EINVAL;
+	if (gnutls_record_set_max_early_data_size(session,
+						  0xffffffffu) != GNUTLS_E_SUCCESS)
 		goto err_session;
 
 	gnutls_session_set_ptr(session, conn);
 	ticket_key.data = conn->ticket;
 	ticket_key.size = conn->ticket_len;
-	ret = gnutls_session_ticket_enable_server(session, &ticket_key);
-	if (ret)
+	if (gnutls_session_ticket_enable_server(session,
+						&ticket_key) != GNUTLS_E_SUCCESS)
 		goto err_session;
 	gnutls_handshake_set_hook_function(session, GNUTLS_HANDSHAKE_CLIENT_HELLO,
 					   GNUTLS_HOOK_POST, tlshd_quic_server_alpn_verify);
-	ret = gnutls_credentials_set(session, GNUTLS_CRD_CERTIFICATE, cred);
-	if (ret)
+	ret = -ENOMEM;
+	if (gnutls_credentials_set(session, GNUTLS_CRD_CERTIFICATE,
+				   cred) != GNUTLS_E_SUCCESS)
 		goto err_session;
 	gnutls_certificate_server_set_request(session, conn->cert_req);
 
@@ -635,21 +637,29 @@ static int tlshd_quic_server_set_psk_session(struct tlshd_quic_conn *conn)
 {
 	gnutls_psk_server_credentials_t cred;
 	gnutls_session_t session;
-	int ret;
+	int ret, err;
 
-	ret = gnutls_psk_allocate_server_credentials(&cred);
-	if (ret)
+	switch (gnutls_psk_allocate_server_credentials(&cred)) {
+	case GNUTLS_E_SUCCESS:
+		break;
+	case GNUTLS_E_MEMORY_ERROR:
+		ret = -ENOMEM;
 		goto err;
+	case GNUTLS_E_INVALID_REQUEST:
+		ret = -EINVAL;
+		goto err;
+	}
 	gnutls_psk_set_server_credentials_function(cred, tlshd_quic_server_psk_cb);
 
-	ret = gnutls_init(&session, GNUTLS_SERVER | GNUTLS_NO_AUTO_SEND_TICKET);
-	if (ret)
+	ret = -ENOMEM;
+	if (gnutls_init(&session, GNUTLS_SERVER |
+			GNUTLS_NO_AUTO_SEND_TICKET) != GNUTLS_E_SUCCESS)
 		goto err_cred;
 	gnutls_session_set_ptr(session, conn);
 	gnutls_handshake_set_hook_function(session, GNUTLS_HANDSHAKE_CLIENT_HELLO,
 					   GNUTLS_HOOK_POST, tlshd_quic_server_alpn_verify);
-	ret = gnutls_credentials_set(session, GNUTLS_CRD_PSK, cred);
-	if (ret)
+	if (gnutls_credentials_set(session, GNUTLS_CRD_PSK,
+				   cred) != GNUTLS_E_SUCCESS)
 		goto err_session;
 
 	conn->is_serv = 1;
-- 
2.51.0

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Chuck Lever]

On 9/22/25 3:07 PM, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Static analysis tools have noticed that
> tlshd_quic_serverhello_handshake() expects its helper functions to
> return errno values, but two of them return GNUTLS_E_* values in
> most cases. Convert those functions to always return errno values
> or zero (on success).
> 
> Cc: Xin Long <lucien.xin@gmail.com>
> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)

Xin -

Looks like src/tlshd/quic.c has the same issue with conn->errcode.
Callers expect an errno, but internally the utility functions set that
field to a GNUTLS_E_ value.

Can you apply this patch, fix up quic.c as well, and then test both and
post the fixes here?


-- 
Chuck Lever

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Xin Long]

On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 9/22/25 3:07 PM, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> >
> > Static analysis tools have noticed that
> > tlshd_quic_serverhello_handshake() expects its helper functions to
> > return errno values, but two of them return GNUTLS_E_* values in
> > most cases. Convert those functions to always return errno values
> > or zero (on success).
> >
> > Cc: Xin Long <lucien.xin@gmail.com>
> > Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
> >  1 file changed, 32 insertions(+), 22 deletions(-)
>
> Xin -
>
> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
> Callers expect an errno, but internally the utility functions set that
> field to a GNUTLS_E_ value.
>
> Can you apply this patch, fix up quic.c as well, and then test both and
> post the fixes here?
>
Sure.

But conn->errcode is used to pass the err to parms->session_status.
What does parms->session_status expect? errno or GNUTLS_E_ value?
looking at tlshd_start_tls_handshake():
                /* Any errors here should default to blocking access: */
                parms->session_status = EACCES; <---
                switch (ret) {
                case GNUTLS_E_CERTIFICATE_ERROR:
                case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
                        tlshd_log_cert_verification_error(session);
                        break;
                case -ETIMEDOUT:
                        tlshd_log_gnutls_error(ret);
                        parms->session_status = -ret; <----
        parms->session_status = tlshd_initialize_ktls(session);

It also seems mixed, no?

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Chuck Lever]

On 9/23/25 1:27 PM, Xin Long wrote:
> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 9/22/25 3:07 PM, Chuck Lever wrote:
>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>
>>> Static analysis tools have noticed that
>>> tlshd_quic_serverhello_handshake() expects its helper functions to
>>> return errno values, but two of them return GNUTLS_E_* values in
>>> most cases. Convert those functions to always return errno values
>>> or zero (on success).
>>>
>>> Cc: Xin Long <lucien.xin@gmail.com>
>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
>>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> Xin -
>>
>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
>> Callers expect an errno, but internally the utility functions set that
>> field to a GNUTLS_E_ value.
>>
>> Can you apply this patch, fix up quic.c as well, and then test both and
>> post the fixes here?
>>
> Sure.
> 
> But conn->errcode is used to pass the err to parms->session_status.
> What does parms->session_status expect? errno or GNUTLS_E_ value?

session_status contains a positive errno or zero.


> looking at tlshd_start_tls_handshake():
>                 /* Any errors here should default to blocking access: */
>                 parms->session_status = EACCES; <---
>                 switch (ret) {
>                 case GNUTLS_E_CERTIFICATE_ERROR:
>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
>                         tlshd_log_cert_verification_error(session);
>                         break;
>                 case -ETIMEDOUT:
>                         tlshd_log_gnutls_error(ret);
>                         parms->session_status = -ret; <----
>         parms->session_status = tlshd_initialize_ktls(session);
> 
> It also seems mixed, no?

The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.

This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
gnutls to kernel"). Passing -ETIMEDOUT directly to
tlshd_log_gnutls_error() does seem a little bogus.


-- 
Chuck Lever

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Xin Long]

On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 9/23/25 1:27 PM, Xin Long wrote:
> > On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 9/22/25 3:07 PM, Chuck Lever wrote:
> >>> From: Chuck Lever <chuck.lever@oracle.com>
> >>>
> >>> Static analysis tools have noticed that
> >>> tlshd_quic_serverhello_handshake() expects its helper functions to
> >>> return errno values, but two of them return GNUTLS_E_* values in
> >>> most cases. Convert those functions to always return errno values
> >>> or zero (on success).
> >>>
> >>> Cc: Xin Long <lucien.xin@gmail.com>
> >>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
> >>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>> ---
> >>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
> >>>  1 file changed, 32 insertions(+), 22 deletions(-)
> >>
> >> Xin -
> >>
> >> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
> >> Callers expect an errno, but internally the utility functions set that
> >> field to a GNUTLS_E_ value.
> >>
> >> Can you apply this patch, fix up quic.c as well, and then test both and
> >> post the fixes here?
> >>
> > Sure.
> >
> > But conn->errcode is used to pass the err to parms->session_status.
> > What does parms->session_status expect? errno or GNUTLS_E_ value?
>
> session_status contains a positive errno or zero.
>
>
> > looking at tlshd_start_tls_handshake():
> >                 /* Any errors here should default to blocking access: */
> >                 parms->session_status = EACCES; <---
> >                 switch (ret) {
> >                 case GNUTLS_E_CERTIFICATE_ERROR:
> >                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
> >                         tlshd_log_cert_verification_error(session);
> >                         break;
> >                 case -ETIMEDOUT:
> >                         tlshd_log_gnutls_error(ret);
> >                         parms->session_status = -ret; <----
> >         parms->session_status = tlshd_initialize_ktls(session);
> >
> > It also seems mixed, no?
>
> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
>
> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
> gnutls to kernel"). Passing -ETIMEDOUT directly to
> tlshd_log_gnutls_error() does seem a little bogus.
>

Got it. I'm trying to align the QUIC err process with tls13 code.

- tlshd_tls13_client_x509_handshake()
- tlshd_tls13_client_psk_handshake()

- tlshd_tls13_server_x509_handshake()
- tlshd_tls13_server_psk_handshake()

It seems to me that: When gnutls_xxx() returns errs in these functions,
it only logs them, and doesn't set any value to parms->session_status,
but sets it until tlshd_start_tls_handshake().

So my suggestion is: not touch these functions for QUIC code:

- tlshd_quic_client_set_x509_session()
- tlshd_quic_client_set_psk_session()

- tlshd_quic_server_set_x509_session()
- tlshd_quic_server_set_psk_session()

but change their callers tlshd_quic_client/serverhello_handshake():

@@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
tlshd_handshake_parms *parms)
                ret = -EINVAL;
                tlshd_log_debug("Unrecognized auth mode (%d)",
parms->auth_mode);
        }
-       if (ret) {
-               conn->errcode = -ret;
-               goto out;
+       if (!ret) {
+               tlshd_quic_start_handshake(conn);
+               parms->session_status = conn->errcode;
        }
-
-       tlshd_quic_start_handshake(conn);
-out:
-       parms->session_status = conn->errcode;
        tlshd_quic_conn_destroy(conn);

and then in tlshd_quic_start_handshake(), keep the code setting
conn->errcode = errno, but set conn->errcode = EACCES for all
GNUTLS_E_ places.

What do you think?

Thanks.

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Chuck Lever]

On 9/23/25 3:41 PM, Xin Long wrote:
> On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 9/23/25 1:27 PM, Xin Long wrote:
>>> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 9/22/25 3:07 PM, Chuck Lever wrote:
>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>>
>>>>> Static analysis tools have noticed that
>>>>> tlshd_quic_serverhello_handshake() expects its helper functions to
>>>>> return errno values, but two of them return GNUTLS_E_* values in
>>>>> most cases. Convert those functions to always return errno values
>>>>> or zero (on success).
>>>>>
>>>>> Cc: Xin Long <lucien.xin@gmail.com>
>>>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
>>>>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>>>
>>>> Xin -
>>>>
>>>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
>>>> Callers expect an errno, but internally the utility functions set that
>>>> field to a GNUTLS_E_ value.
>>>>
>>>> Can you apply this patch, fix up quic.c as well, and then test both and
>>>> post the fixes here?
>>>>
>>> Sure.
>>>
>>> But conn->errcode is used to pass the err to parms->session_status.
>>> What does parms->session_status expect? errno or GNUTLS_E_ value?
>>
>> session_status contains a positive errno or zero.
>>
>>
>>> looking at tlshd_start_tls_handshake():
>>>                 /* Any errors here should default to blocking access: */
>>>                 parms->session_status = EACCES; <---
>>>                 switch (ret) {
>>>                 case GNUTLS_E_CERTIFICATE_ERROR:
>>>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
>>>                         tlshd_log_cert_verification_error(session);
>>>                         break;
>>>                 case -ETIMEDOUT:
>>>                         tlshd_log_gnutls_error(ret);
>>>                         parms->session_status = -ret; <----
>>>         parms->session_status = tlshd_initialize_ktls(session);
>>>
>>> It also seems mixed, no?
>>
>> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
>> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
>>
>> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
>> gnutls to kernel"). Passing -ETIMEDOUT directly to
>> tlshd_log_gnutls_error() does seem a little bogus.
>>
> 
> Got it. I'm trying to align the QUIC err process with tls13 code.
> 
> - tlshd_tls13_client_x509_handshake()
> - tlshd_tls13_client_psk_handshake()
> 
> - tlshd_tls13_server_x509_handshake()
> - tlshd_tls13_server_psk_handshake()
> 
> It seems to me that: When gnutls_xxx() returns errs in these functions,
> it only logs them, and doesn't set any value to parms->session_status,
> but sets it until tlshd_start_tls_handshake().
> 
> So my suggestion is: not touch these functions for QUIC code:
> 
> - tlshd_quic_client_set_x509_session()
> - tlshd_quic_client_set_psk_session()
> 
> - tlshd_quic_server_set_x509_session()
> - tlshd_quic_server_set_psk_session()
> 
> but change their callers tlshd_quic_client/serverhello_handshake():
> 
> @@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
> tlshd_handshake_parms *parms)
>                 ret = -EINVAL;
>                 tlshd_log_debug("Unrecognized auth mode (%d)",
> parms->auth_mode);
>         }
> -       if (ret) {
> -               conn->errcode = -ret;
> -               goto out;
> +       if (!ret) {

   if (ret == GNUTLS_E_SUCCESSS) {

is preferred.


> +               tlshd_quic_start_handshake(conn);
> +               parms->session_status = conn->errcode;
>         }
> -
> -       tlshd_quic_start_handshake(conn);
> -out:
> -       parms->session_status = conn->errcode;
>         tlshd_quic_conn_destroy(conn);

We usually express the return code checking as

   do something
   if (error) {
      error flow
   }

That's why the goto is in there. But I'll leave it up to you in
the quic code.


> and then in tlshd_quic_start_handshake(), keep the code setting
> conn->errcode = errno, but set conn->errcode = EACCES for all
> GNUTLS_E_ places.
> 
> What do you think?

I think that still mixes the error return values for the internal
functions?

The reason I noticed this is I'm trying to add proper Doxygen
comments in here. The internal functions need to be consistent about
returning either only GNUTLS_E or errno. The documenting comments
can't say "returns GNUTLS_E, negative errno, or zero", for example.


-- 
Chuck Lever

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Xin Long]

On Tue, Sep 23, 2025 at 7:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 9/23/25 3:41 PM, Xin Long wrote:
> > On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 9/23/25 1:27 PM, Xin Long wrote:
> >>> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> On 9/22/25 3:07 PM, Chuck Lever wrote:
> >>>>> From: Chuck Lever <chuck.lever@oracle.com>
> >>>>>
> >>>>> Static analysis tools have noticed that
> >>>>> tlshd_quic_serverhello_handshake() expects its helper functions to
> >>>>> return errno values, but two of them return GNUTLS_E_* values in
> >>>>> most cases. Convert those functions to always return errno values
> >>>>> or zero (on success).
> >>>>>
> >>>>> Cc: Xin Long <lucien.xin@gmail.com>
> >>>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
> >>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>> ---
> >>>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
> >>>>>  1 file changed, 32 insertions(+), 22 deletions(-)
> >>>>
> >>>> Xin -
> >>>>
> >>>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
> >>>> Callers expect an errno, but internally the utility functions set that
> >>>> field to a GNUTLS_E_ value.
> >>>>
> >>>> Can you apply this patch, fix up quic.c as well, and then test both and
> >>>> post the fixes here?
> >>>>
> >>> Sure.
> >>>
> >>> But conn->errcode is used to pass the err to parms->session_status.
> >>> What does parms->session_status expect? errno or GNUTLS_E_ value?
> >>
> >> session_status contains a positive errno or zero.
> >>
> >>
> >>> looking at tlshd_start_tls_handshake():
> >>>                 /* Any errors here should default to blocking access: */
> >>>                 parms->session_status = EACCES; <---
> >>>                 switch (ret) {
> >>>                 case GNUTLS_E_CERTIFICATE_ERROR:
> >>>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
> >>>                         tlshd_log_cert_verification_error(session);
> >>>                         break;
> >>>                 case -ETIMEDOUT:
> >>>                         tlshd_log_gnutls_error(ret);
> >>>                         parms->session_status = -ret; <----
> >>>         parms->session_status = tlshd_initialize_ktls(session);
> >>>
> >>> It also seems mixed, no?
> >>
> >> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
> >> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
> >>
> >> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
> >> gnutls to kernel"). Passing -ETIMEDOUT directly to
> >> tlshd_log_gnutls_error() does seem a little bogus.
> >>
> >
> > Got it. I'm trying to align the QUIC err process with tls13 code.
> >
> > - tlshd_tls13_client_x509_handshake()
> > - tlshd_tls13_client_psk_handshake()
> >
> > - tlshd_tls13_server_x509_handshake()
> > - tlshd_tls13_server_psk_handshake()
> >
> > It seems to me that: When gnutls_xxx() returns errs in these functions,
> > it only logs them, and doesn't set any value to parms->session_status,
> > but sets it until tlshd_start_tls_handshake().
> >
> > So my suggestion is: not touch these functions for QUIC code:
> >
> > - tlshd_quic_client_set_x509_session()
> > - tlshd_quic_client_set_psk_session()
> >
> > - tlshd_quic_server_set_x509_session()
> > - tlshd_quic_server_set_psk_session()
> >
> > but change their callers tlshd_quic_client/serverhello_handshake():
> >
> > @@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
> > tlshd_handshake_parms *parms)
> >                 ret = -EINVAL;
> >                 tlshd_log_debug("Unrecognized auth mode (%d)",
> > parms->auth_mode);
> >         }
> > -       if (ret) {
> > -               conn->errcode = -ret;
> > -               goto out;
> > +       if (!ret) {
>
>    if (ret == GNUTLS_E_SUCCESSS) {
>
> is preferred.
>
>
> > +               tlshd_quic_start_handshake(conn);
> > +               parms->session_status = conn->errcode;
> >         }
> > -
> > -       tlshd_quic_start_handshake(conn);
> > -out:
> > -       parms->session_status = conn->errcode;
> >         tlshd_quic_conn_destroy(conn);
>
> We usually express the return code checking as
>
>    do something
>    if (error) {
>       error flow
>    }
>
> That's why the goto is in there. But I'll leave it up to you in
> the quic code.
>
>
> > and then in tlshd_quic_start_handshake(), keep the code setting
> > conn->errcode = errno, but set conn->errcode = EACCES for all
> > GNUTLS_E_ places.
> >
> > What do you think?
>
> I think that still mixes the error return values for the internal
> functions?
>
> The reason I noticed this is I'm trying to add proper Doxygen
> comments in here. The internal functions need to be consistent about
> returning either only GNUTLS_E or errno. The documenting comments
> can't say "returns GNUTLS_E, negative errno, or zero", for example.
>
I see, how about if I do this in all GNUTLS_E returned functions?

diff --git a/src/tlshd/server.c b/src/tlshd/server.c
index db5db31..0ee1157 100644
--- a/src/tlshd/server.c
+++ b/src/tlshd/server.c
@@ -538,7 +538,7 @@ err:
        tlshd_x509_server_put_privkey();
        tlshd_x509_server_put_certs();
        tlshd_log_gnutls_error(ret);
-       return ret;
+       return ret ? -EACCES : 0;
 }

This way, we will not need to change their callers.

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Chuck Lever]

On 9/23/25 5:00 PM, Xin Long wrote:
> On Tue, Sep 23, 2025 at 7:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 9/23/25 3:41 PM, Xin Long wrote:
>>> On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 9/23/25 1:27 PM, Xin Long wrote:
>>>>> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>
>>>>>> On 9/22/25 3:07 PM, Chuck Lever wrote:
>>>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>>>>
>>>>>>> Static analysis tools have noticed that
>>>>>>> tlshd_quic_serverhello_handshake() expects its helper functions to
>>>>>>> return errno values, but two of them return GNUTLS_E_* values in
>>>>>>> most cases. Convert those functions to always return errno values
>>>>>>> or zero (on success).
>>>>>>>
>>>>>>> Cc: Xin Long <lucien.xin@gmail.com>
>>>>>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>>> ---
>>>>>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
>>>>>>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> Xin -
>>>>>>
>>>>>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
>>>>>> Callers expect an errno, but internally the utility functions set that
>>>>>> field to a GNUTLS_E_ value.
>>>>>>
>>>>>> Can you apply this patch, fix up quic.c as well, and then test both and
>>>>>> post the fixes here?
>>>>>>
>>>>> Sure.
>>>>>
>>>>> But conn->errcode is used to pass the err to parms->session_status.
>>>>> What does parms->session_status expect? errno or GNUTLS_E_ value?
>>>>
>>>> session_status contains a positive errno or zero.
>>>>
>>>>
>>>>> looking at tlshd_start_tls_handshake():
>>>>>                 /* Any errors here should default to blocking access: */
>>>>>                 parms->session_status = EACCES; <---
>>>>>                 switch (ret) {
>>>>>                 case GNUTLS_E_CERTIFICATE_ERROR:
>>>>>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
>>>>>                         tlshd_log_cert_verification_error(session);
>>>>>                         break;
>>>>>                 case -ETIMEDOUT:
>>>>>                         tlshd_log_gnutls_error(ret);
>>>>>                         parms->session_status = -ret; <----
>>>>>         parms->session_status = tlshd_initialize_ktls(session);
>>>>>
>>>>> It also seems mixed, no?
>>>>
>>>> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
>>>> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
>>>>
>>>> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
>>>> gnutls to kernel"). Passing -ETIMEDOUT directly to
>>>> tlshd_log_gnutls_error() does seem a little bogus.
>>>>
>>>
>>> Got it. I'm trying to align the QUIC err process with tls13 code.
>>>
>>> - tlshd_tls13_client_x509_handshake()
>>> - tlshd_tls13_client_psk_handshake()
>>>
>>> - tlshd_tls13_server_x509_handshake()
>>> - tlshd_tls13_server_psk_handshake()
>>>
>>> It seems to me that: When gnutls_xxx() returns errs in these functions,
>>> it only logs them, and doesn't set any value to parms->session_status,
>>> but sets it until tlshd_start_tls_handshake().
>>>
>>> So my suggestion is: not touch these functions for QUIC code:
>>>
>>> - tlshd_quic_client_set_x509_session()
>>> - tlshd_quic_client_set_psk_session()
>>>
>>> - tlshd_quic_server_set_x509_session()
>>> - tlshd_quic_server_set_psk_session()
>>>
>>> but change their callers tlshd_quic_client/serverhello_handshake():
>>>
>>> @@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
>>> tlshd_handshake_parms *parms)
>>>                 ret = -EINVAL;
>>>                 tlshd_log_debug("Unrecognized auth mode (%d)",
>>> parms->auth_mode);
>>>         }
>>> -       if (ret) {
>>> -               conn->errcode = -ret;
>>> -               goto out;
>>> +       if (!ret) {
>>
>>    if (ret == GNUTLS_E_SUCCESSS) {
>>
>> is preferred.
>>
>>
>>> +               tlshd_quic_start_handshake(conn);
>>> +               parms->session_status = conn->errcode;
>>>         }
>>> -
>>> -       tlshd_quic_start_handshake(conn);
>>> -out:
>>> -       parms->session_status = conn->errcode;
>>>         tlshd_quic_conn_destroy(conn);
>>
>> We usually express the return code checking as
>>
>>    do something
>>    if (error) {
>>       error flow
>>    }
>>
>> That's why the goto is in there. But I'll leave it up to you in
>> the quic code.
>>
>>
>>> and then in tlshd_quic_start_handshake(), keep the code setting
>>> conn->errcode = errno, but set conn->errcode = EACCES for all
>>> GNUTLS_E_ places.
>>>
>>> What do you think?
>>
>> I think that still mixes the error return values for the internal
>> functions?
>>
>> The reason I noticed this is I'm trying to add proper Doxygen
>> comments in here. The internal functions need to be consistent about
>> returning either only GNUTLS_E or errno. The documenting comments
>> can't say "returns GNUTLS_E, negative errno, or zero", for example.
>>
> I see, how about if I do this in all GNUTLS_E returned functions?
> 
> diff --git a/src/tlshd/server.c b/src/tlshd/server.c
> index db5db31..0ee1157 100644
> --- a/src/tlshd/server.c
> +++ b/src/tlshd/server.c
> @@ -538,7 +538,7 @@ err:
>         tlshd_x509_server_put_privkey();
>         tlshd_x509_server_put_certs();
>         tlshd_log_gnutls_error(ret);
> -       return ret;
> +       return ret ? -EACCES : 0;
>  }
> 
> This way, we will not need to change their callers.

IMHO we want to distinguish between a temporary memory allocation
failure, invalid input, and a permanent access failure.


-- 
Chuck Lever

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Xin Long]

On Tue, Sep 23, 2025 at 8:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 9/23/25 5:00 PM, Xin Long wrote:
> > On Tue, Sep 23, 2025 at 7:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 9/23/25 3:41 PM, Xin Long wrote:
> >>> On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> On 9/23/25 1:27 PM, Xin Long wrote:
> >>>>> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>>
> >>>>>> On 9/22/25 3:07 PM, Chuck Lever wrote:
> >>>>>>> From: Chuck Lever <chuck.lever@oracle.com>
> >>>>>>>
> >>>>>>> Static analysis tools have noticed that
> >>>>>>> tlshd_quic_serverhello_handshake() expects its helper functions to
> >>>>>>> return errno values, but two of them return GNUTLS_E_* values in
> >>>>>>> most cases. Convert those functions to always return errno values
> >>>>>>> or zero (on success).
> >>>>>>>
> >>>>>>> Cc: Xin Long <lucien.xin@gmail.com>
> >>>>>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
> >>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>>>> ---
> >>>>>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
> >>>>>>>  1 file changed, 32 insertions(+), 22 deletions(-)
> >>>>>>
> >>>>>> Xin -
> >>>>>>
> >>>>>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
> >>>>>> Callers expect an errno, but internally the utility functions set that
> >>>>>> field to a GNUTLS_E_ value.
> >>>>>>
> >>>>>> Can you apply this patch, fix up quic.c as well, and then test both and
> >>>>>> post the fixes here?
> >>>>>>
> >>>>> Sure.
> >>>>>
> >>>>> But conn->errcode is used to pass the err to parms->session_status.
> >>>>> What does parms->session_status expect? errno or GNUTLS_E_ value?
> >>>>
> >>>> session_status contains a positive errno or zero.
> >>>>
> >>>>
> >>>>> looking at tlshd_start_tls_handshake():
> >>>>>                 /* Any errors here should default to blocking access: */
> >>>>>                 parms->session_status = EACCES; <---
> >>>>>                 switch (ret) {
> >>>>>                 case GNUTLS_E_CERTIFICATE_ERROR:
> >>>>>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
> >>>>>                         tlshd_log_cert_verification_error(session);
> >>>>>                         break;
> >>>>>                 case -ETIMEDOUT:
> >>>>>                         tlshd_log_gnutls_error(ret);
> >>>>>                         parms->session_status = -ret; <----
> >>>>>         parms->session_status = tlshd_initialize_ktls(session);
> >>>>>
> >>>>> It also seems mixed, no?
> >>>>
> >>>> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
> >>>> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
> >>>>
> >>>> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
> >>>> gnutls to kernel"). Passing -ETIMEDOUT directly to
> >>>> tlshd_log_gnutls_error() does seem a little bogus.
> >>>>
> >>>
> >>> Got it. I'm trying to align the QUIC err process with tls13 code.
> >>>
> >>> - tlshd_tls13_client_x509_handshake()
> >>> - tlshd_tls13_client_psk_handshake()
> >>>
> >>> - tlshd_tls13_server_x509_handshake()
> >>> - tlshd_tls13_server_psk_handshake()
> >>>
> >>> It seems to me that: When gnutls_xxx() returns errs in these functions,
> >>> it only logs them, and doesn't set any value to parms->session_status,
> >>> but sets it until tlshd_start_tls_handshake().
> >>>
> >>> So my suggestion is: not touch these functions for QUIC code:
> >>>
> >>> - tlshd_quic_client_set_x509_session()
> >>> - tlshd_quic_client_set_psk_session()
> >>>
> >>> - tlshd_quic_server_set_x509_session()
> >>> - tlshd_quic_server_set_psk_session()
> >>>
> >>> but change their callers tlshd_quic_client/serverhello_handshake():
> >>>
> >>> @@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
> >>> tlshd_handshake_parms *parms)
> >>>                 ret = -EINVAL;
> >>>                 tlshd_log_debug("Unrecognized auth mode (%d)",
> >>> parms->auth_mode);
> >>>         }
> >>> -       if (ret) {
> >>> -               conn->errcode = -ret;
> >>> -               goto out;
> >>> +       if (!ret) {
> >>
> >>    if (ret == GNUTLS_E_SUCCESSS) {
> >>
> >> is preferred.
> >>
> >>
> >>> +               tlshd_quic_start_handshake(conn);
> >>> +               parms->session_status = conn->errcode;
> >>>         }
> >>> -
> >>> -       tlshd_quic_start_handshake(conn);
> >>> -out:
> >>> -       parms->session_status = conn->errcode;
> >>>         tlshd_quic_conn_destroy(conn);
> >>
> >> We usually express the return code checking as
> >>
> >>    do something
> >>    if (error) {
> >>       error flow
> >>    }
> >>
> >> That's why the goto is in there. But I'll leave it up to you in
> >> the quic code.
> >>
> >>
> >>> and then in tlshd_quic_start_handshake(), keep the code setting
> >>> conn->errcode = errno, but set conn->errcode = EACCES for all
> >>> GNUTLS_E_ places.
> >>>
> >>> What do you think?
> >>
> >> I think that still mixes the error return values for the internal
> >> functions?
> >>
> >> The reason I noticed this is I'm trying to add proper Doxygen
> >> comments in here. The internal functions need to be consistent about
> >> returning either only GNUTLS_E or errno. The documenting comments
> >> can't say "returns GNUTLS_E, negative errno, or zero", for example.
> >>
> > I see, how about if I do this in all GNUTLS_E returned functions?
> >
> > diff --git a/src/tlshd/server.c b/src/tlshd/server.c
> > index db5db31..0ee1157 100644
> > --- a/src/tlshd/server.c
> > +++ b/src/tlshd/server.c
> > @@ -538,7 +538,7 @@ err:
> >         tlshd_x509_server_put_privkey();
> >         tlshd_x509_server_put_certs();
> >         tlshd_log_gnutls_error(ret);
> > -       return ret;
> > +       return ret ? -EACCES : 0;
> >  }
> >
> > This way, we will not need to change their callers.
>
> IMHO we want to distinguish between a temporary memory allocation
> failure, invalid input, and a permanent access failure.
>
Yes, that will be nice.

However, the tlshd_tls13_* functions don’t currently convert GNUTLS_E
values into errno for parms->session_status; they only log them via
tlshd_log_gnutls_error(ret).

If we adopt your original patch (which kind of maps GNUTLS_E values to
errno and  assigns them to parms->session_status) for QUIC, it would make
the tlshd_quic_* functions diverge in behavior from the tlshd_tls13_*
functions.

Or you think we should make this change in quic first and align tls13 later?

Sorry for the confusion.

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Chuck Lever]

On 9/23/25 6:40 PM, Xin Long wrote:
> On Tue, Sep 23, 2025 at 8:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 9/23/25 5:00 PM, Xin Long wrote:
>>> On Tue, Sep 23, 2025 at 7:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 9/23/25 3:41 PM, Xin Long wrote:
>>>>> On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>
>>>>>> On 9/23/25 1:27 PM, Xin Long wrote:
>>>>>>> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>>>>>
>>>>>>>> On 9/22/25 3:07 PM, Chuck Lever wrote:
>>>>>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>>>>>>
>>>>>>>>> Static analysis tools have noticed that
>>>>>>>>> tlshd_quic_serverhello_handshake() expects its helper functions to
>>>>>>>>> return errno values, but two of them return GNUTLS_E_* values in
>>>>>>>>> most cases. Convert those functions to always return errno values
>>>>>>>>> or zero (on success).
>>>>>>>>>
>>>>>>>>> Cc: Xin Long <lucien.xin@gmail.com>
>>>>>>>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
>>>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>>>>> ---
>>>>>>>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
>>>>>>>>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>>>>>>>
>>>>>>>> Xin -
>>>>>>>>
>>>>>>>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
>>>>>>>> Callers expect an errno, but internally the utility functions set that
>>>>>>>> field to a GNUTLS_E_ value.
>>>>>>>>
>>>>>>>> Can you apply this patch, fix up quic.c as well, and then test both and
>>>>>>>> post the fixes here?
>>>>>>>>
>>>>>>> Sure.
>>>>>>>
>>>>>>> But conn->errcode is used to pass the err to parms->session_status.
>>>>>>> What does parms->session_status expect? errno or GNUTLS_E_ value?
>>>>>>
>>>>>> session_status contains a positive errno or zero.
>>>>>>
>>>>>>
>>>>>>> looking at tlshd_start_tls_handshake():
>>>>>>>                 /* Any errors here should default to blocking access: */
>>>>>>>                 parms->session_status = EACCES; <---
>>>>>>>                 switch (ret) {
>>>>>>>                 case GNUTLS_E_CERTIFICATE_ERROR:
>>>>>>>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
>>>>>>>                         tlshd_log_cert_verification_error(session);
>>>>>>>                         break;
>>>>>>>                 case -ETIMEDOUT:
>>>>>>>                         tlshd_log_gnutls_error(ret);
>>>>>>>                         parms->session_status = -ret; <----
>>>>>>>         parms->session_status = tlshd_initialize_ktls(session);
>>>>>>>
>>>>>>> It also seems mixed, no?
>>>>>>
>>>>>> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
>>>>>> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
>>>>>>
>>>>>> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
>>>>>> gnutls to kernel"). Passing -ETIMEDOUT directly to
>>>>>> tlshd_log_gnutls_error() does seem a little bogus.
>>>>>>
>>>>>
>>>>> Got it. I'm trying to align the QUIC err process with tls13 code.
>>>>>
>>>>> - tlshd_tls13_client_x509_handshake()
>>>>> - tlshd_tls13_client_psk_handshake()
>>>>>
>>>>> - tlshd_tls13_server_x509_handshake()
>>>>> - tlshd_tls13_server_psk_handshake()
>>>>>
>>>>> It seems to me that: When gnutls_xxx() returns errs in these functions,
>>>>> it only logs them, and doesn't set any value to parms->session_status,
>>>>> but sets it until tlshd_start_tls_handshake().
>>>>>
>>>>> So my suggestion is: not touch these functions for QUIC code:
>>>>>
>>>>> - tlshd_quic_client_set_x509_session()
>>>>> - tlshd_quic_client_set_psk_session()
>>>>>
>>>>> - tlshd_quic_server_set_x509_session()
>>>>> - tlshd_quic_server_set_psk_session()
>>>>>
>>>>> but change their callers tlshd_quic_client/serverhello_handshake():
>>>>>
>>>>> @@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
>>>>> tlshd_handshake_parms *parms)
>>>>>                 ret = -EINVAL;
>>>>>                 tlshd_log_debug("Unrecognized auth mode (%d)",
>>>>> parms->auth_mode);
>>>>>         }
>>>>> -       if (ret) {
>>>>> -               conn->errcode = -ret;
>>>>> -               goto out;
>>>>> +       if (!ret) {
>>>>
>>>>    if (ret == GNUTLS_E_SUCCESSS) {
>>>>
>>>> is preferred.
>>>>
>>>>
>>>>> +               tlshd_quic_start_handshake(conn);
>>>>> +               parms->session_status = conn->errcode;
>>>>>         }
>>>>> -
>>>>> -       tlshd_quic_start_handshake(conn);
>>>>> -out:
>>>>> -       parms->session_status = conn->errcode;
>>>>>         tlshd_quic_conn_destroy(conn);
>>>>
>>>> We usually express the return code checking as
>>>>
>>>>    do something
>>>>    if (error) {
>>>>       error flow
>>>>    }
>>>>
>>>> That's why the goto is in there. But I'll leave it up to you in
>>>> the quic code.
>>>>
>>>>
>>>>> and then in tlshd_quic_start_handshake(), keep the code setting
>>>>> conn->errcode = errno, but set conn->errcode = EACCES for all
>>>>> GNUTLS_E_ places.
>>>>>
>>>>> What do you think?
>>>>
>>>> I think that still mixes the error return values for the internal
>>>> functions?
>>>>
>>>> The reason I noticed this is I'm trying to add proper Doxygen
>>>> comments in here. The internal functions need to be consistent about
>>>> returning either only GNUTLS_E or errno. The documenting comments
>>>> can't say "returns GNUTLS_E, negative errno, or zero", for example.
>>>>
>>> I see, how about if I do this in all GNUTLS_E returned functions?
>>>
>>> diff --git a/src/tlshd/server.c b/src/tlshd/server.c
>>> index db5db31..0ee1157 100644
>>> --- a/src/tlshd/server.c
>>> +++ b/src/tlshd/server.c
>>> @@ -538,7 +538,7 @@ err:
>>>         tlshd_x509_server_put_privkey();
>>>         tlshd_x509_server_put_certs();
>>>         tlshd_log_gnutls_error(ret);
>>> -       return ret;
>>> +       return ret ? -EACCES : 0;
>>>  }
>>>
>>> This way, we will not need to change their callers.
>>
>> IMHO we want to distinguish between a temporary memory allocation
>> failure, invalid input, and a permanent access failure.
>>
> Yes, that will be nice.

Perhaps all that matters here is the audit message -- the admin will
want to know what kind of error it was. tlshd recovery is just clean
up and exit for all three.


> However, the tlshd_tls13_* functions don’t currently convert GNUTLS_E
> values into errno for parms->session_status; they only log them via
> tlshd_log_gnutls_error(ret).

The tlshd_tls13_ helpers return void, IIRC, because the session_status
field is initialized to EIO, and that gets cleared by
tlshd_start_tls_handshake() when the handshake is successful.

The quic helpers return an int... maybe they should return void as well?


> If we adopt your original patch (which kind of maps GNUTLS_E values to
> errno and  assigns them to parms->session_status) for QUIC, it would make
> the tlshd_quic_* functions diverge in behavior from the tlshd_tls13_*
> functions.
> 
> Or you think we should make this change in quic first and align tls13 later?
> 
> Sorry for the confusion.

Confusion is mostly because I don't remember how any of this works.


-- 
Chuck Lever

Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Xin Long]

On Wed, Sep 24, 2025 at 9:47 AM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On 9/23/25 6:40 PM, Xin Long wrote:
> > On Tue, Sep 23, 2025 at 8:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>
> >> On 9/23/25 5:00 PM, Xin Long wrote:
> >>> On Tue, Sep 23, 2025 at 7:05 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>
> >>>> On 9/23/25 3:41 PM, Xin Long wrote:
> >>>>> On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>>
> >>>>>> On 9/23/25 1:27 PM, Xin Long wrote:
> >>>>>>> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>>>>>>
> >>>>>>>> On 9/22/25 3:07 PM, Chuck Lever wrote:
> >>>>>>>>> From: Chuck Lever <chuck.lever@oracle.com>
> >>>>>>>>>
> >>>>>>>>> Static analysis tools have noticed that
> >>>>>>>>> tlshd_quic_serverhello_handshake() expects its helper functions to
> >>>>>>>>> return errno values, but two of them return GNUTLS_E_* values in
> >>>>>>>>> most cases. Convert those functions to always return errno values
> >>>>>>>>> or zero (on success).
> >>>>>>>>>
> >>>>>>>>> Cc: Xin Long <lucien.xin@gmail.com>
> >>>>>>>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
> >>>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>>>>>> ---
> >>>>>>>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
> >>>>>>>>>  1 file changed, 32 insertions(+), 22 deletions(-)
> >>>>>>>>
> >>>>>>>> Xin -
> >>>>>>>>
> >>>>>>>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
> >>>>>>>> Callers expect an errno, but internally the utility functions set that
> >>>>>>>> field to a GNUTLS_E_ value.
> >>>>>>>>
> >>>>>>>> Can you apply this patch, fix up quic.c as well, and then test both and
> >>>>>>>> post the fixes here?
> >>>>>>>>
> >>>>>>> Sure.
> >>>>>>>
> >>>>>>> But conn->errcode is used to pass the err to parms->session_status.
> >>>>>>> What does parms->session_status expect? errno or GNUTLS_E_ value?
> >>>>>>
> >>>>>> session_status contains a positive errno or zero.
> >>>>>>
> >>>>>>
> >>>>>>> looking at tlshd_start_tls_handshake():
> >>>>>>>                 /* Any errors here should default to blocking access: */
> >>>>>>>                 parms->session_status = EACCES; <---
> >>>>>>>                 switch (ret) {
> >>>>>>>                 case GNUTLS_E_CERTIFICATE_ERROR:
> >>>>>>>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
> >>>>>>>                         tlshd_log_cert_verification_error(session);
> >>>>>>>                         break;
> >>>>>>>                 case -ETIMEDOUT:
> >>>>>>>                         tlshd_log_gnutls_error(ret);
> >>>>>>>                         parms->session_status = -ret; <----
> >>>>>>>         parms->session_status = tlshd_initialize_ktls(session);
> >>>>>>>
> >>>>>>> It also seems mixed, no?
> >>>>>>
> >>>>>> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
> >>>>>> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
> >>>>>>
> >>>>>> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
> >>>>>> gnutls to kernel"). Passing -ETIMEDOUT directly to
> >>>>>> tlshd_log_gnutls_error() does seem a little bogus.
> >>>>>>
> >>>>>
> >>>>> Got it. I'm trying to align the QUIC err process with tls13 code.
> >>>>>
> >>>>> - tlshd_tls13_client_x509_handshake()
> >>>>> - tlshd_tls13_client_psk_handshake()
> >>>>>
> >>>>> - tlshd_tls13_server_x509_handshake()
> >>>>> - tlshd_tls13_server_psk_handshake()
> >>>>>
> >>>>> It seems to me that: When gnutls_xxx() returns errs in these functions,
> >>>>> it only logs them, and doesn't set any value to parms->session_status,
> >>>>> but sets it until tlshd_start_tls_handshake().
> >>>>>
> >>>>> So my suggestion is: not touch these functions for QUIC code:
> >>>>>
> >>>>> - tlshd_quic_client_set_x509_session()
> >>>>> - tlshd_quic_client_set_psk_session()
> >>>>>
> >>>>> - tlshd_quic_server_set_x509_session()
> >>>>> - tlshd_quic_server_set_psk_session()
> >>>>>
> >>>>> but change their callers tlshd_quic_client/serverhello_handshake():
> >>>>>
> >>>>> @@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
> >>>>> tlshd_handshake_parms *parms)
> >>>>>                 ret = -EINVAL;
> >>>>>                 tlshd_log_debug("Unrecognized auth mode (%d)",
> >>>>> parms->auth_mode);
> >>>>>         }
> >>>>> -       if (ret) {
> >>>>> -               conn->errcode = -ret;
> >>>>> -               goto out;
> >>>>> +       if (!ret) {
> >>>>
> >>>>    if (ret == GNUTLS_E_SUCCESSS) {
> >>>>
> >>>> is preferred.
> >>>>
> >>>>
> >>>>> +               tlshd_quic_start_handshake(conn);
> >>>>> +               parms->session_status = conn->errcode;
> >>>>>         }
> >>>>> -
> >>>>> -       tlshd_quic_start_handshake(conn);
> >>>>> -out:
> >>>>> -       parms->session_status = conn->errcode;
> >>>>>         tlshd_quic_conn_destroy(conn);
> >>>>
> >>>> We usually express the return code checking as
> >>>>
> >>>>    do something
> >>>>    if (error) {
> >>>>       error flow
> >>>>    }
> >>>>
> >>>> That's why the goto is in there. But I'll leave it up to you in
> >>>> the quic code.
> >>>>
> >>>>
> >>>>> and then in tlshd_quic_start_handshake(), keep the code setting
> >>>>> conn->errcode = errno, but set conn->errcode = EACCES for all
> >>>>> GNUTLS_E_ places.
> >>>>>
> >>>>> What do you think?
> >>>>
> >>>> I think that still mixes the error return values for the internal
> >>>> functions?
> >>>>
> >>>> The reason I noticed this is I'm trying to add proper Doxygen
> >>>> comments in here. The internal functions need to be consistent about
> >>>> returning either only GNUTLS_E or errno. The documenting comments
> >>>> can't say "returns GNUTLS_E, negative errno, or zero", for example.
> >>>>
> >>> I see, how about if I do this in all GNUTLS_E returned functions?
> >>>
> >>> diff --git a/src/tlshd/server.c b/src/tlshd/server.c
> >>> index db5db31..0ee1157 100644
> >>> --- a/src/tlshd/server.c
> >>> +++ b/src/tlshd/server.c
> >>> @@ -538,7 +538,7 @@ err:
> >>>         tlshd_x509_server_put_privkey();
> >>>         tlshd_x509_server_put_certs();
> >>>         tlshd_log_gnutls_error(ret);
> >>> -       return ret;
> >>> +       return ret ? -EACCES : 0;
> >>>  }
> >>>
> >>> This way, we will not need to change their callers.
> >>
> >> IMHO we want to distinguish between a temporary memory allocation
> >> failure, invalid input, and a permanent access failure.
> >>
> > Yes, that will be nice.
>
> Perhaps all that matters here is the audit message -- the admin will
> want to know what kind of error it was. tlshd recovery is just clean
> up and exit for all three.
>
>
> > However, the tlshd_tls13_* functions don’t currently convert GNUTLS_E
> > values into errno for parms->session_status; they only log them via
> > tlshd_log_gnutls_error(ret).
>
> The tlshd_tls13_ helpers return void, IIRC, because the session_status
> field is initialized to EIO, and that gets cleared by
> tlshd_start_tls_handshake() when the handshake is successful.
>
> The quic helpers return an int... maybe they should return void as well?
Yes, that's also my thought.

>
>
> > If we adopt your original patch (which kind of maps GNUTLS_E values to
> > errno and  assigns them to parms->session_status) for QUIC, it would make
> > the tlshd_quic_* functions diverge in behavior from the tlshd_tls13_*
> > functions.
> >
> > Or you think we should make this change in quic first and align tls13 later?
> >
> > Sorry for the confusion.
>
> Confusion is mostly because I don't remember how any of this works.
>
:D Thank you.