Hi Linus,
Could you pull these fixes/changes for keyrings?
(1) Fix request_key() so that it doesn't cache a looked up key on the
current thread if that thread is a kernel thread. The cache is
cleared during notify_resume - but that doesn't happen in kernel
threads. This is causing cifs DNS keys to be un-invalidateable.
(2) Fix a wrapper check in verify_pefile() to not round up the length.
(3) Change asymmetric_keys code to log errors to make it easier for users
to work out why failures occurred.
Thanks,
David
---
The following changes since commit fc89d7fb499b0162e081f434d45e8d1b47e82ece:
Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost (2023-03-13 10:43:09 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-fixes-20230321
for you to fetch changes up to 3584c1dbfffdabf8e3dc1dd25748bb38dd01cd43:
asymmetric_keys: log on fatal failures in PE/pkcs7 (2023-03-21 16:23:56 +0000)
----------------------------------------------------------------
keyrings fixes
----------------------------------------------------------------
David Howells (1):
keys: Do not cache key in task struct if key is requested from kernel thread
Robbie Harwood (2):
verify_pefile: relax wrapper length check
asymmetric_keys: log on fatal failures in PE/pkcs7
crypto/asymmetric_keys/pkcs7_verify.c | 10 +++++-----
crypto/asymmetric_keys/verify_pefile.c | 32 ++++++++++++++++++--------------
security/keys/request_key.c | 9 ++++++---
3 files changed, 29 insertions(+), 22 deletions(-)
On Tue, Mar 21, 2023 at 9:43 AM David Howells <[email protected]> wrote:
>
> (1) Fix request_key() so that it doesn't cache a looked up key on the
> current thread if that thread is a kernel thread. The cache is
> cleared during notify_resume - but that doesn't happen in kernel
> threads. This is causing cifs DNS keys to be un-invalidateable.
I've pulled this, but I'd like people to look a bit more at this.
The issue with TIF_NOTIFY_RESUME is that it is only done on return to
user space.
And these days, PF_KTHREAD isn't the only case that never returns to
user space. PF_IO_WORKER has the exact same behaviour.
Now, to counteract this, as of this merge window (and marked for
stable) IO threads do a fake "return to user mode" handling in
io_run_task_work(), and so I think we're all good, but I'd like people
to at least think about this.
Linus
The pull request you sent on Tue, 21 Mar 2023 16:43:49 +0000:
> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-fixes-20230321
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/2faac9a98f010cf5b342fa89ac489c4586364e6e
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On 3/21/23 12:48?PM, Linus Torvalds wrote:
> On Tue, Mar 21, 2023 at 9:43?AM David Howells <[email protected]> wrote:
>>
>> (1) Fix request_key() so that it doesn't cache a looked up key on the
>> current thread if that thread is a kernel thread. The cache is
>> cleared during notify_resume - but that doesn't happen in kernel
>> threads. This is causing cifs DNS keys to be un-invalidateable.
>
> I've pulled this, but I'd like people to look a bit more at this.
>
> The issue with TIF_NOTIFY_RESUME is that it is only done on return to
> user space.
>
> And these days, PF_KTHREAD isn't the only case that never returns to
> user space. PF_IO_WORKER has the exact same behaviour.
>
> Now, to counteract this, as of this merge window (and marked for
> stable) IO threads do a fake "return to user mode" handling in
> io_run_task_work(), and so I think we're all good, but I'd like people
> to at least think about this.
I haven't seen the patch yet as it hasn't been pushed, but can imagine
what it looks like. It may make sense to add some debug check for
PF_KTHREAD having TIF_NOTIFY_RESUME set, or task_work pending for that
matter, as that is generally not workable without doing something to
handle it explicitly.
For PF_IO_WORKER, with the commit you mentioned, those threads should
deal with TIF_NOTIFY_RESUME just fine. Until something else gets added
that is also run from exit_to_user_mode...
--
Jens Axboe
On Tue, Mar 21, 2023 at 12:16 PM Jens Axboe <[email protected]> wrote:
>
> I haven't seen the patch yet as it hasn't been pushed,
Well, it went out a couple of minutes before your email, so it's out now.
> It may make sense to add some debug check for
> PF_KTHREAD having TIF_NOTIFY_RESUME set, or task_work pending for that
> matter, as that is generally not workable without doing something to
> handle it explicitly.
Yeah, I guess we could have some generic check for that. I'm not sure
where it would be. Scheduler?
Linus
On 3/21/23 1:21?PM, Linus Torvalds wrote:
> On Tue, Mar 21, 2023 at 12:16?PM Jens Axboe <[email protected]> wrote:
>>
>> I haven't seen the patch yet as it hasn't been pushed,
>
> Well, it went out a couple of minutes before your email, so it's out now.
Yep I see it now, looks as expected.
>> It may make sense to add some debug check for
>> PF_KTHREAD having TIF_NOTIFY_RESUME set, or task_work pending for that
>> matter, as that is generally not workable without doing something to
>> handle it explicitly.
>
> Yeah, I guess we could have some generic check for that. I'm not sure
> where it would be. Scheduler?
Off the top of my head, two options, both in kernel/sched/core.c:
1) Add it to schedule_debug()
2) Add it to sched_submit_work(), adding PF_KTHREAD to the flags checked
for PF_IO_WORKER | PF_WQ_WORKER to avoid adding any extra fast-path
overhead.
Alternatively, I guess it could go in kthread_exit() as well. But for
workloads with a persistent kthread that doesn't really go away, that
won't catch it.
--
Jens Axboe