[PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Bharath SM]

The key which gets cached in task structure from a kernel thread does not
get invalidated even after expiry. Due to which, a new key request from
kernel thread will be served with the cached key if it's present in task
struct irrespective of the key validity.
The change is to not cache key in task_struct when key requested from kernel
thread so that kernel thread gets a valid key on every key request.

Signed-off-by: Bharath SM <bharathsm@microsoft.com>
---
 security/keys/request_key.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 2da4404276f0..07a0ef2baacd 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -38,9 +38,12 @@ static void cache_requested_key(struct key *key)
 #ifdef CONFIG_KEYS_REQUEST_CACHE
        struct task_struct *t = current;

-       key_put(t->cached_requested_key);
-       t->cached_requested_key = key_get(key);
-       set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
+       /* Do not cache key if it is a kernel thread */
+       if (!(t->flags & PF_KTHREAD)) {
+               key_put(t->cached_requested_key);
+               t->cached_requested_key = key_get(key);
+               set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
+       }
 #endif
 }

--
2.25.1

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Jarkko Sakkinen]

On Mon, 2023-03-13 at 00:23 +0530, Bharath SM wrote:
> The key which gets cached in task structure from a kernel thread does not
> get invalidated even after expiry. Due to which, a new key request from
> kernel thread will be served with the cached key if it's present in task
> struct irrespective of the key validity.
> The change is to not cache key in task_struct when key requested from kernel
> thread so that kernel thread gets a valid key on every key request.
> 
> Signed-off-by: Bharath SM <bharathsm@microsoft.com>

What is the context where you bumped into this?

> ---
>  security/keys/request_key.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 2da4404276f0..07a0ef2baacd 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -38,9 +38,12 @@ static void cache_requested_key(struct key *key)
>  #ifdef CONFIG_KEYS_REQUEST_CACHE
>         struct task_struct *t = current;
> 
> -       key_put(t->cached_requested_key);
> -       t->cached_requested_key = key_get(key);
> -       set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> +       /* Do not cache key if it is a kernel thread */
> +       if (!(t->flags & PF_KTHREAD)) {
> +               key_put(t->cached_requested_key);
> +               t->cached_requested_key = key_get(key);
> +               set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> +       }
>  #endif
>  }
> 
> --
> 2.25.1

BR, Jarkko

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Bharath SM]

Linux kernel cifs module uses dns_resolver for dns resolution and
dns_resolver will use kernel keys infrastructure for key management.
Cifs module calls dns_query during reconnect for dns resolution, we noticed
an issue with dns resolution requests during reconnect operations from cifs.
Where the dns_query was failing by returning EKEYEXPIRED to cifs. And
this issue was
happening only when CONFIG_KEYS_REQUEST_CACHE was enabled.
Further debugging the keys subsystem and discussing with david howells revealed
this issue in keys subsystem.

To reproduce the issue mount a few SMB shares on device with
nosharesock mount option and try disconnecting connections a few times
using "ss -K src dport 445".

Logs from dns_resolver:
Notice that 2nd time, we can see dns_query returning -127(EKEYEXPIRED)

Disconnected first time and got right response for dns_query:

