On Sun, May 22, 2022 at 12:06:24PM -0600, Jens Axboe wrote:
> union {
> + size_t uaddr_len;
WTF for?
> +#define iterate_uaddr(i, n, base, len, off, STEP) { \
> + size_t off = 0; \
> + size_t skip = i->iov_offset; \
> + len = min(n, i->uaddr_len - skip); \
How would you ever get offset past the size? Note that we do
keep track of amount of space left...
> - if (iter_is_iovec(i))
> + if (!i->nofault)
> might_fault();
Nope. Sorry, but by default nofault will be false for *all* types.
It's not "I won't fault", it's "pass FOLL_NOFAULT to g_u_p".
> - if (iov_iter_is_bvec(i) || iov_iter_is_kvec(i) || iov_iter_is_xarray(i)) {
> + if (iov_iter_is_bvec(i) || iov_iter_is_kvec(i) ||
> + iter_is_uaddr(i) || iov_iter_is_xarray(i)) {
> void *kaddr = kmap_local_page(page);
> size_t wanted = _copy_to_iter(kaddr + offset, bytes, i);
> kunmap_local(kaddr);
Nope. This is bogus - _copy_to_iter() *can* legitimately fault for those, so
the same considerations as for iovec apply.
> @@ -900,7 +934,8 @@ size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
> return 0;
> if (likely(iter_is_iovec(i)))
> return copy_page_from_iter_iovec(page, offset, bytes, i);
> - if (iov_iter_is_bvec(i) || iov_iter_is_kvec(i) || iov_iter_is_xarray(i)) {
> + if (iov_iter_is_bvec(i) || iov_iter_is_kvec(i) ||
> + iter_is_uaddr(i) || iov_iter_is_xarray(i)) {
Ditto.
> @@ -1319,6 +1368,14 @@ unsigned long iov_iter_alignment(const struct iov_iter *i)
> if (iov_iter_is_bvec(i))
> return iov_iter_alignment_bvec(i);
>
> + if (iter_is_uaddr(i)) {
> + size_t len = i->count - i->iov_offset;
> +
> + if (len)
> + return (unsigned long) i->uaddr + i->iov_offset;
Huh? iov_iter_alignment() wants the worse of beginning and end, if there's
any data at all.
> @@ -1527,6 +1584,9 @@ ssize_t iov_iter_get_pages(struct iov_iter *i,
> if (!maxsize)
> return 0;
>
> + if (WARN_ON_ONCE(iter_is_uaddr(i)))
> + return 0;
So no DIO read(2) or write(2) anymore? Harsh...
> @@ -1652,6 +1712,8 @@ ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
> maxsize = i->count;
> if (!maxsize)
> return 0;
> + if (WARN_ON_ONCE(iter_is_uaddr(i)))
> + return 0;
Ditto.
> + if (iter_is_uaddr(i)) {
> + unsigned long uaddr = (unsigned long) i->uaddr;
> + unsigned long start, end;
> +
> + end = (uaddr + i->count - i->iov_offset + PAGE_SIZE - 1)
> + >> PAGE_SHIFT;
> + start = uaddr >> PAGE_SHIFT;
> + return min_t(int, end - start, maxpages);
Nope. The value is wrong (sanity check - decrement the base and increment
iov_offset by the same amount; the range of addresses is the same,
and the same should go for the result; yours fails that test).
FWIW, here it's
if (likely(iter_is_ubuf(i))) {
unsigned offs = offset_in_page(i->ubuf + i->iov_offset);
int npages = DIV_ROUND_UP(offs + i->count, PAGE_SIZE);
return min(npages, maxpages);
}
BTW, you've missed iov_iter_gap_alignment()...
