2024-02-20, 17:50:53 -0800, Jakub Kicinski wrote:
On Tue, 20 Feb 2024 00:10:58 +0100 Sabrina Dubroca wrote:
2024-02-19, 12:07:03 -0800, Jakub Kicinski wrote:
On Thu, 15 Feb 2024 17:17:31 +0100 Sabrina Dubroca wrote:
@@ -1772,7 +1772,8 @@ static int process_rx_list(struct tls_sw_context_rx *ctx, u8 *control, size_t skip, size_t len,
bool is_peek)
bool is_peek,bool *more){ struct sk_buff *skb = skb_peek(&ctx->rx_list); struct tls_msg *tlm;
@@ -1844,6 +1845,10 @@ static int process_rx_list(struct tls_sw_context_rx *ctx, out: return copied ? : err; +more:
- if (more)
*more = true;- goto out;
Patches look correct, one small nit here -
I don't have great ideas how to avoid the 7th argument completely but
I hesitated between this patch and a variant combining is_peek and more into a single u8 *flags, but that felt a bit messy (or does that fall into what you describe as "not [having] great ideas"? :))
I guess it saves a register, it seems a bit better but then it's a truly in/out argument :)
We already do that with darg all over the receive code, so it shouldn't be too confusing to readers. It can be named flags_inout if you think that would help, or have a comment like above tls_decrypt_sg.
I think it'd be a little cleaner if we either:
- passed in err as an output argument (some datagram code does that IIRC), then function can always return copied directly, or
(yes, __skb_wait_for_more_packets, __skb_try_recv_datagram, and their variants)
- passed copied as an output argument, and then we can always return err?
Aren't those 2 options adding an 8th argument?
No, no, still 7, if we separate copied from err - checking err < 0 is enough to know that we need to exit.
Right, I realized that you probably meant something like that as I was going to bed last night.
It's not exactly enough, since tls_record_content_type will return 0 on a content type mismatch. We'll have to translate that into an "error". I think it would be a bit nicer to set err=1 and then check err != 0 in tls_sw_recvmsg (we can document that in a comment above process_rx_list) rather than making up a fake errno. See diff [1].
Or we could swap the 0/1 returns from tls_record_content_type and switch the err <= 0 tests to err != 0 after the existing calls, then process_rx_list doesn't have a weird special case [2].
What do you think?
Differently put, perhaps, my preference is to pass an existing entity (err or copied), rather that conjure new concept (more) on one end and interpret it on the other.
I tend to find ">= 0 on success, otherwise errno" more readable, probably because that's a very common pattern (either for recvmsg style of cases, or all the ERR_PTR type situations).
Right it definitely is a good pattern. I think passing copied via argument would give us those semantics still?
For recvmsg sure, but not for process_rx_list.
I like the former a little better because we won't have to special case NULL for the "after async decryption" call sites.
We could also pass &rx_more every time and not check for NULL.
What do you want to clean up more specifically? The number of arguments, the backwards goto, the NULL check before setting *more, something else/all of the above?
Not compiled, but what I had in mind was something along the lines of:
copied is a ssize_t (but ret isn't), so the change gets a bit uglier :(
------------ 8< ------------
[1] fix by setting err=1 in process_rx_list
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 43dd0d82b6ed..711504614da7 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1766,13 +1766,19 @@ static void tls_rx_rec_done(struct tls_sw_context_rx *ctx) * decrypted records into the buffer provided by caller zero copy is not * true. Further, the records are removed from the rx_list if it is not a peek * case and the record has been consumed completely. + * + * Return: + * - 0 if len bytes were copied + * - 1 if < len bytes were copied due to a record type mismatch + * - <0 if an error occurred */ static int process_rx_list(struct tls_sw_context_rx *ctx, struct msghdr *msg, u8 *control, size_t skip, size_t len, - bool is_peek) + bool is_peek, + ssize_t *out_copied) { struct sk_buff *skb = skb_peek(&ctx->rx_list); struct tls_msg *tlm; @@ -1802,8 +1808,11 @@ static int process_rx_list(struct tls_sw_context_rx *ctx, tlm = tls_msg(skb);
err = tls_record_content_type(msg, tlm, control); - if (err <= 0) + if (err <= 0) { + if (err == 0) + err = 1; goto out; + }
err = skb_copy_datagram_msg(skb, rxm->offset + skip, msg, chunk); @@ -1843,7 +1852,8 @@ static int process_rx_list(struct tls_sw_context_rx *ctx, err = 0;
out: - return copied ? : err; + *out_copied = copied; + return err; }
static bool @@ -1966,11 +1976,10 @@ int tls_sw_recvmsg(struct sock *sk, goto end;
/* Process pending decrypted records. It must be non-zero-copy */ - err = process_rx_list(ctx, msg, &control, 0, len, is_peek); - if (err < 0) + err = process_rx_list(ctx, msg, &control, 0, len, is_peek, &copied); + if (err != 0) goto end;
- copied = err; if (len <= copied || (copied && control != TLS_RECORD_TYPE_DATA)) goto end;
@@ -2114,6 +2123,7 @@ int tls_sw_recvmsg(struct sock *sk,
recv_end: if (async) { + ssize_t ret2; int ret;
/* Wait for all previously submitted records to be decrypted */ @@ -2130,10 +2140,10 @@ int tls_sw_recvmsg(struct sock *sk, /* Drain records from the rx_list & copy if required */ if (is_peek || is_kvec) err = process_rx_list(ctx, msg, &control, copied, - decrypted, is_peek); + decrypted, is_peek, &ret2); else err = process_rx_list(ctx, msg, &control, 0, - async_copy_bytes, is_peek); + async_copy_bytes, is_peek, &ret2); }
copied += decrypted;
------------ 8< ------------
[2] fixing the bug by changing tls_record_content_type as well
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 43dd0d82b6ed..3da62ba97945 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1734,6 +1734,11 @@ int decrypt_skb(struct sock *sk, struct scatterlist *sgout) return tls_decrypt_sg(sk, NULL, sgout, &darg); }
+/* Return: + * - 0 on success + * - 1 if the record's type doesn't match the value in control + * - <0 if an error occurred + */ static int tls_record_content_type(struct msghdr *msg, struct tls_msg *tlm, u8 *control) { @@ -1751,10 +1756,10 @@ static int tls_record_content_type(struct msghdr *msg, struct tls_msg *tlm, return -EIO; } } else if (*control != tlm->control) { - return 0; + return 1; }
- return 1; + return 0; }
static void tls_rx_rec_done(struct tls_sw_context_rx *ctx) @@ -1766,13 +1771,19 @@ static void tls_rx_rec_done(struct tls_sw_context_rx *ctx) * decrypted records into the buffer provided by caller zero copy is not * true. Further, the records are removed from the rx_list if it is not a peek * case and the record has been consumed completely. + * + * Return: + * - 0 if len bytes were copied + * - 1 if < len bytes were copied due to a record type mismatch + * - <0 if an error occurred */ static int process_rx_list(struct tls_sw_context_rx *ctx, struct msghdr *msg, u8 *control, size_t skip, size_t len, - bool is_peek) + bool is_peek, + ssize_t *out_copied) { struct sk_buff *skb = skb_peek(&ctx->rx_list); struct tls_msg *tlm; @@ -1784,7 +1795,7 @@ static int process_rx_list(struct tls_sw_context_rx *ctx, tlm = tls_msg(skb);
err = tls_record_content_type(msg, tlm, control); - if (err <= 0) + if (err != 0) goto out;
if (skip < rxm->full_len) @@ -1802,7 +1813,7 @@ static int process_rx_list(struct tls_sw_context_rx *ctx, tlm = tls_msg(skb);
err = tls_record_content_type(msg, tlm, control); - if (err <= 0) + if (err != 0) goto out;
err = skb_copy_datagram_msg(skb, rxm->offset + skip, @@ -1843,7 +1854,8 @@ static int process_rx_list(struct tls_sw_context_rx *ctx, err = 0;
out: - return copied ? : err; + *out_copied = copied; + return err; }
static bool @@ -1966,11 +1978,10 @@ int tls_sw_recvmsg(struct sock *sk, goto end;
/* Process pending decrypted records. It must be non-zero-copy */ - err = process_rx_list(ctx, msg, &control, 0, len, is_peek); - if (err < 0) + err = process_rx_list(ctx, msg, &control, 0, len, is_peek, &copied); + if (err != 0) goto end;
- copied = err; if (len <= copied || (copied && control != TLS_RECORD_TYPE_DATA)) goto end;
@@ -2032,7 +2043,7 @@ int tls_sw_recvmsg(struct sock *sk, * For tls1.3, we disable async. */ err = tls_record_content_type(msg, tls_msg(darg.skb), &control); - if (err <= 0) { + if (err != 0) { DEBUG_NET_WARN_ON_ONCE(darg.zc); tls_rx_rec_done(ctx); put_on_rx_list_err: @@ -2114,6 +2125,7 @@ int tls_sw_recvmsg(struct sock *sk,
recv_end: if (async) { + ssize_t ret2; int ret;
/* Wait for all previously submitted records to be decrypted */ @@ -2130,10 +2142,10 @@ int tls_sw_recvmsg(struct sock *sk, /* Drain records from the rx_list & copy if required */ if (is_peek || is_kvec) err = process_rx_list(ctx, msg, &control, copied, - decrypted, is_peek); + decrypted, is_peek, &ret2); else err = process_rx_list(ctx, msg, &control, 0, - async_copy_bytes, is_peek); + async_copy_bytes, is_peek, &ret2); }
copied += decrypted;