Message-ID: <ZdYBzKcmIorAO47N@hog>
Date: Wed, 21 Feb 2024 14:59:40 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Boris Pismenny <borisp@...dia.com>,
	John Fastabend <john.fastabend@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Shuah Khan <shuah@...nel.org>, Vakul Garg <vakul.garg@....com>,
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net 3/5] tls: don't skip over different type records from
 the rx_list

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

Powered by blists - more mailing lists