On 10.03.21 17:35, Fam Zheng wrote:
On Wed, 10 Mar 2021 at 15:02, Max Reitz <[email protected] <mailto:[email protected]>> wrote:On 10.03.21 15:17, [email protected] <mailto:[email protected]> wrote: > From: Fam Zheng <[email protected] <mailto:[email protected]>> > > null-co:// has a read-zeroes=off default, when used to in security > analysis, this can cause false positives because the driver doesn't > write to the read buffer. > > null-co:// has the highest possible performance as a block driver, so > let's keep it that way. This patch introduces zero-co:// and > zero-aio://, largely similar with null-*://, but have read-zeroes=on by > default, so it's more suitable in cases than null-co://. > > Signed-off-by: Fam Zheng <[email protected] <mailto:[email protected]>> > --- > block/null.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) You’ll also need to make all tests that currently use null-{co,aio} use zero-{co,aio}, because none of those are performance tests (as far as I’m aware), so they all want a block driver that memset()s. (And that’s basically my problem with this approach; nearly everyone who uses null-* right now probably wants read-zeroes=on, so keeping null-* as it is means all of those users should be changed. Sure, they were all wrong to not specify read-zeroes=on, but that’s how it is. So while technically this patch is a compatible change, in contrast to the one making read-zeroes=on the default, in practice it absolutely isn’t.) Another problem arising from that is I can imagine that some distributions might have whitelisted null-co because many iotests implicitly depend on it, so the iotests will fail if they aren’t whitelisted. Now they’d need to whitelist zero-co, too. Not impossible, sure, but it’s work that would need to be done. My problem is this: We have a not-really problem, namely “valgrind and other tools complain”. Philippe (and I guess me on IRC) proposed a simple solution whose only drawbacks (AFAIU) are: (1) When writing performance tests, you’ll then need to explicitly specify read-zeroes=off. Existing performance tests using null-co will probably wrongly show degredation. (Are there such tests, though?) (2) null will not quite conform to its name, strictly speaking. Frankly, I don’t know who’d care other than people who write those performance tests mentioned in (1). I know I don’t care. (Technically: (3) We should look into all qemu tests that use null-co to see whether they test performance. In practice, they don’t, so we don’t need to.) So AFAIU change the read-zeroes default would affect very few people, if any. I see you care about (2), and you’re the maintainer, so I can’t say that there is no problem. But it isn’t a practical one. So on the practical front, we get a small benefit (tools won’t complain) for a small drawback (having to remember read-zeroes=off for performance tests). Now you propose a change that has the following drawbacks, as I see it: (1) All non-performance tests using null-* should be changed to zero-*. And those are quite a few tests, so this is some work. (2) Distributions that have whitelisted null-co now should whitelist zero-co, too. Not impossible, but I consider these much bigger practical drawbacks than making read-zeroes=on the default. It’s actual work that must be done. To me personally, these drawbacks far outweigh the benefit of having valgrind and other tools not complain. I can’t stop you from updating this patch to do (1), but it doesn’t make sense to me from a practical perspective. It just doesn’t seem worth it to me.But using null-co:// in tests is not wrong, the problem is the caller failed to initialize its buffer.
Then I don’t see why we’d need zero-co://. Max
