Re: [RFC 5/9] iov_iter: Add iov_iter_fault_in_writeable()

[Al Viro]

On Mon, May 31, 2021 at 05:12:31PM +0000, Al Viro wrote:

> > +int iov_iter_fault_in_writeable(struct iov_iter *i, size_t bytes)
> > +{
> > +	size_t skip = i->iov_offset;
> > +	const struct iovec *iov;
> > +	int err;
> > +	struct iovec v;
> > +
> > +	if (!(i->type & (ITER_BVEC|ITER_KVEC))) {
> > +		iterate_iovec(i, bytes, v, iov, skip, ({
> > +			err = fault_in_pages_writeable(v.iov_base, v.iov_len);
> > +			if (unlikely(err))
> > +				return err;
> > +		0;}))
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(iov_iter_fault_in_writeable);
> 
> I really don't like that.  Conflicts with iov_iter patches are not hard to
> deal with, but (like fault_in_pages_writeable() itself) it's dangerous as
> hell - fault-in for read is non-destructive, but that is *not*.  Existing
> users have to be careful with it and there are very few of those.  Adding
> that as a new primitive is inviting trouble; at the very least it needs
> a big fat "Don't use unless you really know what you are doing" kind of
> warning.

Actually, is there any good way to make sure that write fault is triggered
_without_ modification of the data?  On x86 lock xadd (of 0, that is) would
probably do it and some of the other architectures could probably get away
with using cmpxchg and its relatives, but how reliable it is wrt always
triggering a write fault if the page is currently read-only?

I mean, something like
	do {
		r0 = r = *p
		atomically [if (*p == r) *p = r; r = *p;]
	} while (r != r0);
would look like a feasible candidate, but what if the processor
"optimizes" that cmpxchg to simple load, seeing that new value is
equal to expected old one?