[Mon Mar 13 05:05:23 2023] [cifsd ] ==>
dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
[Mon Mar 13 05:05:23 2023] [cifsd ] call
request_key(,storagesouthcus1.file.core.windows.net,)
[Mon Mar 13 05:05:23 2023] [cifsd ] ==>
dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
[Mon Mar 13 05:05:23 2023] [cifsd ] call
request_key(,storagesouthcus1.file.core.windows.net,)
[Mon Mar 13 05:05:23 2023] [cifsd ] ==>
dns_resolver_cmp(storagesouthcus1.file.core.windows.net,storagesouthcus1.file.core.windows.net)
[Mon Mar 13 05:05:23 2023] [cifsd ] <== dns_resolver_cmp() = 1
[Mon Mar 13 05:05:23 2023] [key.dn] ==> dns_resolver_preparse('
20.150.20.136',14)
[Mon Mar 13 05:05:23 2023] [key.dn] no options
[Mon Mar 13 05:05:23 2023] [key.dn] store result
[Mon Mar 13 05:05:23 2023] [key.dn] <== dns_resolver_preparse() = 0
[Mon Mar 13 05:05:23 2023] [cifsd ] <== dns_query() = 13
[Mon Mar 13 05:05:23 2023] [cifsd ] <== dns_query() = 13

Disconnected second time, but this time we can see one of the
dns_query request is failing with -127

[Mon Mar 13 05:05:30 2023] [cifsd ] ==>
dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
[Mon Mar 13 05:05:30 2023] [cifsd ] call
request_key(,storagesouthcus1.file.core.windows.net,)
[Mon Mar 13 05:05:30 2023] [cifsd ] ==>
dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
[Mon Mar 13 05:05:30 2023] [cifsd ] call
request_key(,storagesouthcus1.file.core.windows.net,)
[Mon Mar 13 05:05:30 2023] [cifsd ] ==>
dns_resolver_cmp(storagesouthcus1.file.core.windows.net,storagesouthcus1.file.core.windows.net)
[Mon Mar 13 05:05:30 2023] [cifsd ] <== dns_resolver_cmp() = 1
[Mon Mar 13 05:05:30 2023] [cifsd ] <== dns_query() = -127
[Mon Mar 13 05:05:30 2023] [key.dn] ==> dns_resolver_preparse('
20.150.20.136',14)
[Mon Mar 13 05:05:30 2023] [key.dn] no options
[Mon Mar 13 05:05:30 2023] [key.dn] store result
[Mon Mar 13 05:05:30 2023] [key.dn] <== dns_resolver_preparse() = 0
[Mon Mar 13 05:05:30 2023] [cifsd ] <== dns_query() = 13

On Mon, Mar 13, 2023 at 3:07 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Mon, 2023-03-13 at 00:23 +0530, Bharath SM wrote:
> > The key which gets cached in task structure from a kernel thread does not
> > get invalidated even after expiry. Due to which, a new key request from
> > kernel thread will be served with the cached key if it's present in task
> > struct irrespective of the key validity.
> > The change is to not cache key in task_struct when key requested from kernel
> > thread so that kernel thread gets a valid key on every key request.
> >
> > Signed-off-by: Bharath SM <bharathsm@microsoft.com>
>
> What is the context where you bumped into this?
>
> > ---
> >  security/keys/request_key.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> > index 2da4404276f0..07a0ef2baacd 100644
> > --- a/security/keys/request_key.c
> > +++ b/security/keys/request_key.c
> > @@ -38,9 +38,12 @@ static void cache_requested_key(struct key *key)
> >  #ifdef CONFIG_KEYS_REQUEST_CACHE
> >         struct task_struct *t = current;
> >
> > -       key_put(t->cached_requested_key);
> > -       t->cached_requested_key = key_get(key);
> > -       set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> > +       /* Do not cache key if it is a kernel thread */
> > +       if (!(t->flags & PF_KTHREAD)) {
> > +               key_put(t->cached_requested_key);
> > +               t->cached_requested_key = key_get(key);
> > +               set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> > +       }
> >  #endif
> >  }
> >
> > --
> > 2.25.1
>
> BR, Jarkko

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Jarkko Sakkinen]

