| Message ID | CAGypqWw951d=zYRbdgNR4snUDvJhWL=q3=WOyh7HhSJupjz2vA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KEYS: Do not cache key in task struct if key is requested from kernel thread | expand |
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
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
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
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
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
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.?
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")
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
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
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 }
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(-) -- 2.25.1