Re: [PATCH v8 00/10] fs: interface for directly reading/writing compressed data

On Fri, Mar 19, 2021 at 2:12 PM Omar Sandoval [off-list ref] wrote:

After spending a few minutes trying to simplify copy_struct_from_iter(),
it's honestly easier to just use the iterate_all_kinds() craziness than
open coding it to only operate on iov[0]. But that's an implementation
detail, and we can trivially make the interface stricter:

This is an improvement, but talking about the iterate_all_kinds()
craziness, I think your existing code is broken.

That third case (kernel pointer source):

+    copy = min(ksize - copied, v.iov_len);
+    memcpy(dst + copied, v.iov_base, copy);
+    if (memchr_inv(v.iov_base, 0, v.iov_len))
+        return -E2BIG;

can't be right. Aren't you checking that it's *all* zero, even the
part you copied?

Our iov_iter stuff is really complicated already, this is part of why
I'm not a huge fan of using it.

I still suspect you'd be better off not using the iterate_all_kinds()
thing at all, and just explicitly checking ITER_BVEC/ITER_KVEC
manually.

Because you can play games like fooling your "copy_struct_from_iter()"
to not copy anything at all with ITER_DISCARD, can't you?

Which then sounds like it might end up being useful as a kernel data
leak, because it will use some random uninitialized kernel memory for
the structure.

Now, I don't think you can actually get that ITER_DISCARD case, so
this is not *really* a problem, but it's another example of how that
iterate_all_kinds() thing has these subtle cases embedded into it.

The whole point of copy_struct_from_iter() is presumably to be the
same kind of "obviously safe" interface as copy_struct_from_user() is
meant to be, so these subtle cases just then make me go "Hmm".

I think just open-coding this when  you know there is no actual
looping going on, and the data has to be at the *beginning*, should be
fairly simple. What makes iterate_all_kinds() complicated is that
iteration, the fact that there can be empty entries in there, but it's
also that "iov_offset" thing etc.

For the case where you just (a) require that iov_offset is zero, and
(b) everything has to fit into the very first iov entry (regardless of
what type that iov entry is), I think you actually end up with a much
simpler model.

I do realize that I am perhaps concentrating a bit too much on this
one part of the patch series, but the iov_iter thing has bitten us
before. And it has bitten really core developers and then Al has had
to fix up mistakes.

In fact, it wasn't that long ago that I asked Al to verify code I
wrote, because I was worried about having missed something subtle. So
now when I see these iov_iter users, it just makes me go all nervous.

          Linus