Message ID | 167112117887.152641.6194213035340041732.stgit@warthog.procyon.org.uk |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:e747:0:0:0:0:0 with SMTP id c7csp459261wrn; Thu, 15 Dec 2022 08:24:11 -0800 (PST) X-Google-Smtp-Source: AA0mqf4PUOx7SwpDtXqx/VS+DkozPSTItJWj4RT1YXWlEO3gnw1ViO5ErfwoQFgCbdnvf3OqnIws X-Received: by 2002:a05:6a20:9d8f:b0:a9:ec2a:100e with SMTP id mu15-20020a056a209d8f00b000a9ec2a100emr35381187pzb.32.1671121451262; Thu, 15 Dec 2022 08:24:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1671121451; cv=none; d=google.com; s=arc-20160816; b=AHYkmj3J2dP9bncAnPUUr4vBV6XscKr3NLw3xjqF5+/wqNUEXnAUL5Nj1hN5JBtOGH 9+IPEwhfq31A27Do0IZRvLXzxMlcu9Ghb5s2El+Jj1Q2MSjIUTV6WynRFPZ6KiJBsDYE +BwxVqMyEpYstkjjj32WBdQ7OS6mVdg2EKmdCJtqFjKxgmi8R4fGwIsXP3wXjkM55Wo5 MXgvd+aPzrl/MeU1qocaJtWdCvIFvfpdEx9f+A6z/rXEz59X/QO5mqjVT3zBvqkc0OCc oP43PXIf/vdepv4kSCppm9cNapW7EYYao/ts3JB63iucMpXSWXhjVsBf3QGtFLaPVevq ieOA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:message-id:date:cc:to:from:subject:dkim-signature; bh=PeT2G/peKanAPSUrEqEYmZc7h4/xzdeLQWMNVJZY+ew=; b=XjkPzxn1MN8xqmXQGeUcLLfnJUSdIHL+sfiavaACKug6nQy1xzWnH2Uid7LcAUls9k Fq7VXIGI/0c08oQnLyjYSsQkxo8x3d7RZOsGLNuA8o1BELLrGHEg3PoWYXNpHBbhTOMg DAepfJ5wyU2W9NNm7SkdIp4ouIVQ3w1qClmEvwbzD6CCOIDdJMSblrDHwgb51V/l1weQ w9janjIA+gpYVahheFG0M2JokOjIKvwgzwwLxfMucKH/dZQlDSx3/qqUshtegBdkw28Z l43E//LYm31FQb0AbDuHEAVbGR7PWXFRUsqXkmz2aOFbmwTKOA+7igweZZJ95+ByE33e HhVg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="IqQb/vUn"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 203-20020a6304d4000000b0046040a8be4esi3000775pge.754.2022.12.15.08.23.57; Thu, 15 Dec 2022 08:24:11 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="IqQb/vUn"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229911AbiLOQUt (ORCPT <rfc822;jeantsuru.cumc.mandola@gmail.com> + 99 others); Thu, 15 Dec 2022 11:20:49 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48238 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229975AbiLOQUi (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 15 Dec 2022 11:20:38 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9E49A31347 for <linux-kernel@vger.kernel.org>; Thu, 15 Dec 2022 08:19:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1671121194; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=PeT2G/peKanAPSUrEqEYmZc7h4/xzdeLQWMNVJZY+ew=; b=IqQb/vUna2Y0eFdWymo3CAvtsc1gb6/IVp0wLEepQFQT+qRWEQxLYdTD4LqEUsZD/luI42 abXbmRe+3wbWoxvf3O5IHvygX1AfmYocZv1t+gaujiNybLKYh1SOEeLrp1u9CQ7LMAUfIM 2+PiWLwt1a1JMV2tVjWSbsNloyZnnvI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-623-Ph_XzNJuMjO3L4o-24vpKg-1; Thu, 15 Dec 2022 11:19:46 -0500 X-MC-Unique: Ph_XzNJuMjO3L4o-24vpKg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B764D100F909; Thu, 15 Dec 2022 16:19:42 +0000 (UTC) Received: from warthog.procyon.org.uk (unknown [10.33.36.96]) by smtp.corp.redhat.com (Postfix) with ESMTP id 90C0151FF; Thu, 15 Dec 2022 16:19:41 +0000 (UTC) Subject: [PATCH net 0/9] rxrpc: Fixes for I/O thread conversion/SACK table expansion From: David Howells <dhowells@redhat.com> To: netdev@vger.kernel.org Cc: Dan Carpenter <error27@gmail.com>, linux-afs@lists.infradead.org, Marc Dionne <marc.dionne@auristor.com>, Hillf Danton <hdanton@sina.com>, syzbot+3538a6a72efa8b059c38@syzkaller.appspotmail.com, dhowells@redhat.com, linux-afs@lists.infradead.org, linux-kernel@vger.kernel.org Date: Thu, 15 Dec 2022 16:19:38 +0000 Message-ID: <167112117887.152641.6194213035340041732.stgit@warthog.procyon.org.uk> User-Agent: StGit/1.5 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1752297846660282323?= X-GMAIL-MSGID: =?utf-8?q?1752297846660282323?= |
Series |
rxrpc: Fixes for I/O thread conversion/SACK table expansion
|
|
Message
David Howells
Dec. 15, 2022, 4:19 p.m. UTC
Here are some fixes for AF_RXRPC: (1) Fix missing unlock in rxrpc's sendmsg. (2) Fix (lack of) propagation of security settings to rxrpc_call. (3) Fix NULL ptr deref in rxrpc_unuse_local(). (4) Fix problem with kthread_run() not invoking the I/O thread function if the kthread gets stopped first. Possibly this should actually be fixed in the kthread code. (5) Fix locking problem as putting a peer (which may be done from RCU) may now invoke kthread_stop(). (6) Fix switched parameters in a couple of trace calls. (7) Fix I/O thread's checking for kthread stop to make sure it completes all outstanding work before returning so that calls are cleaned up. (8) Fix an uninitialised var in the new rxperf test server. (9) Fix the return value of rxrpc_new_incoming_call() so that the checks on it work correctly. The patches fix at least one syzbot bug[1] and probably some others that don't have reproducers[2][3][4]. I think it also fixes another[5], but that showed another failure during testing that was different to the original. There's also an outstanding bug in rxrpc_put_peer()[6] that is fixed by a combination of several patches in my rxrpc-next branch, but I haven't included that here. The patches are tagged here: git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git rxrpc-fixes-20221215 and can also be found on the following branch: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes David Link: https://syzkaller.appspot.com/bug?extid=3538a6a72efa8b059c38 [1] Link: https://syzkaller.appspot.com/bug?extid=2a99eae8dc7c754bc16b [2] Link: https://syzkaller.appspot.com/bug?extid=e1391a5bf3f779e31237 [3] Link: https://syzkaller.appspot.com/bug?extid=2aea8e1c8e20cb27a01f [4] Link: https://syzkaller.appspot.com/bug?extid=1eb4232fca28c0a6d1c2 [5] Link: https://syzkaller.appspot.com/bug?extid=c22650d2844392afdcfd [6] --- David Howells (9): rxrpc: Fix missing unlock in rxrpc_do_sendmsg() rxrpc: Fix security setting propagation rxrpc: Fix NULL deref in rxrpc_unuse_local() rxrpc: Fix I/O thread startup getting skipped rxrpc: Fix locking issues in rxrpc_put_peer_locked() rxrpc: Fix switched parameters in peer tracing rxrpc: Fix I/O thread stop rxrpc: rxperf: Fix uninitialised variable rxrpc: Fix the return value of rxrpc_new_incoming_call() include/trace/events/rxrpc.h | 2 +- net/rxrpc/ar-internal.h | 8 ++++---- net/rxrpc/call_accept.c | 18 +++++++++--------- net/rxrpc/call_object.c | 1 + net/rxrpc/conn_client.c | 2 -- net/rxrpc/io_thread.c | 10 +++++++--- net/rxrpc/local_object.c | 5 ++++- net/rxrpc/peer_event.c | 10 +++++++--- net/rxrpc/peer_object.c | 23 ++--------------------- net/rxrpc/rxperf.c | 2 +- net/rxrpc/security.c | 6 +++--- net/rxrpc/sendmsg.c | 2 +- 12 files changed, 40 insertions(+), 49 deletions(-)
Comments
On Thu, Dec 15, 2022 at 12:20 PM David Howells <dhowells@redhat.com> wrote: > > > Here are some fixes for AF_RXRPC: > > (1) Fix missing unlock in rxrpc's sendmsg. > > (2) Fix (lack of) propagation of security settings to rxrpc_call. > > (3) Fix NULL ptr deref in rxrpc_unuse_local(). > > (4) Fix problem with kthread_run() not invoking the I/O thread function if > the kthread gets stopped first. Possibly this should actually be > fixed in the kthread code. > > (5) Fix locking problem as putting a peer (which may be done from RCU) may > now invoke kthread_stop(). > > (6) Fix switched parameters in a couple of trace calls. > > (7) Fix I/O thread's checking for kthread stop to make sure it completes > all outstanding work before returning so that calls are cleaned up. > > (8) Fix an uninitialised var in the new rxperf test server. > > (9) Fix the return value of rxrpc_new_incoming_call() so that the checks > on it work correctly. > > The patches fix at least one syzbot bug[1] and probably some others that > don't have reproducers[2][3][4]. I think it also fixes another[5], but > that showed another failure during testing that was different to the > original. > > There's also an outstanding bug in rxrpc_put_peer()[6] that is fixed by a > combination of several patches in my rxrpc-next branch, but I haven't > included that here. > > The patches are tagged here: > > git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git > rxrpc-fixes-20221215 > > and can also be found on the following branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes > > David > > Link: https://syzkaller.appspot.com/bug?extid=3538a6a72efa8b059c38 [1] > Link: https://syzkaller.appspot.com/bug?extid=2a99eae8dc7c754bc16b [2] > Link: https://syzkaller.appspot.com/bug?extid=e1391a5bf3f779e31237 [3] > Link: https://syzkaller.appspot.com/bug?extid=2aea8e1c8e20cb27a01f [4] > Link: https://syzkaller.appspot.com/bug?extid=1eb4232fca28c0a6d1c2 [5] > Link: https://syzkaller.appspot.com/bug?extid=c22650d2844392afdcfd [6] > > --- > David Howells (9): > rxrpc: Fix missing unlock in rxrpc_do_sendmsg() > rxrpc: Fix security setting propagation > rxrpc: Fix NULL deref in rxrpc_unuse_local() > rxrpc: Fix I/O thread startup getting skipped > rxrpc: Fix locking issues in rxrpc_put_peer_locked() > rxrpc: Fix switched parameters in peer tracing > rxrpc: Fix I/O thread stop > rxrpc: rxperf: Fix uninitialised variable > rxrpc: Fix the return value of rxrpc_new_incoming_call() > > > include/trace/events/rxrpc.h | 2 +- > net/rxrpc/ar-internal.h | 8 ++++---- > net/rxrpc/call_accept.c | 18 +++++++++--------- > net/rxrpc/call_object.c | 1 + > net/rxrpc/conn_client.c | 2 -- > net/rxrpc/io_thread.c | 10 +++++++--- > net/rxrpc/local_object.c | 5 ++++- > net/rxrpc/peer_event.c | 10 +++++++--- > net/rxrpc/peer_object.c | 23 ++--------------------- > net/rxrpc/rxperf.c | 2 +- > net/rxrpc/security.c | 6 +++--- > net/rxrpc/sendmsg.c | 2 +- > 12 files changed, 40 insertions(+), 49 deletions(-) For the series: Tested-by: Marc Dionne <marc.dionne@auristor.com> Tested-by: kafs-testing+fedora36_64checkkafs-build-164@auristor.com Marc
Hillf Danton <hdanton@sina.com> wrote: > > @@ -478,13 +479,14 @@ int rxrpc_io_thread(void *data) > > } > > > > set_current_state(TASK_INTERRUPTIBLE); > > + should_stop = kthread_should_stop(); > > if (!skb_queue_empty(&local->rx_queue) || > > !list_empty(&local->call_attend_q)) { > > __set_current_state(TASK_RUNNING); > > continue; > > } > > > > - if (kthread_should_stop()) > > + if (should_stop) > > break; > > schedule(); > > } > > At the second glance still fail to see the difference this change can > make. There is a window here: if (!skb_queue_empty(&local->rx_queue) ...) continue; ---> if (kthread_should_stop()) break; in which an event can happen that should be attended to. For instance the AF_RXRPC socket being closed, aborting all its calls and stopping the kthread by doing the final unuse on its rxrpc_local struct - in that order. The window can be expanded by an interrupt or softirq handler running. So once we've observed that we've been asked to stop, we need to check if there's more work to be done and, if so, do that work first. So by doing: should_stop = kthread_should_stop(); if (!skb_queue_empty(&local->rx_queue) ...) continue; if (should_stop) break; we do all outstanding cleanup work before exiting the loop to stop the thread. David
Hillf Danton <hdanton@sina.com> wrote: > In line with > > if (condition) > return; > add to wait queue > if (!condition) > schedule(); > > this change should look like > > if (!skb_queue_empty(&local->rx_queue) ...) > continue; > > if (kthread_should_stop()) > if (!skb_queue_empty(&local->rx_queue) ...) > continue; > else > break; > > as checking condition once barely makes sense. Really, no. The condition is going to expand to have a whole bunch of things in it and I don't want to have it twice, e.g.: if (!skb_queue_empty(&local->rx_queue) || READ_ONCE(local->events) || !list_empty(&local->call_attend_q) || !list_empty(&local->conn_attend_q) || !list_empty(&local->new_client_calls) || test_bit(RXRPC_CLIENT_CONN_REAP_TIMER, &local->client_conn_flags)) { ... Hmmm... I wonder if kthread_should_stop() needs a barrier associated with it. It's just a test_bit(), so the compiler can cache the results of all these tests - or reorder them. David
Hillf Danton <hdanton@sina.com> wrote: > > So once we've observed that we've been asked to stop, we need to check if > > there's more work to be done and, if so, do that work first. > > In line with > > if (condition) > return; > add to wait queue > if (!condition) > schedule(); > > this change should look like > > if (!skb_queue_empty(&local->rx_queue) ...) > continue; > > if (kthread_should_stop()) > if (!skb_queue_empty(&local->rx_queue) ...) > continue; > else > break; > > as checking condition once barely makes sense. Note that these are not really analogous. The add-to-wait-queue step is significantly more expensive than kthread_should_stop() and requires removal in the event that the condition becomes true in the window. In the case of kthread_should_stop(), it's just a test_bit() of a word that's in a cacheline not going to get changed until the thread is stopped. Testing the value first and then checking the condition should be fine as the stop flag can be shared in the cpu's data cache until it's set. Also from a code-maintenance PoV, I don't want to write the condition twice if I can avoid it. That allows for the two copies to get out of sync. > Because of a bit complex condition does not mean checking it once is neither > sane nor correct. So you agree with me, I think? David
Hello: This series was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Thu, 15 Dec 2022 16:19:38 +0000 you wrote: > Here are some fixes for AF_RXRPC: > > (1) Fix missing unlock in rxrpc's sendmsg. > > (2) Fix (lack of) propagation of security settings to rxrpc_call. > > (3) Fix NULL ptr deref in rxrpc_unuse_local(). > > [...] Here is the summary with links: - [net,1/9] rxrpc: Fix missing unlock in rxrpc_do_sendmsg() https://git.kernel.org/netdev/net/c/4feb2c44629e - [net,2/9] rxrpc: Fix security setting propagation https://git.kernel.org/netdev/net/c/fdb99487b018 - [net,3/9] rxrpc: Fix NULL deref in rxrpc_unuse_local() https://git.kernel.org/netdev/net/c/eaa02390adb0 - [net,4/9] rxrpc: Fix I/O thread startup getting skipped https://git.kernel.org/netdev/net/c/8fbcc83334a7 - [net,5/9] rxrpc: Fix locking issues in rxrpc_put_peer_locked() https://git.kernel.org/netdev/net/c/608aecd16a31 - [net,6/9] rxrpc: Fix switched parameters in peer tracing https://git.kernel.org/netdev/net/c/c838f1a73d77 - [net,7/9] rxrpc: Fix I/O thread stop https://git.kernel.org/netdev/net/c/743d1768a008 - [net,8/9] rxrpc: rxperf: Fix uninitialised variable https://git.kernel.org/netdev/net/c/11e1706bc84f - [net,9/9] rxrpc: Fix the return value of rxrpc_new_incoming_call() https://git.kernel.org/netdev/net/c/31d35a02ad5b You are awesome, thank you!