linux-kernel - Re: [PATCH 01/24] iov_iter: Separate type from direction and use accessor functions

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]

Message-ID: <20181020045608.GH32577@ZenIV.linux.org.uk>
Date:   Sat, 20 Oct 2018 05:56:08 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     David Howells <dhowells@...hat.com>
Cc:     linux-afs@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/24] iov_iter: Separate type from direction and use
 accessor functions

On Sat, Oct 20, 2018 at 02:10:44AM +0100, David Howells wrote:

One general comment: I would strongly recommend splitting the iov_iter
initializers change into a separate patch.

> index 8d41ca7bfcf1..dcdbcb6f09f8 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2990,7 +2990,7 @@ cifs_readdata_to_iov(struct cifs_readdata *rdata, struct iov_iter *iter)
>  		size_t copy = min_t(size_t, remaining, PAGE_SIZE);
>  		size_t written;
>  
> -		if (unlikely(iter->type & ITER_PIPE)) {
> +		if (unlikely(iov_iter_is_pipe(iter))) {
>  			void *addr = kmap_atomic(page);
>  
>  			written = copy_to_iter(addr, copy, iter);

FWIW, I wonder if that one is actually a missing primitive getting open-coded...

> @@ -786,7 +786,7 @@ setup_aio_ctx_iter(struct cifs_aio_ctx *ctx, struct iov_iter *iter, int rw)
>  	struct page **pages = NULL;
>  	struct bio_vec *bv = NULL;
>  
> -	if (iter->type & ITER_KVEC) {
> +	if (iov_iter_is_kvec(iter)) {
>  		memcpy(&ctx->iter, iter, sizeof(struct iov_iter));
>  		ctx->len = count;
>  		iov_iter_advance(iter, count);

... and so, to much greater extent, is this.

> @@ -2054,14 +2054,22 @@ int smbd_recv(struct smbd_connection *info, struct msghdr *msg)

> +	switch (iov_iter_type(&msg->msg_iter)) {
> +	case ITER_KVEC:
>  		buf = msg->msg_iter.kvec->iov_base;
>  		to_read = msg->msg_iter.kvec->iov_len;
>  		rc = smbd_recv_buf(info, buf, to_read);
>  		break;
>  
> -	case READ | ITER_BVEC:
> +	case ITER_BVEC:
>  		page = msg->msg_iter.bvec->bv_page;
>  		page_offset = msg->msg_iter.bvec->bv_offset;
>  		to_read = msg->msg_iter.bvec->bv_len;

Incidentally, this is bollocks - looks like a fallout of RDMA patches of some
sort, but AFAICS there's no reason have separate bvec and kvec
paths there - smbd_recv_buf() can bloody well use copy_to_iter(),
eliminating the need for kmap_atomic, sleep avoidance, etc.
As well as this branching on iterator flavour...  Anyway,
not your headache.

> @@ -1313,7 +1313,7 @@ do_blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
>  	spin_lock_init(&dio->bio_lock);
>  	dio->refcount = 1;
>  
> -	dio->should_dirty = (iter->type == ITER_IOVEC);
> +	dio->should_dirty = iter_is_iovec(iter);

Nope.  This path *can* get both read and write iov_iter.  Not an equivalent
change.

> @@ -1795,7 +1795,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  		if (pos >= dio->i_size)
>  			goto out_free_dio;
>  
> -		if (iter->type == ITER_IOVEC)
> +		if (iter_is_iovec(iter))
>  			dio->flags |= IOMAP_DIO_DIRTY;

Ditto.

> @@ -417,28 +417,35 @@ int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes)
>  	int err;
>  	struct iovec v;
>  
> -	if (!(i->type & (ITER_BVEC|ITER_KVEC))) {
> +	switch (iov_iter_type(i)) {
> +	case ITER_IOVEC:
> +	case ITER_PIPE:
>  		iterate_iovec(i, bytes, v, iov, skip, ({
>  			err = fault_in_pages_readable(v.iov_base, v.iov_len);
>  			if (unlikely(err))
>  			return err;
>  		0;}))
> +		break;
> +	case ITER_KVEC:
> +	case ITER_BVEC:
> +		break;
>  	}
>  	return 0;
>  }
>  EXPORT_SYMBOL(iov_iter_fault_in_readable);

Huh?  That makes no sense whatsoever - ITER_PIPE ones are write-only in the first place,
so they won't be passed to that one, but feeding ITER_PIPE to iterate_iovec() is
insane.  And even if they copy-from ITER_PIPES would appear, why the devil would we
want to fault-in anything?

> @@ -987,7 +1003,7 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll)
>  		return;
>  	i->count += unroll;
> -	if (unlikely(i->type & ITER_PIPE)) {
> +	if (unlikely(iov_iter_is_pipe(i))) {
>  		struct pipe_inode_info *pipe = i->pipe;
...
> +	case ITER_PIPE:
> +		BUG();
> +	}
>  }
>  EXPORT_SYMBOL(iov_iter_revert);

Wha...?