Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <xhsmho7xv512f.mognet@vschneid.remote.csb>
References: <Yrw5f8GN2fh2orid@zx2c4.com> <20220629114240.946411-1-Jason@zx2c4.com>
 <87v8s8ubws.fsf@kernel.org> <xhsmho7xv512f.mognet@vschneid.remote.csb>
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
Date:   Mon, 11 Jul 2022 13:53:31 +0200
Message-ID: <CAHmME9q8-1vpV9zFsKkawk+XFm96S6fmug7v-NPJNpQmRoe6-Q@mail.gmail.com>
Subject: Re: [PATCH v8] ath9k: let sleep be interrupted when unregistering hwrng
To:     Valentin Schneider <vschneid@redhat.com>
Cc:     Kalle Valo <kvalo@kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
        Gregory Erwin <gregerwin256@gmail.com>,
        =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>,
        Rui Salvaterra <rsalvaterra@gmail.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Christian Brauner <brauner@kernel.org>,
        linux-crypto@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Valentin,

On 7/11/22, Valentin Schneider <vschneid@redhat.com> wrote:
> On 07/07/22 19:26, Kalle Valo wrote:
>> "Jason A. Donenfeld" <Jason@zx2c4.com> writes:
>>
>>> There are two deadlock scenarios that need addressing, which cause
>>> problems when the computer goes to sleep, the interface is set down, an=
d
>>> hwrng_unregister() is called. When the deadlock is hit, sleep is delaye=
d
>>> for tens of seconds, causing it to fail. These scenarios are:
>>>
>>> 1) The hwrng kthread can't be stopped while it's sleeping, because it
>>>    uses msleep_interruptible() instead of
>>> schedule_timeout_interruptible().
>>>    The fix is a simple moving to the correct function. At the same time=
,
>>>    we should cleanup a common and useless dmesg splat in the same area.
>>>
>>> 2) A normal user thread can't be interrupted by hwrng_unregister() whil=
e
>>>    it's sleeping, because hwrng_unregister() is called from elsewhere.
>>>    The solution here is to keep track of which thread is currently
>>>    reading, and asleep, and signal that thread when it's time to
>>>    unregister. There's a bit of book keeping required to prevent
>>>    lifetime issues on current.
>>>
>>> Reported-by: Gregory Erwin <gregerwin256@gmail.com>
>>> Cc: Toke H=C3=B8iland-J=C3=B8rgensen <toke@redhat.com>
>>> Cc: Kalle Valo <kvalo@kernel.org>
>>> Cc: Rui Salvaterra <rsalvaterra@gmail.com>
>>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>>> Cc: stable@vger.kernel.org
>>> Fixes: fcd09c90c3c5 ("ath9k: use hw_random API instead of directly
>>> dumping into random.c")
>>> Link:
>>> https://lore.kernel.org/all/CAO+Okf6ZJC5-nTE_EJUGQtd8JiCkiEHytGgDsFGTEj=
s0c00giw@mail.gmail.com/
>>> Link:
>>> https://lore.kernel.org/lkml/CAO+Okf5k+C+SE6pMVfPf-d8MfVPVq4PO7EY8Hys_D=
VXtent3HA@mail.gmail.com/
>>> Link: https://bugs.archlinux.org/task/75138
>>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>> ---
>>> Changes v7->v8:
>>> - Add a missing export_symbol.
>>>
>>>  drivers/char/hw_random/core.c        | 30 ++++++++++++++++++++++++----
>>>  drivers/net/wireless/ath/ath9k/rng.c | 19 +++++++-----------
>>>  kernel/sched/core.c                  |  1 +
>>>  3 files changed, 34 insertions(+), 16 deletions(-)
>>
>> I don't see any acks for the hw_random and the scheduler change, adding
>> more
>> people to CC. Full patch here:
>>
>> https://patchwork.kernel.org/project/linux-wireless/patch/20220629114240=
.946411-1-Jason@zx2c4.com/
>>
>> Are everyone ok if I take this patch via wireless-next?
>>
>
> Thanks for the Cc.
>
> I'm not hot on the export of wake_up_state(), IMO any wakeup with
> !(state & TASK_NORMAL) should be reserved to kernel internals. Now, here
> IIUC the problem is that the patch uses an inline invoking
>
>   wake_up_state(p, TASK_INTERRUPTIBLE)
>
> so this isn't playing with any 'exotic' task state, thus it shouldn't
> actually need the export.
>
> I've been trying to figure out if this could work with just a
> wake_up_process(), but the sleeping pattern here is not very conforming
> (cf. 'wait loop' pattern in sched/core.c), AFAICT the signal is used to
> circumvent that :/

I don't intend to work on this patch more. If you'd like to ack the
trivial scheduler change (adding EXPORT_SYMBOL), that'd help, and then
this can move forward as planned. Otherwise, if you have particular
opinions about this patch that you want to happen, feel free to pick
up the patch and send your own revisions (though I don't intend to do
further review). Alternatively, I'll just send a patch to remove the
driver entirely. Hopefully you do find this ack-able, though.

Jason