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

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

On 2023-02-05 23:11, Tung Quang Nguyen wrote:
>
>> -----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 ?
Yes, that makes sense. Go ahead.

///jon

>> ///jon