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