On 10.03.21 15:17, fam@euphon.net wrote:
> From: Fam Zheng <famzheng@amazon.com>
>
> 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 <fam@euphon.net>
> ---
> 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. IMO the valgrind issue should really be fixed like this:
diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index dcbccee294..9078718445 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -55,7 +55,7 @@ struct partition {
static int guess_disk_lchs(BlockBackend *blk,
int *pcylinders, int *pheads, int *psectors)
{
- uint8_t buf[BDRV_SECTOR_SIZE];
+ uint8_t buf[BDRV_SECTOR_SIZE] = {};
int i, heads, sectors, cylinders;
struct partition *p;
uint32_t nr_sects;