Re: [tipc-discussion] [PATCH v1 1/1] tipc: fix kernel warning when sending SYN message

[Jon M. <jm...@re...>]

On 2023-01-31 23:03, Tung Nguyen wrote:
> When sending a SYN message, this kernel stack trace is observed:
>
> ...
> [   13.396352] RIP: 0010:_copy_from_iter+0xb4/0x550
> ...
> [   13.398494] Call Trace:
> [   13.398630]  <TASK>
> [   13.398630]  ? __alloc_skb+0xed/0x1a0
> [   13.398630]  tipc_msg_build+0x12c/0x670 [tipc]
> [   13.398630]  ? shmem_add_to_page_cache.isra.71+0x151/0x290
> [   13.398630]  __tipc_sendmsg+0x2d1/0x710 [tipc]
> [   13.398630]  ? tipc_connect+0x1d9/0x230 [tipc]
> [   13.398630]  ? __local_bh_enable_ip+0x37/0x80
> [   13.398630]  tipc_connect+0x1d9/0x230 [tipc]
> [   13.398630]  ? __sys_connect+0x9f/0xd0
> [   13.398630]  __sys_connect+0x9f/0xd0
> [   13.398630]  ? preempt_count_add+0x4d/0xa0
> [   13.398630]  ? fpregs_assert_state_consistent+0x22/0x50
> [   13.398630]  __x64_sys_connect+0x16/0x20
> [   13.398630]  do_syscall_64+0x42/0x90
> [   13.398630]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> It is because commit a41dad905e5a ("iov_iter: saner checks for attempt to copy to/from iterator")
> has introduced sanity check for copying from/to iov iterator. Lacking
> of copy direction from the iterator viewpoint would lead to kernel
> stack trace like above.
>
> This commit fixes this issue by initializing the iov iterator with
> the correct copy direction.
>
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>   net/tipc/msg.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 5c9fd4791c4b..f40cd9032b62 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -377,10 +377,14 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, int offset,
>   	int pktno = 1;
>   	char *pktpos;
>   	int pktsz;
> +	struct iovec iov;
>   	int rc;
>   
>   	msg_set_size(mhdr, msz);
>   
> +	if (!dsz)
> +		iov_iter_init(&m->msg_iter, ITER_SOURCE, &iov, 0, 0);
> +
>   	/* No fragmentation needed? */
>   	if (likely(msz <= pktmax)) {
>   		skb = tipc_buf_acquire(msz, GFP_KERNEL);

I feel a little uncomfortable with adding an uninitialized struct iovec 
here.

Al Viro had a different proposal in his email from Dec 8th:

On 2022-12-11 04:38, Al Viro wrote:
> On Thu, Dec 08, 2022 at 08:38:14PM +0100, Eric Dumazet wrote:
>
>> Exposes an old bug in tipc ?
>>
>> Seems a new check added by Al in :
>>
>> Author: Al Viro <vi...@ze...>
>> Date:   Thu Sep 15 20:11:15 2022 -0400
>>
>>      iov_iter: saner checks for attempt to copy to/from iterator
>>
>>      instead of "don't do it to ITER_PIPE" check for ->data_source being
>>      false on copying from iterator.  Check for !->data_source for
>>      copying to iterator, while we are at it.
>>
>>      Signed-off-by: Al Viro <vi...@ze...>
> Lovely...  zero-length sendmsg with uninitialized ->msg_data...
>
> 	I would probably argue that it's a bug in tipc_connect(),
> fixed by iov_iter_kvec(&m.msg_iter, ITER_SOURCE, NULL, 0, 0);
> in there.  Depends - if that kind of uninitialized msg_iter used
> as zero length source or zero length destination is a frequent pattern,
> might as well make zero-byte copy_...iter() succeed quietly;
> I hope it isn't, but that's definitely something I'd missed
> when doing that series.
>
> 	I'll take a look tomorrow^Win the morning, after I get
> some sleep...
>

Did you consider that one?

///jon