[GIT PULL] keys: Miscellaneous fixes/changes

[GIT PULL] keys: Miscellaneous fixes/changes

[David Howells]

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(-)

Re: [GIT PULL] keys: Miscellaneous fixes/changes

[Linus Torvalds]

On Tue, Mar 21, 2023 at 9:43 AM David Howells <dhowells@redhat.com> 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

Re: [GIT PULL] keys: Miscellaneous fixes/changes

[pr-tracker-bot]

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

Re: [GIT PULL] keys: Miscellaneous fixes/changes

[Jens Axboe]

On 3/21/23 12:48?PM, Linus Torvalds wrote:
> On Tue, Mar 21, 2023 at 9:43?AM David Howells <dhowells@redhat.com> 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

Re: [GIT PULL] keys: Miscellaneous fixes/changes

[Linus Torvalds]

On Tue, Mar 21, 2023 at 12:16 PM Jens Axboe <axboe@kernel.dk> 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

Re: [GIT PULL] keys: Miscellaneous fixes/changes

[Jens Axboe]

On 3/21/23 1:21?PM, Linus Torvalds wrote:
> On Tue, Mar 21, 2023 at 12:16?PM Jens Axboe <axboe@kernel.dk> 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