[PATCH] KEYS: reaching the keys quotas correctly when instanttiating

[PATCH] KEYS: reaching the keys quotas correctly when instanttiating

[Yang Xu]

I wrote a regression test[1]to test commit a08bf91ce28ed
("KEYS: allow reaching the keys quotas exactly"), but it
failed even I used lastest kernel. It looks like the previous
patch doesn't remove "=" when we instantiate the key.

[1]http://lists.linux.it/pipermail/ltp/2020-February/015331.html

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 security/keys/key.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index 718bf7217420..e959b3c96b48 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -382,7 +382,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
 		spin_lock(&key->user->lock);
 
 		if (delta > 0 &&
-		    (key->user->qnbytes + delta >= maxbytes ||
+		    (key->user->qnbytes + delta > maxbytes ||
 		     key->user->qnbytes + delta < key->user->qnbytes)) {
 			ret = -EDQUOT;
 		}
-- 
2.18.0

Re: [PATCH] KEYS: reaching the keys quotas correctly when instanttiating

[Jarkko Sakkinen]

On Thu, Feb 27, 2020 at 06:25:45PM +0800, Yang Xu wrote:
> I wrote a regression test[1]to test commit a08bf91ce28ed
> ("KEYS: allow reaching the keys quotas exactly"), but it
> failed even I used lastest kernel. It looks like the previous
> patch doesn't remove "=" when we instantiate the key.
> 
> [1]http://lists.linux.it/pipermail/ltp/2020-February/015331.html
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>

1. Describe the failure and how this patch fixes in the commit
   message.
2. Add a fixes tag.

/Jarkko

[PATCH v2] KEYS: reaching the keys quotas(max_bytes) correctly

[Yang Xu]

Currently, when we add a new user key, the calltrace as below:

add_key()
  key_create_or_update()
    key_alloc()
    __key_instantiate_and_link
      generic_key_instantiate
        key_payload_reserve
          ......

Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
we can reach max bytes/keys in key_alloc, but we forget to remove this
limit when we reserver space for payload in key_payload_reserve. So we
can only reach max keys but not max bytes when having delta between plen
and type->def_datalen. Remove this limit when instantiating the key, so we
can keep consistent with key_alloc.

Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
Cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 security/keys/key.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index 718bf7217420..e959b3c96b48 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -382,7 +382,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
 		spin_lock(&key->user->lock);
 
 		if (delta > 0 &&
-		    (key->user->qnbytes + delta >= maxbytes ||
+		    (key->user->qnbytes + delta > maxbytes ||
 		     key->user->qnbytes + delta < key->user->qnbytes)) {
 			ret = -EDQUOT;
 		}
-- 
2.18.0

Re: [PATCH v2] KEYS: reaching the keys quotas(max_bytes) correctly

[Eric Biggers]

On Fri, Feb 28, 2020 at 10:32:57AM +0800, Yang Xu wrote:
> Currently, when we add a new user key, the calltrace as below:
> 
> add_key()
>   key_create_or_update()
>     key_alloc()
>     __key_instantiate_and_link
>       generic_key_instantiate
>         key_payload_reserve
>           ......
> 
> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> we can reach max bytes/keys in key_alloc, but we forget to remove this
> limit when we reserver space for payload in key_payload_reserve. So we
> can only reach max keys but not max bytes when having delta between plen
> and type->def_datalen. Remove this limit when instantiating the key, so we
> can keep consistent with key_alloc.
> 
> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>  security/keys/key.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 718bf7217420..e959b3c96b48 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -382,7 +382,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
>  		spin_lock(&key->user->lock);
>  
>  		if (delta > 0 &&
> -		    (key->user->qnbytes + delta >= maxbytes ||
> +		    (key->user->qnbytes + delta > maxbytes ||
>  		     key->user->qnbytes + delta < key->user->qnbytes)) {
>  			ret = -EDQUOT;
>  		}

This looks good, but I see that both of us forgot to update keyctl_chown_key().
Can you handle that too?

You could also use two Fixes tags:

    Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
    Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")

... to make it clearer that this is fixing an incomplete fix for the original
bug, as opposed to fixing a regression.

- Eric

[PATCH v3] KEYS: reaching the keys quotas correctly

[Yang Xu]

Currently, when we add a new user key, the calltrace as below:

add_key()
  key_create_or_update()
    key_alloc()
    __key_instantiate_and_link
      generic_key_instantiate
        key_payload_reserve
          ......

Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
we can reach max bytes/keys in key_alloc, but we forget to remove this
limit when we reserver space for payload in key_payload_reserve. So we
can only reach max keys but not max bytes when having delta between plen
and type->def_datalen. Remove this limit when instantiating the key, so we
can keep consistent with key_alloc.

Also, fix the similar problem in keyctl_chown_key().

Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
Cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 security/keys/key.c    | 2 +-
 security/keys/keyctl.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index 718bf7217420..e959b3c96b48 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -382,7 +382,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
 		spin_lock(&key->user->lock);
 
 		if (delta > 0 &&
-		    (key->user->qnbytes + delta >= maxbytes ||
+		    (key->user->qnbytes + delta > maxbytes ||
 		     key->user->qnbytes + delta < key->user->qnbytes)) {
 			ret = -EDQUOT;
 		}
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..d1a3dea58dee 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -937,8 +937,8 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 				key_quota_root_maxbytes : key_quota_maxbytes;
 
 			spin_lock(&newowner->lock);
-			if (newowner->qnkeys + 1 >= maxkeys ||
-			    newowner->qnbytes + key->quotalen >= maxbytes ||
+			if (newowner->qnkeys + 1 > maxkeys ||
+			    newowner->qnbytes + key->quotalen > maxbytes ||
 			    newowner->qnbytes + key->quotalen <
 			    newowner->qnbytes)
 				goto quota_overrun;
-- 
2.18.0

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[Jarkko Sakkinen]

On Fri, 2020-02-28 at 12:41 +0800, Yang Xu wrote:
> Currently, when we add a new user key, the calltrace as below:
> 
> add_key()
>   key_create_or_update()
>     key_alloc()
>     __key_instantiate_and_link
>       generic_key_instantiate
>         key_payload_reserve
>           ......
> 
> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> we can reach max bytes/keys in key_alloc, but we forget to remove this
> limit when we reserver space for payload in key_payload_reserve. So we
> can only reach max keys but not max bytes when having delta between plen
> and type->def_datalen. Remove this limit when instantiating the key, so we
> can keep consistent with key_alloc.
> 
> Also, fix the similar problem in keyctl_chown_key().
> 
> Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[Eric Biggers]

On Fri, Feb 28, 2020 at 12:41:51PM +0800, Yang Xu wrote:
>
> Subject: Re: [PATCH v3] KEYS: reaching the keys quotas correctly

The subject should be in imperative tense, like
"KEYS: reach the keys quotas correctly"

> Currently, when we add a new user key, the calltrace as below:
> 
> add_key()
>   key_create_or_update()
>     key_alloc()
>     __key_instantiate_and_link
>       generic_key_instantiate
>         key_payload_reserve
>           ......
> 
> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> we can reach max bytes/keys in key_alloc, but we forget to remove this
> limit when we reserver space for payload in key_payload_reserve. So we
> can only reach max keys but not max bytes when having delta between plen
> and type->def_datalen. Remove this limit when instantiating the key, so we
> can keep consistent with key_alloc.
> 
> Also, fix the similar problem in keyctl_chown_key().
> 
> Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>

Otherwise this looks fine.  Thanks!

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[Yang Xu]

on 2020/03/03 12:17, Eric Biggers wrote:
> On Fri, Feb 28, 2020 at 12:41:51PM +0800, Yang Xu wrote:
>>
>> Subject: Re: [PATCH v3] KEYS: reaching the keys quotas correctly
> 
> The subject should be in imperative tense, like
> "KEYS: reach the keys quotas correctly"
Sound reasonable, I think maintainer can change this when merged this patch.

Best Regards
Yang Xu
> 
>> Currently, when we add a new user key, the calltrace as below:
>>
>> add_key()
>>    key_create_or_update()
>>      key_alloc()
>>      __key_instantiate_and_link
>>        generic_key_instantiate
>>          key_payload_reserve
>>            ......
>>
>> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
>> we can reach max bytes/keys in key_alloc, but we forget to remove this
>> limit when we reserver space for payload in key_payload_reserve. So we
>> can only reach max keys but not max bytes when having delta between plen
>> and type->def_datalen. Remove this limit when instantiating the key, so we
>> can keep consistent with key_alloc.
>>
>> Also, fix the similar problem in keyctl_chown_key().
>>
>> Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
>> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
>> Cc: Eric Biggers <ebiggers@google.com>
>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> 
> Otherwise this looks fine.  Thanks!
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>
> 
> - Eric
> 
> 

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[Jarkko Sakkinen]

On Mon, Mar 02, 2020 at 08:17:32PM -0800, Eric Biggers wrote:
> On Fri, Feb 28, 2020 at 12:41:51PM +0800, Yang Xu wrote:
> >
> > Subject: Re: [PATCH v3] KEYS: reaching the keys quotas correctly
> 
> The subject should be in imperative tense, like
> "KEYS: reach the keys quotas correctly"

Preferably with a capital letter .

> 
> > Currently, when we add a new user key, the calltrace as below:
> > 
> > add_key()
> >   key_create_or_update()
> >     key_alloc()
> >     __key_instantiate_and_link
> >       generic_key_instantiate
> >         key_payload_reserve
> >           ......
> > 
> > Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> > we can reach max bytes/keys in key_alloc, but we forget to remove this
> > limit when we reserver space for payload in key_payload_reserve. So we
> > can only reach max keys but not max bytes when having delta between plen
> > and type->def_datalen. Remove this limit when instantiating the key, so we
> > can keep consistent with key_alloc.
> > 
> > Also, fix the similar problem in keyctl_chown_key().
> > 
> > Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
> > Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> > Cc: Eric Biggers <ebiggers@google.com>
> > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> 
> Otherwise this looks fine.  Thanks!
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>

David, should I pick this is up to my tree?

/Jarkko

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[David Howells]

How about:

	keys: Fix the keys quotas limit check

It's a bit unfortunate that "key" is also usable as an adjective.

David

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[Jarkko Sakkinen]

On Thu, Mar 19, 2020 at 03:08:21PM +0000, David Howells wrote:
> How about:
> 
> 	keys: Fix the keys quotas limit check
> 
> It's a bit unfortunate that "key" is also usable as an adjective.
> 
> David

Unfortunately it is already hanging here:

https://www.lkml.org/lkml/2020/3/15/314

/Jarkko

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[David Howells]

Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> Unfortunately it is already hanging here:
> 
> https://www.lkml.org/lkml/2020/3/15/314

Hanging? Or queued?

David

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[Jarkko Sakkinen]

On Thu, Mar 19, 2020 at 09:30:13PM +0000, David Howells wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > Unfortunately it is already hanging here:
> > 
> > https://www.lkml.org/lkml/2020/3/15/314
> 
> Hanging? Or queued?

Not yet queued.

Should I request to withdraw it? There is still time to do that.

/Jarkko

Re: [PATCH v3] KEYS: reaching the keys quotas correctly

[David Howells]

Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> > > Unfortunately it is already hanging here:
> > > 
> > > https://www.lkml.org/lkml/2020/3/15/314
> > 
> > Hanging? Or queued?
> 
> Not yet queued.
> 
> Should I request to withdraw it? There is still time to do that.

I was making up a series of fix patches to send to Linus.  If you want to send
your queue instead, you can stick my Acked-by on this patch.

David

Re: [PATCH v2] KEYS: reaching the keys quotas(max_bytes) correctly

[Yang Xu]

on 2020/02/28 11:30, Eric Biggers wrote:
> On Fri, Feb 28, 2020 at 10:32:57AM +0800, Yang Xu wrote:
>> Currently, when we add a new user key, the calltrace as below:
>>
>> add_key()
>>    key_create_or_update()
>>      key_alloc()
>>      __key_instantiate_and_link
>>        generic_key_instantiate
>>          key_payload_reserve
>>            ......
>>
>> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
>> we can reach max bytes/keys in key_alloc, but we forget to remove this
>> limit when we reserver space for payload in key_payload_reserve. So we
>> can only reach max keys but not max bytes when having delta between plen
>> and type->def_datalen. Remove this limit when instantiating the key, so we
>> can keep consistent with key_alloc.
>>
>> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
>> Cc: Eric Biggers <ebiggers@google.com>
>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
>> ---
>>   security/keys/key.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/keys/key.c b/security/keys/key.c
>> index 718bf7217420..e959b3c96b48 100644
>> --- a/security/keys/key.c
>> +++ b/security/keys/key.c
>> @@ -382,7 +382,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
>>   		spin_lock(&key->user->lock);
>>   
>>   		if (delta > 0 &&
>> -		    (key->user->qnbytes + delta >= maxbytes ||
>> +		    (key->user->qnbytes + delta > maxbytes ||
>>   		     key->user->qnbytes + delta < key->user->qnbytes)) {
>>   			ret = -EDQUOT;
>>   		}
> 
> This looks good, but I see that both of us forgot to update keyctl_chown_key().
> Can you handle that too?
> 
Of course. I will handle this together.
> You could also use two Fixes tags:
> 
>      Fixes: 0b77f5bfb45c ("keys: make the keyring quotas controllable through /proc/sys")
>      Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> 
> ... to make it clearer that this is fixing an incomplete fix for the original
> bug, as opposed to fixing a regression.
OK. This is more clearer.
Thanks for your comment.

Best Reagrds
Yang Xu
> 
> - Eric
> 
> 

Re: [PATCH v2] KEYS: reaching the keys quotas(max_bytes) correctly

[Jarkko Sakkinen]

On Fri, 2020-02-28 at 10:32 +0800, Yang Xu wrote:
> Currently, when we add a new user key, the calltrace as below:
> 
> add_key()
>   key_create_or_update()
>     key_alloc()
>     __key_instantiate_and_link
>       generic_key_instantiate
>         key_payload_reserve
>           ......
> 
> Since commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly"),
> we can reach max bytes/keys in key_alloc, but we forget to remove this
> limit when we reserver space for payload in key_payload_reserve. So we
> can only reach max keys but not max bytes when having delta between plen
> and type->def_datalen. Remove this limit when instantiating the key, so we
> can keep consistent with key_alloc.
> 
> Fixes: a08bf91ce28e ("KEYS: allow reaching the keys quotas exactly")
> Cc: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko