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

[Tung Q. N. <tun...@de...>]

>-----Original Message-----
>From: Jon Maloy <jm...@re...>
>Sent: Saturday, February 4, 2023 1:02 AM
>To: Tung Quang Nguyen <tun...@de...>; tip...@li...; ma...@do...;
>yin...@wi...
>Subject: Re: [tipc-discussion][PATCH v1 1/1] tipc: fix kernel warning when sending SYN message
>
>
>
>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.
It is OK not using iovec, just passing NULL to iov_iter_init: iov_iter_init(&m->msg_iter, ITER_SOURCE, NULL, 0, 0).
My intention was to highlight the copy source and no-copy (count = 0) information.
>
>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?
This function has the same effect as the one I used. But from the viewpoint of function
tipc_msg_build(), we mainly want to copy data from user-space. Two exceptions are SYN and ACK- with no data. So, I think using iov_iter_init() makes more sense.
I suggest that we go with what I did (plus passing NULL for iovec) and CC Al Viro to see if the guy has any objection.
What do you think ?
>
>///jon