[PATCH net 1/1] tipc: revert use of copy_from_iter_full()

[PATCH net 1/1] tipc: revert use of copy_from_iter_full()

[Jon Maloy]

commit cbbd26b8b1a6 ("[iov_iter] new primitives - copy_from_iter_full()
and friends") replaced calls to copy_from_iter() in the function
tipc_msg_build(). This causes a an immediate crash as follows:

[ 1209.597076] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
[ 1209.607025] IP: copy_from_iter_full+0x43/0x290
[ 1209.611617] PGD 130f63067
[ 1209.611621] PUD 130f64067
[ 1209.614437] PMD 0
[ 1209.616966]
[ 1209.620351] Oops: 0000 [#1] SMP
[ 1209.622739] Modules linked in: tipc(E) ip6_udp_tunnel udp_tunnel rpcsec_gss_krb5 auth_rpcgss nfsv4 nfs lockd xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat grace fscache nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables ppdev joydev serio_raw parport_pc parport i2c_piix4 sunrpc autofs4 floppy psmouse pata_acpi [last unloaded: tipc]
[ 1209.643115] CPU: 7 PID: 1911 Comm: tipcTC Tainted: G            E   4.9.0+ #619
[ 1209.647707] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 1209.653393] task: ffff96c2f0c58000 task.stack: ffffb7cdc07a8000
[ 1209.656626] RIP: 0010:copy_from_iter_full+0x43/0x290
[ 1209.659412] RSP: 0018:ffffb7cdc07abc38 EFLAGS: 00010246
[ 1209.662459] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000018
[ 1209.666443] RDX: ffffb7cdc07abe00 RSI: 0000000000000000 RDI: ffff96c2f39e86b8
[ 1209.669918] RBP: ffffb7cdc07abc78 R08: 0000000000000000 R09: 0000000000000000
[ 1209.673379] R10: ffff96c2f50032c0 R11: ffff96c2f39e8600 R12: ffffb7cdc07abe00
[ 1209.676585] R13: 0000000000000000 R14: ffff96c2f39e86b8 R15: 0000000000000000
[ 1209.678573] FS:  00007fb5db2a3700(0000) GS:ffff96c2f9dc0000(0000) knlGS:0000000000000000
[ 1209.681433] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1209.684321] CR2: 0000000000000008 CR3: 0000000130f66000 CR4: 00000000000406e0
[ 1209.687676] Call Trace:
[ 1209.689257]  tipc_msg_build+0xe1/0x590 [tipc]
[ 1209.691479]  ? _raw_spin_unlock_bh+0x1e/0x20
[ 1209.694641]  ? tipc_node_find+0x30/0xa0 [tipc]
[ 1209.696789]  __tipc_sendmsg+0x189/0x480 [tipc]
[ 1209.699017]  ? remove_wait_queue+0x4d/0x60
[ 1209.700354]  tipc_connect+0x15f/0x1b0 [tipc]
[ 1209.701684]  SYSC_connect+0xd9/0x110
[ 1209.702847]  ? sock_alloc_file+0xa6/0x130
[ 1209.704083]  ? fd_install+0x25/0x30
[ 1209.705195]  ? sock_map_fd+0x44/0x70
[ 1209.706331]  SyS_connect+0xe/0x10
[ 1209.707385]  entry_SYSCALL_64_fastpath+0x1e/0xad
[ 1209.708714] RIP: 0033:0x7fb5dadca870
[ 1209.709700] RSP: 002b:00007fff6ea43978 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
[ 1209.711948] RAX: ffffffffffffffda RBX: 0000000000000009 RCX: 00007fb5dadca870
[ 1209.713804] RDX: 0000000000000010 RSI: 00007fff6ea43990 RDI: 0000000000000003
[ 1209.715413] RBP: 00000000012e8010 R08: 0000000000008004 R09: 0000000000000009
[ 1209.716440] R10: 000000000000012a R11: 0000000000000246 R12: 0000000000000009
[ 1209.717461] R13: 0000000000000001 R14: 00007fb5db087620 R15: 0000000000404fe4
[ 1209.718486] Code: 01 00 00 48 39 72 10 49 89 f5 49 89 d4 0f 82 69 01 00 00 48 8b 72 08 45 31 c9 a8 04 49 89 fe 45 89 e8 41 89 f7 75 6f 4c 8b 7a 18 <49> 8b 5f 08 48 29 f3 4c 39 eb 49 0f 47 dd a8 02 0f 85 6e 01 00
[ 1209.721328] RIP: copy_from_iter_full+0x43/0x290 RSP: ffffb7cdc07abc38
[ 1209.723573] CR2: 0000000000000008
[ 1209.725262] ---[ end trace 752756f533c3f533 ]---
[ 1209.726603] Kernel panic - not syncing: Fatal exception
[ 1209.728293] Kernel Offset: 0x2f000000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[ 1209.731225] ---[ end Kernel panic - not syncing: Fatal exception

When we revert the change everything works fine, so we choose this solution for now.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/msg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index a22be50..17201aa 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -268,7 +268,7 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,
 		__skb_queue_tail(list, skb);
 		skb_copy_to_linear_data(skb, mhdr, mhsz);
 		pktpos = skb->data + mhsz;
-		if (copy_from_iter_full(pktpos, dsz, &m->msg_iter))
+		if (copy_from_iter(pktpos, dsz, &m->msg_iter) == dsz)
 			return dsz;
 		rc = -EFAULT;
 		goto error;
@@ -299,7 +299,7 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,
 		if (drem < pktrem)
 			pktrem = drem;
 
-		if (!copy_from_iter_full(pktpos, pktrem, &m->msg_iter)) {
+		if (copy_from_iter(pktpos, pktrem, &m->msg_iter) != pktrem) {
 			rc = -EFAULT;
 			goto error;
 		}
-- 
2.7.4

Re: [PATCH net 1/1] tipc: revert use of copy_from_iter_full()

[Al Viro]

On Wed, Dec 21, 2016 at 08:01:37PM -0500, Jon Maloy wrote:
> commit cbbd26b8b1a6 ("[iov_iter] new primitives - copy_from_iter_full()
> and friends") replaced calls to copy_from_iter() in the function
> tipc_msg_build(). This causes a an immediate crash as follows:

Very interesting.

> [ 1209.597076] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [ 1209.607025] IP: copy_from_iter_full+0x43/0x290

> [ 1209.689257]  tipc_msg_build+0xe1/0x590 [tipc]
> [ 1209.691479]  ? _raw_spin_unlock_bh+0x1e/0x20
> [ 1209.694641]  ? tipc_node_find+0x30/0xa0 [tipc]
> [ 1209.696789]  __tipc_sendmsg+0x189/0x480 [tipc]
> [ 1209.699017]  ? remove_wait_queue+0x4d/0x60
> [ 1209.700354]  tipc_connect+0x15f/0x1b0 [tipc]
> [ 1209.701684]  SYSC_connect+0xd9/0x110

I don't believe that it's something tipc-specific; could you post an objdump
of copy_from_iter_full() in your kernel?  That smells like a bug in there
and it really ought to be fixed...

Re: [PATCH net 1/1] tipc: revert use of copy_from_iter_full()

[Al Viro]

On Thu, Dec 22, 2016 at 01:21:01AM +0000, Al Viro wrote:
> On Wed, Dec 21, 2016 at 08:01:37PM -0500, Jon Maloy wrote:
> > commit cbbd26b8b1a6 ("[iov_iter] new primitives - copy_from_iter_full()
> > and friends") replaced calls to copy_from_iter() in the function
> > tipc_msg_build(). This causes a an immediate crash as follows:
> 
> Very interesting.
> 
> > [ 1209.597076] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> > [ 1209.607025] IP: copy_from_iter_full+0x43/0x290
> 
> > [ 1209.689257]  tipc_msg_build+0xe1/0x590 [tipc]
> > [ 1209.691479]  ? _raw_spin_unlock_bh+0x1e/0x20
> > [ 1209.694641]  ? tipc_node_find+0x30/0xa0 [tipc]
> > [ 1209.696789]  __tipc_sendmsg+0x189/0x480 [tipc]
> > [ 1209.699017]  ? remove_wait_queue+0x4d/0x60
> > [ 1209.700354]  tipc_connect+0x15f/0x1b0 [tipc]
> > [ 1209.701684]  SYSC_connect+0xd9/0x110
> 
> I don't believe that it's something tipc-specific; could you post an objdump
> of copy_from_iter_full() in your kernel?  That smells like a bug in there
> and it really ought to be fixed...

FWIW, looking at the tipc, am I reading the trace correctly?  We seem to
have tipc_connect() taking an msghdr with empty payload and hitting this
        switch (sk->sk_state) {
        case TIPC_OPEN:
                /* Send a 'SYN-' to destination */
                m.msg_name = dest;
                m.msg_namelen = destlen;

                /* If connect is in non-blocking case, set MSG_DONTWAIT to
                 * indicate send_msg() is never blocked.
                 */
                if (!timeout)
                        m.msg_flags = MSG_DONTWAIT;

                res = __tipc_sendmsg(sock, &m, 0);
which eventually calls
        rc = tipc_msg_build(mhdr, m, 0, dsz, mtu, &pktchain);
possibly more than once, but with explicit "restore m->msg_iter to what
it was before the first call" before each subsequent call.

What's putting anything into m.msg_iter on that codepath?  AFAICS, it should
be completely empty...  Wait.  AAARRRGH!

OK, I see what's going on there - unlike iterate_and_advance(), which
explicitly skips any work in case of empty iterator, iterate_all_kind()
does not.  Could you check if the following fixes your problem?

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 228892dabba6..6a0396b8d47f 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -73,19 +73,21 @@
 }
 
 #define iterate_all_kinds(i, n, v, I, B, K) {			\
-	size_t skip = i->iov_offset;				\
-	if (unlikely(i->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)) {		\
-		const struct kvec *kvec;			\
-		struct kvec v;					\
-		iterate_kvec(i, n, v, kvec, skip, (K))		\
-	} else {						\
-		const struct iovec *iov;			\
-		struct iovec v;					\
-		iterate_iovec(i, n, v, iov, skip, (I))		\
+	if (i->count) {						\
+		size_t skip = i->iov_offset;			\
+		if (unlikely(i->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)) {	\
+			const struct kvec *kvec;		\
+			struct kvec v;				\
+			iterate_kvec(i, n, v, kvec, skip, (K))	\
+		} else {					\
+			const struct iovec *iov;		\
+			struct iovec v;				\
+			iterate_iovec(i, n, v, iov, skip, (I))	\
+		}						\
 	}							\
 }
 

Re: [PATCH net 1/1] tipc: revert use of copy_from_iter_full()

[Jon Maloy]

> -----Original Message-----
> From: Al Viro [mailto:viro@ftp.linux.org.uk] On Behalf Of Al Viro
> Sent: Wednesday, 21 December, 2016 20:43
> To: Jon Maloy <jon.maloy@ericsson.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Parthasarathy Bhuvaragan
> <parthasarathy.bhuvaragan@ericsson.com>; Ying Xue
> <ying.xue@windriver.com>; maloy@donjonn.com; tipc-
> discussion@lists.sourceforge.net
> Subject: Re: [PATCH net 1/1] tipc: revert use of copy_from_iter_full()
> 
> On Thu, Dec 22, 2016 at 01:21:01AM +0000, Al Viro wrote:
> > On Wed, Dec 21, 2016 at 08:01:37PM -0500, Jon Maloy wrote:
> > > commit cbbd26b8b1a6 ("[iov_iter] new primitives - copy_from_iter_full()
> > > and friends") replaced calls to copy_from_iter() in the function
> > > tipc_msg_build(). This causes a an immediate crash as follows:
> >
> > Very interesting.
> >
> > > [ 1209.597076] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000008
> > > [ 1209.607025] IP: copy_from_iter_full+0x43/0x290
> >
> > > [ 1209.689257]  tipc_msg_build+0xe1/0x590 [tipc]
> > > [ 1209.691479]  ? _raw_spin_unlock_bh+0x1e/0x20
> > > [ 1209.694641]  ? tipc_node_find+0x30/0xa0 [tipc]
> > > [ 1209.696789]  __tipc_sendmsg+0x189/0x480 [tipc]
> > > [ 1209.699017]  ? remove_wait_queue+0x4d/0x60
> > > [ 1209.700354]  tipc_connect+0x15f/0x1b0 [tipc]
> > > [ 1209.701684]  SYSC_connect+0xd9/0x110
> >
> > I don't believe that it's something tipc-specific; could you post an objdump
> > of copy_from_iter_full() in your kernel?  That smells like a bug in there
> > and it really ought to be fixed...
> 
> FWIW, looking at the tipc, am I reading the trace correctly?  We seem to
> have tipc_connect() taking an msghdr with empty payload and hitting this
>         switch (sk->sk_state) {
>         case TIPC_OPEN:
>                 /* Send a 'SYN-' to destination */
>                 m.msg_name = dest;
>                 m.msg_namelen = destlen;
> 
>                 /* If connect is in non-blocking case, set MSG_DONTWAIT to
>                  * indicate send_msg() is never blocked.
>                  */
>                 if (!timeout)
>                         m.msg_flags = MSG_DONTWAIT;
> 
>                 res = __tipc_sendmsg(sock, &m, 0);
> which eventually calls
>         rc = tipc_msg_build(mhdr, m, 0, dsz, mtu, &pktchain);
> possibly more than once, but with explicit "restore m->msg_iter to what
> it was before the first call" before each subsequent call.
> 
> What's putting anything into m.msg_iter on that codepath?  AFAICS, it should
> be completely empty...  Wait.  AAARRRGH!
> 
> OK, I see what's going on there - unlike iterate_and_advance(), which
> explicitly skips any work in case of empty iterator, iterate_all_kind()
> does not.  Could you check if the following fixes your problem?

Confirmed. The crash disappeared and everything works fine.

BR
///jon

> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 228892dabba6..6a0396b8d47f 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -73,19 +73,21 @@
>  }
> 
>  #define iterate_all_kinds(i, n, v, I, B, K) {			\
> -	size_t skip = i->iov_offset;				\
> -	if (unlikely(i->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)) {		\
> -		const struct kvec *kvec;			\
> -		struct kvec v;					\
> -		iterate_kvec(i, n, v, kvec, skip, (K))		\
> -	} else {						\
> -		const struct iovec *iov;			\
> -		struct iovec v;					\
> -		iterate_iovec(i, n, v, iov, skip, (I))		\
> +	if (i->count) {						\
> +		size_t skip = i->iov_offset;			\
> +		if (unlikely(i->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)) {	\
> +			const struct kvec *kvec;		\
> +			struct kvec v;				\
> +			iterate_kvec(i, n, v, kvec, skip, (K))	\
> +		} else {					\
> +			const struct iovec *iov;		\
> +			struct iovec v;				\
> +			iterate_iovec(i, n, v, iov, skip, (I))	\
> +		}						\
>  	}							\
>  }
> 

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/intel

Re: [PATCH net 1/1] tipc: revert use of copy_from_iter_full()

[Al Viro]

On Thu, Dec 22, 2016 at 02:47:09AM +0000, Jon Maloy wrote:

> > OK, I see what's going on there - unlike iterate_and_advance(), which
> > explicitly skips any work in case of empty iterator, iterate_all_kind()
> > does not.  Could you check if the following fixes your problem?
> 
> Confirmed. The crash disappeared and everything works fine.

OK...  This is the somewhat cleaned version of the same that will go to Linus,
assuming it survives the local beating.  Hopefully, cleanups hadn't broken it
in your case...

[iov_iter] fix iterate_all_kinds() on empty iterators

Problem similar to ones dealt with in "fold checks into iterate_and_advance()"
and followups, except that in this case we really want to do nothing when
asked for zero-length operation - unlike zero-length iterate_and_advance(),
zero-length iterate_all_kinds() has no side effects, and callers are simpler
that way.

That got exposed when copy_from_iter_full() had been used by tipc, which
builds an msghdr with zero payload and (now) feeds it to a primitive
based on iterate_all_kinds() instead of iterate_and_advance().

Reported-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 228892dabba6..25f572303801 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -73,19 +73,21 @@
 }
 
 #define iterate_all_kinds(i, n, v, I, B, K) {			\
-	size_t skip = i->iov_offset;				\
-	if (unlikely(i->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)) {		\
-		const struct kvec *kvec;			\
-		struct kvec v;					\
-		iterate_kvec(i, n, v, kvec, skip, (K))		\
-	} else {						\
-		const struct iovec *iov;			\
-		struct iovec v;					\
-		iterate_iovec(i, n, v, iov, skip, (I))		\
+	if (likely(n)) {					\
+		size_t skip = i->iov_offset;			\
+		if (unlikely(i->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)) {	\
+			const struct kvec *kvec;		\
+			struct kvec v;				\
+			iterate_kvec(i, n, v, kvec, skip, (K))	\
+		} else {					\
+			const struct iovec *iov;		\
+			struct iovec v;				\
+			iterate_iovec(i, n, v, iov, skip, (I))	\
+		}						\
 	}							\
 }
 
@@ -576,7 +578,7 @@ bool copy_from_iter_full(void *addr, size_t bytes, struct iov_iter *i)
 		WARN_ON(1);
 		return false;
 	}
-	if (unlikely(i->count < bytes))				\
+	if (unlikely(i->count < bytes))
 		return false;
 
 	iterate_all_kinds(i, bytes, v, ({
@@ -620,7 +622,7 @@ bool copy_from_iter_full_nocache(void *addr, size_t bytes, struct iov_iter *i)
 		WARN_ON(1);
 		return false;
 	}
-	if (unlikely(i->count < bytes))				\
+	if (unlikely(i->count < bytes))
 		return false;
 	iterate_all_kinds(i, bytes, v, ({
 		if (__copy_from_user_nocache((to += v.iov_len) - v.iov_len,
@@ -837,11 +839,8 @@ unsigned long iov_iter_alignment(const struct iov_iter *i)
 	unsigned long res = 0;
 	size_t size = i->count;
 
-	if (!size)
-		return 0;
-
 	if (unlikely(i->type & ITER_PIPE)) {
-		if (i->iov_offset && allocated(&i->pipe->bufs[i->idx]))
+		if (size && i->iov_offset && allocated(&i->pipe->bufs[i->idx]))
 			return size | i->iov_offset;
 		return size;
 	}
@@ -856,10 +855,8 @@ EXPORT_SYMBOL(iov_iter_alignment);
 
 unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
 {
-        unsigned long res = 0;
+	unsigned long res = 0;
 	size_t size = i->count;
-	if (!size)
-		return 0;
 
 	if (unlikely(i->type & ITER_PIPE)) {
 		WARN_ON(1);
@@ -874,7 +871,7 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
 		(res |= (!res ? 0 : (unsigned long)v.iov_base) |
 			(size != v.iov_len ? size : 0))
 		);
-		return res;
+	return res;
 }
 EXPORT_SYMBOL(iov_iter_gap_alignment);
 
@@ -908,6 +905,9 @@ static ssize_t pipe_get_pages(struct iov_iter *i,
 	size_t capacity;
 	int idx;
 
+	if (!maxsize)
+		return 0;
+
 	if (!sanity(i))
 		return -EFAULT;
 
@@ -926,9 +926,6 @@ ssize_t iov_iter_get_pages(struct iov_iter *i,
 	if (maxsize > i->count)
 		maxsize = i->count;
 
-	if (!maxsize)
-		return 0;
-
 	if (unlikely(i->type & ITER_PIPE))
 		return pipe_get_pages(i, pages, maxsize, maxpages, start);
 	iterate_all_kinds(i, maxsize, v, ({
@@ -975,6 +972,9 @@ static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
 	int idx;
 	int npages;
 
+	if (!maxsize)
+		return 0;
+
 	if (!sanity(i))
 		return -EFAULT;
 
@@ -1006,9 +1006,6 @@ ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
 	if (maxsize > i->count)
 		maxsize = i->count;
 
-	if (!maxsize)
-		return 0;
-
 	if (unlikely(i->type & ITER_PIPE))
 		return pipe_get_pages_alloc(i, pages, maxsize, start);
 	iterate_all_kinds(i, maxsize, v, ({

Re: [PATCH net 1/1] tipc: revert use of copy_from_iter_full()

[Jon Maloy]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Al Viro
> Sent: Wednesday, 21 December, 2016 22:08
> To: Jon Maloy <jon.maloy@ericsson.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Parthasarathy Bhuvaragan
> <parthasarathy.bhuvaragan@ericsson.com>; Ying Xue
> <ying.xue@windriver.com>; maloy@donjonn.com; tipc-
> discussion@lists.sourceforge.net
> Subject: Re: [PATCH net 1/1] tipc: revert use of copy_from_iter_full()
> 
> On Thu, Dec 22, 2016 at 02:47:09AM +0000, Jon Maloy wrote:
> 
> > > OK, I see what's going on there - unlike iterate_and_advance(), which
> > > explicitly skips any work in case of empty iterator, iterate_all_kind()
> > > does not.  Could you check if the following fixes your problem?
> >
> > Confirmed. The crash disappeared and everything works fine.
> 
> OK...  This is the somewhat cleaned version of the same that will go to Linus,
> assuming it survives the local beating.  Hopefully, cleanups hadn't broken it
> in your case...

I tested it, and it still works with me.

///jon

> 
> [iov_iter] fix iterate_all_kinds() on empty iterators
> 
> Problem similar to ones dealt with in "fold checks into iterate_and_advance()"
> and followups, except that in this case we really want to do nothing when
> asked for zero-length operation - unlike zero-length iterate_and_advance(),
> zero-length iterate_all_kinds() has no side effects, and callers are simpler
> that way.
> 
> That got exposed when copy_from_iter_full() had been used by tipc, which
> builds an msghdr with zero payload and (now) feeds it to a primitive
> based on iterate_all_kinds() instead of iterate_and_advance().
> 
> Reported-by: Jon Maloy <jon.maloy@ericsson.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 228892dabba6..25f572303801 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -73,19 +73,21 @@
>  }
> 
>  #define iterate_all_kinds(i, n, v, I, B, K) {			\
> -	size_t skip = i->iov_offset;				\
> -	if (unlikely(i->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)) {		\
> -		const struct kvec *kvec;			\
> -		struct kvec v;					\
> -		iterate_kvec(i, n, v, kvec, skip, (K))		\
> -	} else {						\
> -		const struct iovec *iov;			\
> -		struct iovec v;					\
> -		iterate_iovec(i, n, v, iov, skip, (I))		\
> +	if (likely(n)) {					\
> +		size_t skip = i->iov_offset;			\
> +		if (unlikely(i->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)) {	\
> +			const struct kvec *kvec;		\
> +			struct kvec v;				\
> +			iterate_kvec(i, n, v, kvec, skip, (K))	\
> +		} else {					\
> +			const struct iovec *iov;		\
> +			struct iovec v;				\
> +			iterate_iovec(i, n, v, iov, skip, (I))	\
> +		}						\
>  	}							\
>  }
> 
> @@ -576,7 +578,7 @@ bool copy_from_iter_full(void *addr, size_t bytes, struct
> iov_iter *i)
>  		WARN_ON(1);
>  		return false;
>  	}
> -	if (unlikely(i->count < bytes))				\
> +	if (unlikely(i->count < bytes))
>  		return false;
> 
>  	iterate_all_kinds(i, bytes, v, ({
> @@ -620,7 +622,7 @@ bool copy_from_iter_full_nocache(void *addr, size_t
> bytes, struct iov_iter *i)
>  		WARN_ON(1);
>  		return false;
>  	}
> -	if (unlikely(i->count < bytes))				\
> +	if (unlikely(i->count < bytes))
>  		return false;
>  	iterate_all_kinds(i, bytes, v, ({
>  		if (__copy_from_user_nocache((to += v.iov_len) - v.iov_len,
> @@ -837,11 +839,8 @@ unsigned long iov_iter_alignment(const struct iov_iter
> *i)
>  	unsigned long res = 0;
>  	size_t size = i->count;
> 
> -	if (!size)
> -		return 0;
> -
>  	if (unlikely(i->type & ITER_PIPE)) {
> -		if (i->iov_offset && allocated(&i->pipe->bufs[i->idx]))
> +		if (size && i->iov_offset && allocated(&i->pipe->bufs[i->idx]))
>  			return size | i->iov_offset;
>  		return size;
>  	}
> @@ -856,10 +855,8 @@ EXPORT_SYMBOL(iov_iter_alignment);
> 
>  unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
>  {
> -        unsigned long res = 0;
> +	unsigned long res = 0;
>  	size_t size = i->count;
> -	if (!size)
> -		return 0;
> 
>  	if (unlikely(i->type & ITER_PIPE)) {
>  		WARN_ON(1);
> @@ -874,7 +871,7 @@ unsigned long iov_iter_gap_alignment(const struct
> iov_iter *i)
>  		(res |= (!res ? 0 : (unsigned long)v.iov_base) |
>  			(size != v.iov_len ? size : 0))
>  		);
> -		return res;
> +	return res;
>  }
>  EXPORT_SYMBOL(iov_iter_gap_alignment);
> 
> @@ -908,6 +905,9 @@ static ssize_t pipe_get_pages(struct iov_iter *i,
>  	size_t capacity;
>  	int idx;
> 
> +	if (!maxsize)
> +		return 0;
> +
>  	if (!sanity(i))
>  		return -EFAULT;
> 
> @@ -926,9 +926,6 @@ ssize_t iov_iter_get_pages(struct iov_iter *i,
>  	if (maxsize > i->count)
>  		maxsize = i->count;
> 
> -	if (!maxsize)
> -		return 0;
> -
>  	if (unlikely(i->type & ITER_PIPE))
>  		return pipe_get_pages(i, pages, maxsize, maxpages, start);
>  	iterate_all_kinds(i, maxsize, v, ({
> @@ -975,6 +972,9 @@ static ssize_t pipe_get_pages_alloc(struct iov_iter *i,
>  	int idx;
>  	int npages;
> 
> +	if (!maxsize)
> +		return 0;
> +
>  	if (!sanity(i))
>  		return -EFAULT;
> 
> @@ -1006,9 +1006,6 @@ ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
>  	if (maxsize > i->count)
>  		maxsize = i->count;
> 
> -	if (!maxsize)
> -		return 0;
> -
>  	if (unlikely(i->type & ITER_PIPE))
>  		return pipe_get_pages_alloc(i, pages, maxsize, start);
>  	iterate_all_kinds(i, maxsize, v, ({

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/intel