Re: [PATCH net-next 1/2] rds: convert to getsockopt_iter

[Breno Leitao <leitao@debian.org>]

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