On Mon, Mar 13, 2023 at 10:48:29AM +0530, Bharath SM wrote:
> Linux kernel cifs module uses dns_resolver for dns resolution and
> dns_resolver will use kernel keys infrastructure for key management.
> Cifs module calls dns_query during reconnect for dns resolution, we noticed
> an issue with dns resolution requests during reconnect operations from cifs.
> Where the dns_query was failing by returning EKEYEXPIRED to cifs. And
> this issue was
> happening only when CONFIG_KEYS_REQUEST_CACHE was enabled.
> Further debugging the keys subsystem and discussing with david howells revealed
> this issue in keys subsystem.
> 
> To reproduce the issue mount a few SMB shares on device with
> nosharesock mount option and try disconnecting connections a few times
> using "ss -K src dport 445".
> 
> Logs from dns_resolver:
> Notice that 2nd time, we can see dns_query returning -127(EKEYEXPIRED)
> 
> Disconnected first time and got right response for dns_query:
> 
> [Mon Mar 13 05:05:23 2023] [cifsd ] ==>
> dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
> [Mon Mar 13 05:05:23 2023] [cifsd ] call
> request_key(,storagesouthcus1.file.core.windows.net,)
> [Mon Mar 13 05:05:23 2023] [cifsd ] ==>
> dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
> [Mon Mar 13 05:05:23 2023] [cifsd ] call
> request_key(,storagesouthcus1.file.core.windows.net,)
> [Mon Mar 13 05:05:23 2023] [cifsd ] ==>
> dns_resolver_cmp(storagesouthcus1.file.core.windows.net,storagesouthcus1.file.core.windows.net)
> [Mon Mar 13 05:05:23 2023] [cifsd ] <== dns_resolver_cmp() = 1
> [Mon Mar 13 05:05:23 2023] [key.dn] ==> dns_resolver_preparse('
> 20.150.20.136',14)
> [Mon Mar 13 05:05:23 2023] [key.dn] no options
> [Mon Mar 13 05:05:23 2023] [key.dn] store result
> [Mon Mar 13 05:05:23 2023] [key.dn] <== dns_resolver_preparse() = 0
> [Mon Mar 13 05:05:23 2023] [cifsd ] <== dns_query() = 13
> [Mon Mar 13 05:05:23 2023] [cifsd ] <== dns_query() = 13
> 
> Disconnected second time, but this time we can see one of the
> dns_query request is failing with -127
> 
> [Mon Mar 13 05:05:30 2023] [cifsd ] ==>
> dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
> [Mon Mar 13 05:05:30 2023] [cifsd ] call
> request_key(,storagesouthcus1.file.core.windows.net,)
> [Mon Mar 13 05:05:30 2023] [cifsd ] ==>
> dns_query((null),storagesouthcus1.file.core.windows.net,38,(null))
> [Mon Mar 13 05:05:30 2023] [cifsd ] call
> request_key(,storagesouthcus1.file.core.windows.net,)
> [Mon Mar 13 05:05:30 2023] [cifsd ] ==>
> dns_resolver_cmp(storagesouthcus1.file.core.windows.net,storagesouthcus1.file.core.windows.net)
> [Mon Mar 13 05:05:30 2023] [cifsd ] <== dns_resolver_cmp() = 1
> [Mon Mar 13 05:05:30 2023] [cifsd ] <== dns_query() = -127
> [Mon Mar 13 05:05:30 2023] [key.dn] ==> dns_resolver_preparse('
> 20.150.20.136',14)
> [Mon Mar 13 05:05:30 2023] [key.dn] no options
> [Mon Mar 13 05:05:30 2023] [key.dn] store result
> [Mon Mar 13 05:05:30 2023] [key.dn] <== dns_resolver_preparse() = 0
> [Mon Mar 13 05:05:30 2023] [cifsd ] <== dns_query() = 13

Please summarize this to the commit message it is useful stuff. With
this report included the patch could should also have a fixes tag.

BR, Jarkko

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[David Howells]

Bharath SM <bharathsm.hsk@gmail.com> wrote:

> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 2da4404276f0..07a0ef2baacd 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -38,9 +38,12 @@ static void cache_requested_key(struct key *key)
>  #ifdef CONFIG_KEYS_REQUEST_CACHE
>         struct task_struct *t = current;
> 
> -       key_put(t->cached_requested_key);
> -       t->cached_requested_key = key_get(key);
> -       set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> +       /* Do not cache key if it is a kernel thread */
> +       if (!(t->flags & PF_KTHREAD)) {
> +               key_put(t->cached_requested_key);
> +               t->cached_requested_key = key_get(key);
> +               set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> +       }
>  #endif
>  }

It seems all the tabs got converted into spaces.  I'll manually apply it.

David

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[David Howells]

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> Please summarize this to the commit message it is useful stuff. With
> this report included the patch could should also have a fixes tag.

I've expanded the commit message to:

    keys: Do not cache key in task struct if key is requested from kernel thread
    
    The key which gets cached in task structure from a kernel thread does not
    get invalidated even after expiry.  Due to which, a new key request from
    kernel thread will be served with the cached key if it's present in task
    struct irrespective of the key validity.  The change is to not cache key in
    task_struct when key requested from kernel thread so that kernel thread
    gets a valid key on every key request.
    
    The problem has been seen with the cifs module doing DNS lookups from a
    kernel thread and the results getting pinned by being attached to that
    kernel thread's cache - and thus not something that can be easily got rid
    of.  The cache would ordinarily be cleared by notify-resume, but kernel
    threads don't do that.
    
    This isn't seen with AFS because AFS is doing request_key() within the
    kernel half of a user thread - which will do notify-resume.
    
    Signed-off-by: Bharath SM <bharathsm@microsoft.com>
    Signed-off-by: David Howells <dhowells@redhat.com>
    cc: Jarkko Sakkinen <jarkko@kernel.org>
    cc: Shyam Prasad N <nspmangalore@gmail.com>
    cc: Steve French <smfrench@gmail.com>
    cc: keyrings@vger.kernel.org
    cc: linux-cifs@vger.kernel.org
    cc: linux-fsdevel@vger.kernel.org

David

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Bharath SM]

