| 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
| ||
|
Message-ID: <ed385c4d-99ca-d7aa-8874-96e3c6b743bb@gmail.com>
Date: Wed, 27 Jan 2021 15:48:10 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: David Laight <David.Laight@...lab.com>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
Jens Axboe <axboe@...nel.dk>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iov_iter: optimise iter type checking
On 16/01/2021 05:18, Al Viro wrote:
> On Sat, Jan 09, 2021 at 10:11:09PM +0000, Pavel Begunkov wrote:
>
>>> Does any code actually look at the fields as a pair?
>>> Would it even be better to use separate bytes?
>>> Even growing the on-stack structure by a word won't really matter.
>>
>> u8 type, rw;
>>
>> That won't bloat the struct. I like the idea. If used together compilers
>> can treat it as u16.
>
> Reasonable, and from what I remember from looking through the users,
> no readers will bother with looking at both at the same time.
Al, are you going turn it into a patch, or prefer me to take over?
> On the write side... it's only set in iov_iter_{kvec,bvec,pipe,discard,init}.
> I sincerely doubt anyone would give a fuck, not to mention that something
> like
> void iov_iter_pipe(struct iov_iter *i, unsigned int direction,
> struct pipe_inode_info *pipe,
> size_t count)
> {
> BUG_ON(direction != READ);
> WARN_ON(pipe_full(pipe->head, pipe->tail, pipe->ring_size));
> *i = (struct iov_iter) {
> .iter_type = ITER_PIPE,
> .data_source = false,
> .pipe = pipe,
> .head = pipe->head,
> .start_head = pipe->head,
> .count = count,
> .iov_offset = 0
> };
> }
>
> would make more sense anyway - we do want to overwrite everything in the
> object, and let the compiler do whatever it likes to do.
>
> So... something like (completely untested) variant below, perhaps?
>
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 72d88566694e..ed8ad2c5d384 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -19,20 +19,16 @@ struct kvec {
>
> enum iter_type {
> /* iter types */
> - ITER_IOVEC = 4,
> - ITER_KVEC = 8,
> - ITER_BVEC = 16,
> - ITER_PIPE = 32,
> - ITER_DISCARD = 64,
> + ITER_IOVEC,
> + ITER_KVEC,
> + ITER_BVEC,
> + ITER_PIPE,
> + ITER_DISCARD
> };
>
> struct iov_iter {
> - /*
> - * Bit 0 is the read/write bit, set if we're writing.
> - * Bit 1 is the BVEC_FLAG_NO_REF bit, set if type is a bvec and
> - * the caller isn't expecting to drop a page reference when done.
> - */
> - unsigned int type;
> + u8 iter_type;
> + bool data_source;
> size_t iov_offset;
> size_t count;
> union {
> @@ -52,7 +48,7 @@ struct iov_iter {
>
> static inline enum iter_type iov_iter_type(const struct iov_iter *i)
> {
> - return i->type & ~(READ | WRITE);
> + return i->iter_type;
> }
>
> static inline bool iter_is_iovec(const struct iov_iter *i)
> @@ -82,7 +78,7 @@ static inline bool iov_iter_is_discard(const struct iov_iter *i)
>
> static inline unsigned char iov_iter_rw(const struct iov_iter *i)
> {
> - return i->type & (READ | WRITE);
> + return i->data_source ? WRITE : READ;
> }
>
> /*
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 1635111c5bd2..133c03b2dcae 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -81,19 +81,18 @@
> #define iterate_all_kinds(i, n, v, I, B, K) { \
> if (likely(n)) { \
> size_t skip = i->iov_offset; \
> - if (unlikely(i->type & ITER_BVEC)) { \
> + if (likely(i->iter_type == ITER_IOVEC)) { \
> + const struct iovec *iov; \
> + struct iovec v; \
> + iterate_iovec(i, n, v, iov, skip, (I)) \
> + } else if (i->iter_type == ITER_BVEC) { \
> struct bio_vec v; \
> struct bvec_iter __bi; \
> iterate_bvec(i, n, v, __bi, skip, (B)) \
> - } else if (unlikely(i->type & ITER_KVEC)) { \
> + } else if (i->iter_type == ITER_KVEC) { \
> const struct kvec *kvec; \
> struct kvec v; \
> iterate_kvec(i, n, v, kvec, skip, (K)) \
> - } else if (unlikely(i->type & ITER_DISCARD)) { \
> - } else { \
> - const struct iovec *iov; \
> - struct iovec v; \
> - iterate_iovec(i, n, v, iov, skip, (I)) \
> } \
> } \
> }
> @@ -103,7 +102,17 @@
> n = i->count; \
> if (i->count) { \
> size_t skip = i->iov_offset; \
> - if (unlikely(i->type & ITER_BVEC)) { \
> + if (likely(i->iter_type == ITER_IOVEC)) { \
> + const struct iovec *iov; \
> + struct iovec v; \
> + iterate_iovec(i, n, v, iov, skip, (I)) \
> + if (skip == iov->iov_len) { \
> + iov++; \
> + skip = 0; \
> + } \
> + i->nr_segs -= iov - i->iov; \
> + i->iov = iov; \
> + } else if (i->iter_type == ITER_BVEC) { \
> const struct bio_vec *bvec = i->bvec; \
> struct bio_vec v; \
> struct bvec_iter __bi; \
> @@ -111,7 +120,7 @@
> i->bvec = __bvec_iter_bvec(i->bvec, __bi); \
> i->nr_segs -= i->bvec - bvec; \
> skip = __bi.bi_bvec_done; \
> - } else if (unlikely(i->type & ITER_KVEC)) { \
> + } else if (i->iter_type == ITER_KVEC) { \
> const struct kvec *kvec; \
> struct kvec v; \
> iterate_kvec(i, n, v, kvec, skip, (K)) \
> @@ -121,18 +130,8 @@
> } \
> i->nr_segs -= kvec - i->kvec; \
> i->kvec = kvec; \
> - } else if (unlikely(i->type & ITER_DISCARD)) { \
> + } else if (i->iter_type == ITER_DISCARD) { \
> skip += n; \
> - } else { \
> - const struct iovec *iov; \
> - struct iovec v; \
> - iterate_iovec(i, n, v, iov, skip, (I)) \
> - if (skip == iov->iov_len) { \
> - iov++; \
> - skip = 0; \
> - } \
> - i->nr_segs -= iov - i->iov; \
> - i->iov = iov; \
> } \
> i->count -= n; \
> i->iov_offset = skip; \
> @@ -434,7 +433,7 @@ 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))) {
> + if (i->iter_type == ITER_IOVEC) {
> iterate_iovec(i, bytes, v, iov, skip, ({
> err = fault_in_pages_readable(v.iov_base, v.iov_len);
> if (unlikely(err))
> @@ -450,19 +449,26 @@ void iov_iter_init(struct iov_iter *i, unsigned int direction,
> size_t count)
> {
> WARN_ON(direction & ~(READ | WRITE));
> - direction &= READ | WRITE;
>
> /* It will get better. Eventually... */
> - if (uaccess_kernel()) {
> - i->type = ITER_KVEC | direction;
> - i->kvec = (struct kvec *)iov;
> - } else {
> - i->type = ITER_IOVEC | direction;
> - i->iov = iov;
> - }
> - i->nr_segs = nr_segs;
> - i->iov_offset = 0;
> - i->count = count;
> + if (uaccess_kernel())
> + *i = (struct iov_iter) {
> + .iter_type = ITER_KVEC,
> + .data_source = direction,
> + .kvec = (struct kvec *)iov,
> + .nr_segs = nr_segs,
> + .iov_offset = 0,
> + .count = count
> + };
> + else
> + *i = (struct iov_iter) {
> + .iter_type = ITER_IOVEC,
> + .data_source = direction,
> + .iov = iov,
> + .nr_segs = nr_segs,
> + .iov_offset = 0,
> + .count = count
> + };
> }
> EXPORT_SYMBOL(iov_iter_init);
>
> @@ -915,17 +921,20 @@ size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
> {
> if (unlikely(!page_copy_sane(page, offset, bytes)))
> return 0;
> - if (i->type & (ITER_BVEC|ITER_KVEC)) {
> + if (likely(i->iter_type == ITER_IOVEC))
> + return copy_page_to_iter_iovec(page, offset, bytes, i);
> + if (i->iter_type == ITER_BVEC || i->iter_type == ITER_KVEC) {
> void *kaddr = kmap_atomic(page);
> size_t wanted = copy_to_iter(kaddr + offset, bytes, i);
> kunmap_atomic(kaddr);
> return wanted;
> - } else if (unlikely(iov_iter_is_discard(i)))
> - return bytes;
> - else if (likely(!iov_iter_is_pipe(i)))
> - return copy_page_to_iter_iovec(page, offset, bytes, i);
> - else
> + }
> + if (i->iter_type == ITER_PIPE)
> return copy_page_to_iter_pipe(page, offset, bytes, i);
> + if (i->iter_type == ITER_DISCARD)
> + return bytes;
> + WARN_ON(1);
> + return 0;
> }
> EXPORT_SYMBOL(copy_page_to_iter);
>
> @@ -934,17 +943,16 @@ size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
> {
> if (unlikely(!page_copy_sane(page, offset, bytes)))
> return 0;
> - if (unlikely(iov_iter_is_pipe(i) || iov_iter_is_discard(i))) {
> - WARN_ON(1);
> - return 0;
> - }
> - if (i->type & (ITER_BVEC|ITER_KVEC)) {
> + if (likely(i->iter_type == ITER_IOVEC))
> + return copy_page_from_iter_iovec(page, offset, bytes, i);
> + if (i->iter_type == ITER_BVEC || i->iter_type == ITER_KVEC) {
> void *kaddr = kmap_atomic(page);
> size_t wanted = _copy_from_iter(kaddr + offset, bytes, i);
> kunmap_atomic(kaddr);
> return wanted;
> - } else
> - return copy_page_from_iter_iovec(page, offset, bytes, i);
> + }
> + WARN_ON(1);
> + return 0;
> }
> EXPORT_SYMBOL(copy_page_from_iter);
>
> @@ -1172,11 +1180,14 @@ void iov_iter_kvec(struct iov_iter *i, unsigned int direction,
> size_t count)
> {
> WARN_ON(direction & ~(READ | WRITE));
> - i->type = ITER_KVEC | (direction & (READ | WRITE));
> - i->kvec = kvec;
> - i->nr_segs = nr_segs;
> - i->iov_offset = 0;
> - i->count = count;
> + *i = (struct iov_iter) {
> + .iter_type = ITER_KVEC,
> + .data_source = direction,
> + .kvec = kvec,
> + .nr_segs = nr_segs,
> + .iov_offset = 0,
> + .count = count
> + };
> }
> EXPORT_SYMBOL(iov_iter_kvec);
>
> @@ -1185,11 +1196,14 @@ void iov_iter_bvec(struct iov_iter *i, unsigned int direction,
> size_t count)
> {
> WARN_ON(direction & ~(READ | WRITE));
> - i->type = ITER_BVEC | (direction & (READ | WRITE));
> - i->bvec = bvec;
> - i->nr_segs = nr_segs;
> - i->iov_offset = 0;
> - i->count = count;
> + *i = (struct iov_iter) {
> + .iter_type = ITER_BVEC,
> + .data_source = direction,
> + .bvec = bvec,
> + .nr_segs = nr_segs,
> + .iov_offset = 0,
> + .count = count
> + };
> }
> EXPORT_SYMBOL(iov_iter_bvec);
>
> @@ -1199,12 +1213,15 @@ void iov_iter_pipe(struct iov_iter *i, unsigned int direction,
> {
> BUG_ON(direction != READ);
> WARN_ON(pipe_full(pipe->head, pipe->tail, pipe->ring_size));
> - i->type = ITER_PIPE | READ;
> - i->pipe = pipe;
> - i->head = pipe->head;
> - i->iov_offset = 0;
> - i->count = count;
> - i->start_head = i->head;
> + *i = (struct iov_iter) {
> + .iter_type = ITER_PIPE,
> + .data_source = false,
> + .pipe = pipe,
> + .head = pipe->head,
> + .start_head = pipe->head,
> + .count = count,
> + .iov_offset = 0
> + };
> }
> EXPORT_SYMBOL(iov_iter_pipe);
>
> @@ -1220,9 +1237,11 @@ EXPORT_SYMBOL(iov_iter_pipe);
> void iov_iter_discard(struct iov_iter *i, unsigned int direction, size_t count)
> {
> BUG_ON(direction != READ);
> - i->type = ITER_DISCARD | READ;
> - i->count = count;
> - i->iov_offset = 0;
> + *i = (struct iov_iter) {
> + .iter_type = ITER_DISCARD,
> + .data_source = false,
> + .count = count,
> + };
> }
> EXPORT_SYMBOL(iov_iter_discard);
>
>
--
Pavel Begunkov
Powered by blists - more mailing lists