netdev - Re: [PATCH net 3/5] tls: don't skip over different type records from the rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240220175053.16324f4d@kernel.org>
Date: Tue, 20 Feb 2024 17:50:53 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Sabrina Dubroca <sd@...asysnail.net>
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

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 :)

> > 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.

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?

> > 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:

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 9fbc70200cd0..6e6e6d89b173 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -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,
+			   int *out_copied)
 {
 	struct sk_buff *skb = skb_peek(&ctx->rx_list);
 	struct tls_msg *tlm;
@@ -1843,7 +1844,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 +1968,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);
+	err = process_rx_list(ctx, msg, &control, 0, len, is_peek, &copied);
 	if (err < 0)
 		goto end;
 
-	copied = err;
 	if (len <= copied)
 		goto end;
 
@@ -2128,10 +2129,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, &ret);
 		else
 			err = process_rx_list(ctx, msg, &control, 0,
-					      async_copy_bytes, is_peek);
+					      async_copy_bytes, is_peek, &ret);
 	}
 
 	copied += decrypted;