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: <874j0rf71y.fsf@kernel.org>
Date: Tue, 18 Feb 2025 19:20:57 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "John Garry" <john.g.garry@...cle.com>
Cc: "Jens Axboe" <axboe@...nel.dk>,  "Oliver Mangold"
 <oliver.mangold@...me>,  <linux-block@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] block: set bi_vcnt when cloning bio

"John Garry" <john.g.garry@...cle.com> writes:

> On 18/02/2025 11:40, Andreas Hindborg wrote:
>> "John Garry" <john.g.garry@...cle.com> writes:
>>
>>> On 15/02/2025 10:58, Andreas Hindborg wrote:
>>>> When cloning a bio, the `bio.bi_vcnt` field is not cloned. This is a
>>>> problem if users want to perform bounds checks on the `bio.bi_io_vec`
>>>> field.
>>>
>>> Is this fixing a potential problem? Or fixing a real issue?
>>
>> It is fixing a problem I ran into in rnull, the rust null block
>> implementation. When running with debug assertions enabled, a bound
>> check on `bi_io_vec` fails for split bio, because `bio_vcnt` becomes
>> zero in the cloned bio.
>>
>> I can work around this by not using a slice type to represent
>> `bi_io_vec` in rust, not a big deal.
>>
>> But I am genuinely curious if there is a reason for not setting
>> `bi_vcnt` during a clone.
>
> I think that it came from commit 59d276fe0 (with the addition of
> bio_clone_fast()), where we assume that the cloned bio is not having the
> bio_vec touched and so does not need to know bi_vcnt (or bi_max_vecs).
> And it is inefficient to needlessly set bi_vcnt then.

I see. That is a few days ago. I am quite confident that for modern
hardware and workloads, this assignment will not have any measurable
impact on performance.

Can we add it back?

I understand if you would prefer not to, since it is not strictly
necessary. But in that case, I would suggest patching the documentation
of `struct bio` something like this:


--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -255,7 +255,8 @@ struct bio {
 	struct bio_integrity_payload *bi_integrity; /* data integrity */
 #endif
 
-	unsigned short		bi_vcnt;	/* how many bio_vec's */
+	unsigned short bi_vcnt;  /* how many bio_vec's. Not valid if this bio is
+	                            a clone (flagged BIO_CLONED). */
 
 	/*
 	 * Everything starting with bi_max_vecs will be preserved by bio_reset()



Best regards,
Andreas Hindborg



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