On Thu, Jun 04, 2026 at 07:20:24PM -0700, Allison Henderson wrote:
> Thanks for working on this. The conversions look mostly correct to me, just
> a few nits I noticed below:
Thanks for the very detailed review.
> > + /* The info producers write into the pages with kmap_atomic() while
> > + * holding a spinlock, so they need a genuine page-backed user buffer.
> > + */
> > + if (iov_iter_is_kvec(&opt->iter_out)) {
> > + ret = -EOPNOTSUPP;
> > + goto out;
> I think !user_backed_iter() is more accurate for what you're meaning to do
> here? Technically we only ever get kvecs or
> ubufs since rds_info_getsockopt is only called by do_sock_getsockopt. Those
> appear to be the only two types it passes,
> so it works out. But it means we're counting on that assumption from the
> caller and ideally we should filter anything
> that's not user backed. So, I'd replace the iov_iter_is_kvec check with:
>
> if (!user_backed_iter(&opt->iter_out)) {
Agreed, that's more accurate and doesn't lean on the caller only ever
handing us kvec or ubuf. Will switch to !user_backed_iter() in v2.
> > @@ -230,13 +239,12 @@ int rds_info_getsockopt(struct socket *sock, int
> > optname, char __user *optval,
> > ret = lens.each;
> > }
> >
> > - if (put_user(len, optlen))
> > - ret = -EFAULT;
> > + opt->optlen = len;
> >
> > out:
> The unpin here works, but it took me a little bit to trace out where the
> corresponding pin went since it is removed
> earlier in the patch. Pages are pinned on extract, but only for user pages.
> This works out since the only caller here
> passes kvec or ubuf, and we error out on kvec iters. Pages are assumed
> pinned if they're not null, but without the
> !user_backed_iter check, it leans on this behavior from the caller. Ideally
> I think iov_iter_extract_pages is supposed
> to be paired with iov_iter_extract_will_pin. A quick comment would help
> clarify too. So the closing gate would look
> something like this:
>
> /* unpin pages from ubuf iters */
> if (pages && iov_iter_extract_will_pin(&opt->iter_out))
>
> > if (pages)
> > unpin_user_pages(pages, nr_pages);
>
> Having both checks is somewhat redundant, but it doesnt hurt anything either.
> And I think it helps make it a little
> more uniform and readable. Other than that I think this patch is looking
> pretty good to me.
Makes sense. In v2 I'll pair the extract with extract_will_pin and add a
comment so the pin/unpin contract is self-documenting:
out:
/*
* iov_iter_extract_pages() pins only user-backed (ubuf)
* iters; iov_iter_extract_will_pin() reports whether an
* unpin is owed here.
*/
if (pages && iov_iter_extract_will_pin(&opt->iter_out))
unpin_user_pages(pages, nr_pages);
kvfree(pages);
Thanks for the review!
--breno
