On Wed, 10 Mar 2021 at 15:02, Max Reitz <mreitz@redhat.com
<mailto:mreitz@redhat.com>> wrote:
On 10.03.21 15:17, fam@euphon.net <mailto:fam@euphon.net> wrote:
> From: Fam Zheng <famzheng@amazon.com <mailto: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 <mailto: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.