>-----Original Message-----
>From: Jon Maloy <jma...@redhat.com>
>Sent: Saturday, February 4, 2023 1:02 AM
>To: Tung Quang Nguyen <tung.q.ngu...@dektech.com.au>;
>tipc-discussion@lists.sourceforge.net; ma...@donjonn.com;
>ying....@windriver.com
>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 <tung.q.ngu...@dektech.com.au>
>> ---
>> 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 <v...@zeniv.linux.org.uk>
>>> 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 <v...@zeniv.linux.org.uk>
>> 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
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