On Tue, Mar 14, 2023 at 8:58 PM David Howells <dhowells@redhat.com> wrote:
>
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> > Please summarize this to the commit message it is useful stuff. With
> > this report included the patch could should also have a fixes tag.
>
> I've expanded the commit message to:
>
>     keys: Do not cache key in task struct if key is requested from kernel thread
>
>     The key which gets cached in task structure from a kernel thread does not
>     get invalidated even after expiry.  Due to which, a new key request from
>     kernel thread will be served with the cached key if it's present in task
>     struct irrespective of the key validity.  The change is to not cache key in
>     task_struct when key requested from kernel thread so that kernel thread
>     gets a valid key on every key request.
>
>     The problem has been seen with the cifs module doing DNS lookups from a
>     kernel thread and the results getting pinned by being attached to that
>     kernel thread's cache - and thus not something that can be easily got rid
>     of.  The cache would ordinarily be cleared by notify-resume, but kernel
>     threads don't do that.
>
>     This isn't seen with AFS because AFS is doing request_key() within the
>     kernel half of a user thread - which will do notify-resume.
>
>     Signed-off-by: Bharath SM <bharathsm@microsoft.com>
>     Signed-off-by: David Howells <dhowells@redhat.com>
>     cc: Jarkko Sakkinen <jarkko@kernel.org>
>     cc: Shyam Prasad N <nspmangalore@gmail.com>
>     cc: Steve French <smfrench@gmail.com>
>     cc: keyrings@vger.kernel.org
>     cc: linux-cifs@vger.kernel.org
>     cc: linux-fsdevel@vger.kernel.org
>
> David

Your expanded commit message looks fine. Can we please mark this
commit for a stable kernel since it fixes some serious reconnect
problem.?

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Bharath SM]

On Wed, Mar 15, 2023 at 8:43 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
>
> On Tue, Mar 14, 2023 at 8:58 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > > Please summarize this to the commit message it is useful stuff. With
> > > this report included the patch could should also have a fixes tag.
> >
> > I've expanded the commit message to:
> >
> >     keys: Do not cache key in task struct if key is requested from kernel thread
> >
> >     The key which gets cached in task structure from a kernel thread does not
> >     get invalidated even after expiry.  Due to which, a new key request from
> >     kernel thread will be served with the cached key if it's present in task
> >     struct irrespective of the key validity.  The change is to not cache key in
> >     task_struct when key requested from kernel thread so that kernel thread
> >     gets a valid key on every key request.
> >
> >     The problem has been seen with the cifs module doing DNS lookups from a
> >     kernel thread and the results getting pinned by being attached to that
> >     kernel thread's cache - and thus not something that can be easily got rid
> >     of.  The cache would ordinarily be cleared by notify-resume, but kernel
> >     threads don't do that.
> >
> >     This isn't seen with AFS because AFS is doing request_key() within the
> >     kernel half of a user thread - which will do notify-resume.
> >
> >     Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> >     Signed-off-by: David Howells <dhowells@redhat.com>
> >     cc: Jarkko Sakkinen <jarkko@kernel.org>
> >     cc: Shyam Prasad N <nspmangalore@gmail.com>
> >     cc: Steve French <smfrench@gmail.com>
> >     cc: keyrings@vger.kernel.org
> >     cc: linux-cifs@vger.kernel.org
> >     cc: linux-fsdevel@vger.kernel.org
> >
> > David
>
> Your expanded commit message looks fine. Can we please mark this
> commit for a stable kernel since it fixes some serious reconnect
> problem.?

