Commit 14b007e17881 added an address check using iter_iov_addr() and a
length check using iter_iov_len() to iov_iter_extract_bvecs(), but these
cannot be used so and are unsafe in this circumstance as the functions have
hardwired assumptions about the iterator type. They should only be used
with ITER_UBUF or ITER_IOVEC-type iterators; they should not be used with
ITER_BVEC, ITER_KVEC, ITER_FOLIOQ, ITER_XARRAY or ITER_DISCARD iterators.
This change proved to be a problem for cachefiles as an iterator of type
ITER_FOLIOQ is passed and iter_iov_addr() and iter_iov_len() both
malfunction because iter->__iov in iter_iov() is not pointing to an iovec
array.
Fix this by using iov_iter_alignment() instead.
Fixes: 14b007e17881 ("block: validate user space vectors during extraction")
cc: Keith Busch <kbusch@kernel.org>
cc: Jens Axboe <axboe@kernel.dk>
cc: Hannes Reinecke <hare@kernel.org>
cc: Christoph Hellwig <hch@lst.de>
cc: Alexander Viro <viro@zeniv.linux.org.uk>
cc: Paulo Alcantara <pc@manguebit.org>
cc: netfs@lists.linux.dev
cc: linux-block@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
---
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 6665372ecf71..6df716e19247 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1921,15 +1921,19 @@ ssize_t iov_iter_extract_bvecs(struct iov_iter *iter, struct bio_vec *bv,
unsigned short max_vecs, unsigned mem_align_mask,
iov_iter_extraction_t extraction_flags)
{
- unsigned long start = (unsigned long)iter_iov_addr(iter);
unsigned short entries_left = max_vecs - *nr_vecs;
unsigned short nr_pages, i = 0;
size_t left, offset, len;
struct page **pages;
ssize_t size;
- if ((start | iter_iov_len(iter)) & mem_align_mask)
+ if (iov_iter_alignment(iter) & mem_align_mask) {
+ pr_warn("%u %zx %x\n",
+ iter->iter_type,
+ iov_iter_alignment(iter),
+ mem_align_mask);
return -EINVAL;
+ }
/*
* Move page array up in the allocated memory for the bio vecs as far asOn Wed, Aug 26, 2026 at 07:18:37PM +0100, David Howells wrote:
> @@ -1921,15 +1921,19 @@ ssize_t iov_iter_extract_bvecs(struct iov_iter *iter, struct bio_vec *bv,
> unsigned short max_vecs, unsigned mem_align_mask,
> iov_iter_extraction_t extraction_flags)
> {
> - unsigned long start = (unsigned long)iter_iov_addr(iter);
> unsigned short entries_left = max_vecs - *nr_vecs;
> unsigned short nr_pages, i = 0;
> size_t left, offset, len;
> struct page **pages;
> ssize_t size;
>
> - if ((start | iter_iov_len(iter)) & mem_align_mask)
> + if (iov_iter_alignment(iter) & mem_align_mask) {
> + pr_warn("%u %zx %x\n",
> + iter->iter_type,
> + iov_iter_alignment(iter),
> + mem_align_mask);
> return -EINVAL;
> + }
iov_iter_alignment loops over all the vectors when we only need to
examine the current one here.
Keith Busch <kbusch@kernel.org> wrote: > iov_iter_alignment loops over all the vectors when we only need to > examine the current one here. Actually, I don't think that's true. iov_iter_extract_pages() is allowed to pull from multiple bio_vecs in an ITER_BVEC, for example - and if, say, the page in the second bio_vec is contiguous with the first, then iov_iter_extract_bvecs() will use it - so you still need to check bv_len on it. David
On Wed, Aug 26, 2026 at 08:49:00PM +0100, David Howells wrote: > Keith Busch <kbusch@kernel.org> wrote: > > > iov_iter_alignment loops over all the vectors when we only need to > > examine the current one here. > > Actually, I don't think that's true. iov_iter_extract_pages() is allowed to > pull from multiple bio_vecs in an ITER_BVEC, for example - and if, say, the > page in the second bio_vec is contiguous with the first, then > iov_iter_extract_bvecs() will use it - so you still need to check bv_len on > it. I don't think we should be extracting bvecs for the ITER_BVEC type. bio_iov_iter_get_pages() already doesn't. I'll look more into the recently introduced bio_iov_iter_bounce_read() usage, as there may be an optimization there. But in general, yeah, it should be safe for any type. The proposal I sent a bit ago will handle the ITER_BVEC as you've desribed.
Keith Busch <kbusch@kernel.org> wrote: > iov_iter_alignment loops over all the vectors when we only need to > examine the current one here. Can the check be done earlier, then? David
On Wed, Aug 26, 2026 at 08:24:05PM +0100, David Howells wrote:
> Keith Busch <kbusch@kernel.org> wrote:
>
> > iov_iter_alignment loops over all the vectors when we only need to
> > examine the current one here.
>
> Can the check be done earlier, then?
It used to be earlier, but the point was to reduce repeated iter
looping. It adds up, so I trying to co-locate validity checks with
places that have to iterate.
Would it be okay to special case the ubuf, iovec, and kvec types for the
simple check?
---
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 81e5c5e5121f7..b6b1e75352b0b 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1920,15 +1920,22 @@ ssize_t iov_iter_extract_bvecs(struct iov_iter *iter, struct bio_vec *bv,
unsigned short max_vecs, unsigned mem_align_mask,
iov_iter_extraction_t extraction_flags)
{
- unsigned long start = (unsigned long)iter_iov_addr(iter);
unsigned short entries_left = max_vecs - *nr_vecs;
unsigned short nr_pages, i = 0;
size_t left, offset, len;
struct page **pages;
ssize_t size;
- if ((start | iter_iov_len(iter)) & mem_align_mask)
+ if (likely(iter_is_ubuf(iter) ||
+ iter_is_iovec(iter) ||
+ iov_iter_is_kvec(iter))) {
+ unsigned long start = (unsigned long)iter_iov_addr(iter);
+
+ if ((start | iter_iov_len(iter)) & mem_align_mask)
+ return -EINVAL;
+ } else if (iov_iter_alignment(iter) & mem_align_mask) {
return -EINVAL;
+ }
/*
* Move page array up in the allocated memory for the bio vecs as far as
--
Keith Busch <kbusch@kernel.org> wrote:
> + if (likely(iter_is_ubuf(iter) ||
> + iter_is_iovec(iter) ||
> + iov_iter_is_kvec(iter))) {
I wonder if it's worth making an exception for ITER_KVEC. I know the structs
align, as it were, but do we use ITER_KVEC often enough?
Anyway, it seems to work, so you can add:
Reviewed-by: David Howells <dhowells@redhat.com>
if you want to submit the patch yourself.
David
Commit 14b007e17881 added an address check using iter_iov_addr() and a
length check using iter_iov_len() to iov_iter_extract_bvecs(), but these
cannot be used so and are unsafe in this circumstance as the functions have
hardwired assumptions about the iterator type. They should only be used
with ITER_UBUF or ITER_IOVEC-type iterators and work with ITER_KVEC; they
should not be used with ITER_BVEC, ITER_FOLIOQ, ITER_XARRAY or ITER_DISCARD
iterators.
This change proved to be a problem for cachefiles as an iterator of type
ITER_FOLIOQ is passed and iter_iov_addr() and iter_iov_len() both
malfunction because iter->__iov in iter_iov() is not pointing to an iovec
array.
Fix this by using iov_iter_alignment() instead for anything other than
ITER_UBUF, ITER_IOVEC or ITER_KVEC.
Fixes: 14b007e17881 ("block: validate user space vectors during extraction")
Suggested-by: Keith Busch <kbusch@kernel.org>
cc: Keith Busch <kbusch@kernel.org>
cc: Jens Axboe <axboe@kernel.dk>
cc: Hannes Reinecke <hare@kernel.org>
cc: Christoph Hellwig <hch@lst.de>
cc: Alexander Viro <viro@zeniv.linux.org.uk>
cc: Paulo Alcantara <pc@manguebit.org>
cc: netfs@lists.linux.dev
cc: linux-block@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
---
lib/iov_iter.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 6665372ecf71..c489569a5815 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1921,15 +1921,22 @@ ssize_t iov_iter_extract_bvecs(struct iov_iter *iter, struct bio_vec *bv,
unsigned short max_vecs, unsigned mem_align_mask,
iov_iter_extraction_t extraction_flags)
{
- unsigned long start = (unsigned long)iter_iov_addr(iter);
unsigned short entries_left = max_vecs - *nr_vecs;
unsigned short nr_pages, i = 0;
size_t left, offset, len;
struct page **pages;
ssize_t size;
- if ((start | iter_iov_len(iter)) & mem_align_mask)
+ if (likely(iter_is_ubuf(iter) ||
+ iter_is_iovec(iter) ||
+ iov_iter_is_kvec(iter))) {
+ unsigned long start = (unsigned long)iter_iov_addr(iter);
+
+ if ((start | iter_iov_len(iter)) & mem_align_mask)
+ return -EINVAL;
+ } else if (iov_iter_alignment(iter) & mem_align_mask) {
return -EINVAL;
+ }
/*
* Move page array up in the allocated memory for the bio vecs as far asShould this grow a comment explaining why for non-iov/kvec/ubuf we check the alignment of every sector instead of just the start alignment?
Christoph Hellwig <hch@lst.de> wrote: > Should this grow a comment explaining why for non-iov/kvec/ubuf > we check the alignment of every sector instead of just the > start alignment? There should possibly be a comment explaining the need for an address alignment at all. Is it mandatory for all devices? Or are we being a bit over restrictive? I don't think NICs capable of RDMA are so restricted, for example. Note that iov_iter_alignment() for ITER_XARRAY and ITER_FOLIOQ basically just check iov_offset and count anyway as they refer to whole pages/folios only, with just the start and end reduced. David
On Fri, Sep 04, 2026 at 10:42:17AM +0100, David Howells wrote: > Christoph Hellwig <hch@lst.de> wrote: > > > Should this grow a comment explaining why for non-iov/kvec/ubuf > > we check the alignment of every sector instead of just the > > start alignment? > > There should possibly be a comment explaining the need for an address > alignment at all. Sure.
On Fri, Sep 04, 2026 at 10:42:17AM +0100, David Howells wrote: > Christoph Hellwig <hch@lst.de> wrote: > > > Should this grow a comment explaining why for non-iov/kvec/ubuf > > we check the alignment of every sector instead of just the > > start alignment? > > There should possibly be a comment explaining the need for an address > alignment at all. Is it mandatory for all devices? Or are we being a bit > over restrictive? I don't think NICs capable of RDMA are so restricted, for > example. If a block device behind an RDMA NIC really wants to DMA a single byte at a time, we can't describe that capability today. That would require a 0 mask, which is currently treated as "unset" and overridden with a default 512b mask.
Keith Busch <kbusch@kernel.org> wrote:
> If a block device behind an RDMA NIC really wants to DMA a single byte
> at a time, we can't describe that capability today. That would require a
> 0 mask, which is currently treated as "unset" and overridden with a
> default 512b mask.
The thing is, Christoph said:
Massage __bio_iov_iter_get_pages so that it doesn't need the bio, and
move it to lib/iov_iter.c so that it can be used by block code for
other things than filling a bio and by other subsystems like netfs.
but unless it can have a 1-byte alignment, it's not actually much use for
netfslib (I have to be able to support "unbuffered writes", which are almost
exactly like DIO writes but without alignment restrictions).
Do block devices really need an aligned "start memory address" or just an
aligned "start file position" (whatever that means for a blockdev).
David
On Sat, Sep 05, 2026 at 05:26:40PM +0100, David Howells wrote: > Keith Busch <kbusch@kernel.org> wrote: > > > If a block device behind an RDMA NIC really wants to DMA a single byte > > at a time, we can't describe that capability today. That would require a > > 0 mask, which is currently treated as "unset" and overridden with a > > default 512b mask. > > The thing is, Christoph said: > > Massage __bio_iov_iter_get_pages so that it doesn't need the bio, and > move it to lib/iov_iter.c so that it can be used by block code for > other things than filling a bio and by other subsystems like netfs. > > but unless it can have a 1-byte alignment, it's not actually much use for > netfslib (I have to be able to support "unbuffered writes", which are almost > exactly like DIO writes but without alignment restrictions). > > Do block devices really need an aligned "start memory address" or just an > aligned "start file position" (whatever that means for a blockdev). The file position is totally irrelevant, the memory address, or in case of IOMMUs the IOVA (which keeps the lower bits of the offset aligned) matter. Most hardware, bother storage and networking (and lots of others) require at least 4-byte aka DWORD alignment. But if we want to support event less, we'll need to adjust the interface, which shouldn't be too horrible. > > David ---end quoted text---
I forgot to add: Signed-off-by: David Howells <dhowells@redhat.com>
On Wed, Aug 26, 2026 at 09:46:32PM +0100, David Howells wrote:
> Fixes: 14b007e17881 ("block: validate user space vectors during extraction")
> Suggested-by: Keith Busch <kbusch@kernel.org>
Thanks, looks good.
Reviewed-by: Keith Busch <kbusch@kernel.org>
Missing your Signed-off-by?
> cc: Keith Busch <kbusch@kernel.org>
> cc: Jens Axboe <axboe@kernel.dk>
> cc: Hannes Reinecke <hare@kernel.org>
> cc: Christoph Hellwig <hch@lst.de>
> cc: Alexander Viro <viro@zeniv.linux.org.uk>
> cc: Paulo Alcantara <pc@manguebit.org>
> cc: netfs@lists.linux.dev
> cc: linux-block@vger.kernel.org
> cc: linux-fsdevel@vger.kernel.org
> ---
> lib/iov_iter.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 6665372ecf71..c489569a5815 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1921,15 +1921,22 @@ ssize_t iov_iter_extract_bvecs(struct iov_iter *iter, struct bio_vec *bv,
> unsigned short max_vecs, unsigned mem_align_mask,
> iov_iter_extraction_t extraction_flags)
> {
> - unsigned long start = (unsigned long)iter_iov_addr(iter);
> unsigned short entries_left = max_vecs - *nr_vecs;
> unsigned short nr_pages, i = 0;
> size_t left, offset, len;
> struct page **pages;
> ssize_t size;
>
> - if ((start | iter_iov_len(iter)) & mem_align_mask)
> + if (likely(iter_is_ubuf(iter) ||
> + iter_is_iovec(iter) ||
> + iov_iter_is_kvec(iter))) {
> + unsigned long start = (unsigned long)iter_iov_addr(iter);
> +
> + if ((start | iter_iov_len(iter)) & mem_align_mask)
> + return -EINVAL;
> + } else if (iov_iter_alignment(iter) & mem_align_mask) {
> return -EINVAL;
> + }
>
> /*
> * Move page array up in the allocated memory for the bio vecs as far as
>
© 2016 - 2026 Red Hat, Inc.