linux-kernel - Re: [RFC][PATCH] iov_iter: Add ITER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Jan 2020 21:42:22 +0000
From:   David Howells <dhowells@...hat.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     dhowells@...hat.com, Alexander Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] iov_iter: Add ITER_MAPPING

Matthew Wilcox <willy@...radead.org> wrote:

> 
> > +	rcu_read_lock();						\
> > +	for (page = xas_load(&xas); page; page = xas_next(&xas)) {	\
> > +		if (xas_retry(&xas, page))				\
> > +			continue;					\
> > +		if (xa_is_value(page))					\
> > +			break;						\
> 
> Do you also want to check for !page?  That would be a bug in the caller.

Well, I stated that one of the preconditions for using this was that the
caller made sure that segment of the mapping was fully populated, so the check
ought to be unnecessary.

> > +		if (PageCompound(page))					\
> > +			break;						\
> 
> It's perfectly legal to have compound pages in the page cache.  Call
> find_subpage(page, xas.xa_index) unconditionally.

Yeah, I'm just not sure how to deal with them.

> > +		if (page_to_pgoff(page) != xas.xa_index)		\
> > +			break;						\
> 
> ... and you can ditch this if the pages are pinned as find_subpage()
> will bug in this case.

Ok.

> > +		__v.bv_page = page;					\
> > +		offset = (i->mapping_start + skip) & ~PAGE_MASK;	\
> > +		seg = PAGE_SIZE - offset;			\
> > +		__v.bv_offset = offset;				\
> > +		__v.bv_len = min(n, seg);			\
> > +		(void)(STEP);					\
> > +		n -= __v.bv_len;				\
> > +		skip += __v.bv_len;				\
> 
> Do we want STEP to be called with PAGE_SIZE chunks, or if they have a
> THP, can we have it called with larger than a PAGE_SIZE chunk?

It would mean that the STEP function would have to handle multiple pages, some
part(s) of which might need to be ignored and wouldn't be able to simply call
memcpy_from/to_page().

> > +#define iterate_all_kinds(i, n, v, I, B, K, M) {		\
> >  	if (likely(n)) {					\
> >  		size_t skip = i->iov_offset;			\
> >  		if (unlikely(i->type & ITER_BVEC)) {		\
> > @@ -86,6 +119,9 @@
> >  			struct kvec v;				\
> >  			iterate_kvec(i, n, v, kvec, skip, (K))	\
> >  		} else if (unlikely(i->type & ITER_DISCARD)) {	\
> > +		} else if (unlikely(i->type & ITER_MAPPING)) {	\
> > +			struct bio_vec v;			\
> > +			iterate_mapping(i, n, v, skip, (M));	\
> 
> bio_vec?

Yes - as a strictly temporary thing.  I need a struct contains a page pointer,
a start and a length, and constructing a struct bio_vec on the fly here allows
the caller to potentially share code.  For example:

    size_t _copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i)
    {
	char *to = addr;
	if (unlikely(iov_iter_is_pipe(i))) {
		WARN_ON(1);
		return 0;
	}
	iterate_and_advance(i, bytes, v,
		__copy_from_user_inatomic_nocache((to += v.iov_len) - v.iov_len,
					 v.iov_base, v.iov_len),
		memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
				 v.bv_offset, v.bv_len),
ITER_BVEC ^^^^
		memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len),
		memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
				 v.bv_offset, v.bv_len)
ITER_MAPPING ^^^^
	)

	return bytes;
    }

David