On Tue, Mar 28, 2023 at 10:36 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> Don't assume that a user backed iterator is always of the type
> ITER_IOVEC. Handle the single segment case separately, then we can
> use the same logic for ITER_UBUF and ITER_IOVEC.
Ugh. This is ugly.
Yes,. the original code is ugly too, but this makes it worse.
You have that helper for "give me the number of iovecs" and that just
works automatically with the ITER_UBUF case. But this code (and the
sound driver code in the previous patch), really lso wants a helper to
just return the 'iov' array.
And I think you should just do exactly that. The problem with
'iov_iter_iovec()' is that it doesn't return the array, it just
returns the first entry, so it's unusable for this case, and then you
have all these special "do something else for the single-entry
situation" cases.
And iov_iter_iovec() actually tries to be nice and clever and add the
iov_offset, so that you can actually do the proper iov_iter_advance()
on it etc, but again, this is not what any of this code wants, it just
wants the raw iov array, and the base will always be zero, because
this code just doesn't *work* on the iter level, and never advances
the iterator, it just advances the array index.
And the thing is, I think you could easily just add a
const struct iovec *iov_iter_iovec_array(iter);
helper that just always returns a valid array of iov's.
For a ITER_IOV, it would just return the raw iov pointer.
And for a ITER_UBUF, we could either
(a) just always pass in a single-entry auto iov that gets filled in
and the pointer to it returned
(b) be *really* clever (or ugly, depending on how you want to see
it), and do something like this:
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -49,14 +49,23 @@ struct iov_iter {
size_t iov_offset;
int last_offset;
};
- size_t count;
- union {
- const struct iovec *iov;
- const struct kvec *kvec;
- const struct bio_vec *bvec;
- struct xarray *xarray;
- struct pipe_inode_info *pipe;
- void __user *ubuf;
+
+ /*
+ * This has the same layout as 'struct iovec'!
+ * In particular, the ITER_UBUF form can create
+ * a single-entry 'struct iovec' by casting the
+ * address of the 'ubuf' member to that.
+ */
+ struct {
+ union {
+ const struct iovec *iov;
+ const struct kvec *kvec;
+ const struct bio_vec *bvec;
+ struct xarray *xarray;
+ struct pipe_inode_info *pipe;
+ void __user *ubuf;
+ };
+ size_t count;
};
union {
unsigned long nr_segs;
and if you accept the above, then you can do
#define iter_ubuf_to_iov(iter) ((const struct iovec *)&(iter)->ubuf)
which I will admit is not *pretty*, but it's kind of clever, I think.
So now you can trivially turn a user-backed iov_iter into the related
'struct iovec *' by just doing
#define iov_iter_iovec_array(iter) \
((iter)->type == ITER_UBUF ? iter_ubuf_to_iov(iter) : (iter)->iov)
or something like that.
And no, the above is NOT AT ALL TESTED. Caveat emptor.
And if you go blind from looking at that patch, I will not accept
responsibility.
Linus