Adding Fixes tag:
Fixes: 7743c48e54ee ("keys: Cache result of request_key*() temporarily
in task_struct")

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Jarkko Sakkinen]

On Tue, Mar 14, 2023 at 03:27:32PM +0000, David Howells wrote:
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> > Please summarize this to the commit message it is useful stuff. With
> > this report included the patch could should also have a fixes tag.
> 
> I've expanded the commit message to:
> 
>     keys: Do not cache key in task struct if key is requested from kernel thread
>     
>     The key which gets cached in task structure from a kernel thread does not
>     get invalidated even after expiry.  Due to which, a new key request from
>     kernel thread will be served with the cached key if it's present in task
>     struct irrespective of the key validity.  The change is to not cache key in
>     task_struct when key requested from kernel thread so that kernel thread
>     gets a valid key on every key request.
>     
>     The problem has been seen with the cifs module doing DNS lookups from a
>     kernel thread and the results getting pinned by being attached to that
>     kernel thread's cache - and thus not something that can be easily got rid
>     of.  The cache would ordinarily be cleared by notify-resume, but kernel
>     threads don't do that.
>     
>     This isn't seen with AFS because AFS is doing request_key() within the
>     kernel half of a user thread - which will do notify-resume.
>     
>     Signed-off-by: Bharath SM <bharathsm@microsoft.com>
>     Signed-off-by: David Howells <dhowells@redhat.com>
>     cc: Jarkko Sakkinen <jarkko@kernel.org>
>     cc: Shyam Prasad N <nspmangalore@gmail.com>
>     cc: Steve French <smfrench@gmail.com>
>     cc: keyrings@vger.kernel.org
>     cc: linux-cifs@vger.kernel.org
>     cc: linux-fsdevel@vger.kernel.org
> 
> David

Looks good to me! Can you send a version with this?

BR, Jarkko

Re: [PATCH] KEYS: Do not cache key in task struct if key is requested from kernel thread

[Jarkko Sakkinen]

On Sun, Mar 19, 2023 at 03:39:39PM +0200, Jarkko Sakkinen wrote:
> On Tue, Mar 14, 2023 at 03:27:32PM +0000, David Howells wrote:
> > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > > Please summarize this to the commit message it is useful stuff. With
> > > this report included the patch could should also have a fixes tag.
> > 
> > I've expanded the commit message to:
> > 
> >     keys: Do not cache key in task struct if key is requested from kernel thread
> >     
> >     The key which gets cached in task structure from a kernel thread does not
> >     get invalidated even after expiry.  Due to which, a new key request from
> >     kernel thread will be served with the cached key if it's present in task
> >     struct irrespective of the key validity.  The change is to not cache key in
> >     task_struct when key requested from kernel thread so that kernel thread
> >     gets a valid key on every key request.
> >     
> >     The problem has been seen with the cifs module doing DNS lookups from a
> >     kernel thread and the results getting pinned by being attached to that
> >     kernel thread's cache - and thus not something that can be easily got rid
> >     of.  The cache would ordinarily be cleared by notify-resume, but kernel
> >     threads don't do that.
> >     
> >     This isn't seen with AFS because AFS is doing request_key() within the
> >     kernel half of a user thread - which will do notify-resume.
> >     
> >     Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> >     Signed-off-by: David Howells <dhowells@redhat.com>
> >     cc: Jarkko Sakkinen <jarkko@kernel.org>
> >     cc: Shyam Prasad N <nspmangalore@gmail.com>
> >     cc: Steve French <smfrench@gmail.com>
> >     cc: keyrings@vger.kernel.org
> >     cc: linux-cifs@vger.kernel.org
> >     cc: linux-fsdevel@vger.kernel.org
> > 
> > David
> 
> Looks good to me! Can you send a version with this?

Oops, not from original sender. If you apply with this, please add

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko