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]

Date: Tue, 20 Feb 2024 00:10:58 +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-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"? :))

@@ -1772,9 +1777,10 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
 			   u8 *control,
 			   size_t skip,
 			   size_t len,
-			   bool is_peek)
+			   u8 *flags)
 {
 	struct sk_buff *skb = skb_peek(&ctx->rx_list);
+	bool is_peek = *flags & RXLIST_PEEK;
 	struct tls_msg *tlm;
 	ssize_t copied = 0;
 	int err;
[...]
@@ -1844,6 +1850,9 @@ static int process_rx_list(struct tls_sw_context_rx *ctx,
 
 out:
 	return copied ? : err;
+more:
+	*flags |= RXLIST_MORE;
+	goto out;
 }


and then in tls_sw_recvmsg:
u8 rxlist_flags = is_peek ? RXLIST_PEEK : 0;
err = process_rx_list(ctx, msg, &control, 0, len, &rxlist_flags);


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

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

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

-- 
Sabrina