Re: [PATCH 1/1] block: validate user space vectors during extraction

On Thu, Jun 18, 2026 at 12:26:27PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 17, 2026 at 04:32:35PM -0700, Keith Busch wrote:
> > @@ -1251,6 +1251,11 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
> >  
> >  	if (iov_iter_is_bvec(iter)) {
> >  		bio_iov_bvec_set(bio, iter);
> > +
> > +		if (mp_bvec_iter_offset(bio->bi_io_vec, bio->bi_iter) &
> > +							vec_align_mask)
> > +			return -EINVAL;
> 
> Can you add a comment here?  Especially as the bvec iter doesn't actually
> require all individual bvecs to be aligned and I'm not entirely sure this
> handles all case - writing down the rules might help a bit with that.

The rationale is that the only iter_bvec users come from io_uring
registered buffers, which are virtually contiguous. Subsequent IO
referencing it provides only an offset and a length, so the only
possible unlaignment could bne the first offset (we've already verified
the total length earlier). Every subsequent vector must be page aligned
at a minimum, which is the largest possible dma alignment the block
layer allows, so we don't need to check the rest.
 
> >  		ret = iov_iter_extract_bvecs(iter, bio->bi_io_vec,
> >  				BIO_MAX_SIZE - bio->bi_iter.bi_size,
> > -				&bio->bi_vcnt, bio->bi_max_vecs, flags);
> > +				&bio->bi_vcnt, bio->bi_max_vecs,
> > +				vec_align_mask, flags);
> >  		if (ret <= 0) {
> > +			if (ret == -EINVAL) {
> > +				bio_release_pages(bio, false);
> > +				bio_clear_flag(bio, BIO_PAGE_PINNED);
> > +				bio->bi_iter.bi_size = 0;
> > +				bio->bi_vcnt = 0;
> > +				return ret;
> > +			}
> 
> Do we need all this cleanups beyoned the bio_release_pages()?  Most
> callers just free the bio, so should not care about it, and the error
> handling in __blkdev_direct_IO that calls bio_endio looks buggy for
> other reasons..

Yeah, it's exactly for the __blkdev_direct_IO() error handling, though I
think clearing either the PINNED flag or bi_vcnt is sufficient after
bio_release_pages(). The rest is just resetting the bio to the initial
state since I didn't want to return both an error and something that
looks like a partially constructed bio, even if no one currently cares.

But since you mention it, __blkdev_direct_IO's handling does look wrong,
so maybe I can clean that up first.