| Message ID | 20240712072423.1942417-1-cuigaosheng1@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next,v2] gss_krb5: refactor code to return correct PTR_ERR in krb5_DK | expand |
On 2024-07-12 at 12:54:23, Gaosheng Cui (cuigaosheng1@huawei.com) wrote: > Refactor the code in krb5_DK to return PTR_ERR when an error occurs. > nit: Please change "-next" to "net-next" in subject Reviewed-by: Hariprasad Kelam <hkelam@marvell.com> > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > --- > v2: Update IS_ERR to PTR_ERR, thanks very much! > net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c > index 4eb19c3a54c7..5ac8d06ab2c0 100644 > --- a/net/sunrpc/auth_gss/gss_krb5_keys.c > +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c > @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, > goto err_return; > > cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); > - if (IS_ERR(cipher)) > + if (IS_ERR(cipher)) { > + ret = PTR_ERR(cipher); > goto err_return; > + } > + > blocksize = crypto_sync_skcipher_blocksize(cipher); > - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) > + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); > + if (ret) > goto err_free_cipher; > > ret = -ENOMEM; > -- > 2.25.1 > >
On Fri, 2024-07-12 at 15:24 +0800, Gaosheng Cui wrote: > Refactor the code in krb5_DK to return PTR_ERR when an error occurs. > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > --- > v2: Update IS_ERR to PTR_ERR, thanks very much! > net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c > index 4eb19c3a54c7..5ac8d06ab2c0 100644 > --- a/net/sunrpc/auth_gss/gss_krb5_keys.c > +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c > @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, > goto err_return; > > cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); > - if (IS_ERR(cipher)) > + if (IS_ERR(cipher)) { > + ret = PTR_ERR(cipher); > goto err_return; > + } > + > blocksize = crypto_sync_skcipher_blocksize(cipher); > - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) > + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); > + if (ret) > goto err_free_cipher; > > ret = -ENOMEM; Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote: > Refactor the code in krb5_DK to return PTR_ERR when an error occurs. My understanding of the current code is that if either crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize() fails, then krb5_DK() returns -EINVAL. At the only call site for krb5_DK(), that return code is unconditionally discarded. Thus I don't see that the proposed change is necessary or improves anything. > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > --- > v2: Update IS_ERR to PTR_ERR, thanks very much! > net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c > index 4eb19c3a54c7..5ac8d06ab2c0 100644 > --- a/net/sunrpc/auth_gss/gss_krb5_keys.c > +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c > @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, > goto err_return; > > cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); > - if (IS_ERR(cipher)) > + if (IS_ERR(cipher)) { > + ret = PTR_ERR(cipher); > goto err_return; > + } > + > blocksize = crypto_sync_skcipher_blocksize(cipher); > - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) > + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); > + if (ret) > goto err_free_cipher; > > ret = -ENOMEM; > -- > 2.25.1 >
On Fri, Jul 12, 2024 at 09:39:08AM -0400, Chuck Lever wrote: > On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote: > > Refactor the code in krb5_DK to return PTR_ERR when an error occurs. > > My understanding of the current code is that if either > crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize() > fails, then krb5_DK() returns -EINVAL. At the only call site for > krb5_DK(), that return code is unconditionally discarded. Thus I > don't see that the proposed change is necessary or improves > anything. My understanding is wrong ;-) The return code isn't discarded. A non-zero return code from krb5_DK() is carried back up the call stack. The logic in krb5_derive_key_v2() does not use the kernel's usual error flow form, so I missed this. However, it still isn't clear to me why the error behavior here needs to change. It's possible, for example, that -EINVAL is perfectly adequate to indicate when sync_skcipher() can't find the specified encryption algorithm (gk5e->encrypt_name). Specifying the wrong encryption type: -EINVAL. That makes sense. > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > > --- > > v2: Update IS_ERR to PTR_ERR, thanks very much! > > net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c > > index 4eb19c3a54c7..5ac8d06ab2c0 100644 > > --- a/net/sunrpc/auth_gss/gss_krb5_keys.c > > +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c > > @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, > > goto err_return; > > > > cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); > > - if (IS_ERR(cipher)) > > + if (IS_ERR(cipher)) { > > + ret = PTR_ERR(cipher); > > goto err_return; > > + } > > + > > blocksize = crypto_sync_skcipher_blocksize(cipher); > > - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) > > + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); > > + if (ret) > > goto err_free_cipher; > > > > ret = -ENOMEM; > > -- > > 2.25.1 > > > > -- > Chuck Lever >
Thanks for reviewing this patch. I think this modification makes sense, for example, if crypto_sync_skcipher_setkey return -ENOMEM, it's better to return -ENOMEM than to return -EINVAL, just like elsewhere. On 2024/7/12 22:28, Chuck Lever wrote: > On Fri, Jul 12, 2024 at 09:39:08AM -0400, Chuck Lever wrote: >> On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote: >>> Refactor the code in krb5_DK to return PTR_ERR when an error occurs. >> My understanding of the current code is that if either >> crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize() >> fails, then krb5_DK() returns -EINVAL. At the only call site for >> krb5_DK(), that return code is unconditionally discarded. Thus I >> don't see that the proposed change is necessary or improves >> anything. > My understanding is wrong ;-) > > The return code isn't discarded. A non-zero return code from > krb5_DK() is carried back up the call stack. The logic in > krb5_derive_key_v2() does not use the kernel's usual error flow > form, so I missed this. > > However, it still isn't clear to me why the error behavior here > needs to change. It's possible, for example, that -EINVAL is > perfectly adequate to indicate when sync_skcipher() can't find the > specified encryption algorithm (gk5e->encrypt_name). > > Specifying the wrong encryption type: -EINVAL. That makes sense. > > >>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >>> --- >>> v2: Update IS_ERR to PTR_ERR, thanks very much! >>> net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c >>> index 4eb19c3a54c7..5ac8d06ab2c0 100644 >>> --- a/net/sunrpc/auth_gss/gss_krb5_keys.c >>> +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c >>> @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, >>> goto err_return; >>> >>> cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); >>> - if (IS_ERR(cipher)) >>> + if (IS_ERR(cipher)) { >>> + ret = PTR_ERR(cipher); >>> goto err_return; >>> + } >>> + >>> blocksize = crypto_sync_skcipher_blocksize(cipher); >>> - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) >>> + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); >>> + if (ret) >>> goto err_free_cipher; >>> >>> ret = -ENOMEM; >>> -- >>> 2.25.1 >>> >> -- >> Chuck Lever >>
On Sat, 13 Jul 2024, Chuck Lever wrote: > On Fri, Jul 12, 2024 at 09:39:08AM -0400, Chuck Lever wrote: > > On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote: > > > Refactor the code in krb5_DK to return PTR_ERR when an error occurs. > > > > My understanding of the current code is that if either > > crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize() > > fails, then krb5_DK() returns -EINVAL. At the only call site for > > krb5_DK(), that return code is unconditionally discarded. Thus I > > don't see that the proposed change is necessary or improves > > anything. > > My understanding is wrong ;-) True, but I think your conclusion was correct. krb5_DK() returns zero or -EINVAL. It is only used by krb5_derive_key_v2(), which returns zero or -EINVAL, or -ENOMEM. krb4_derive_key_v2() is only used as a ->derive_key() method. This is called from krb5_derive_key(), and various unit tests in gss_krb5_tests.c krb5_derive_key() is only called in gss_krb5_mech.c, and each call site is of the form: if (krb5_derive_key(...)) goto out; so it doesn't matter what error is returned. The unit test calls are all followed by KUNIT_ASSERT_EQ(test, err, 0); so the only place the err is used is (presumably) in failure reports from the unit tests. So the proposed change seems unnecessary from a practical perspective. Maybe it is justified from an aesthetic perspective, but I think that should be clearly stated in the commit message. e.g. This change has no practical effect as all non-zero error statuses are treated equally, however the distinction between EINVAL and ENOMEM may be relevant at some future time and it seems cleaner to maintain the distinction. NeilBrown > > The return code isn't discarded. A non-zero return code from > krb5_DK() is carried back up the call stack. The logic in > krb5_derive_key_v2() does not use the kernel's usual error flow > form, so I missed this. > > However, it still isn't clear to me why the error behavior here > needs to change. It's possible, for example, that -EINVAL is > perfectly adequate to indicate when sync_skcipher() can't find the > specified encryption algorithm (gk5e->encrypt_name). > > Specifying the wrong encryption type: -EINVAL. That makes sense. > > > > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > > > --- > > > v2: Update IS_ERR to PTR_ERR, thanks very much! > > > net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c > > > index 4eb19c3a54c7..5ac8d06ab2c0 100644 > > > --- a/net/sunrpc/auth_gss/gss_krb5_keys.c > > > +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c > > > @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, > > > goto err_return; > > > > > > cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); > > > - if (IS_ERR(cipher)) > > > + if (IS_ERR(cipher)) { > > > + ret = PTR_ERR(cipher); > > > goto err_return; > > > + } > > > + > > > blocksize = crypto_sync_skcipher_blocksize(cipher); > > > - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) > > > + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); > > > + if (ret) > > > goto err_free_cipher; > > > > > > ret = -ENOMEM; > > > -- > > > 2.25.1 > > > > > > > -- > > Chuck Lever > > > > -- > Chuck Lever >
> On Jul 14, 2024, at 12:31 AM, NeilBrown <neilb@suse.de> wrote: > > On Sat, 13 Jul 2024, Chuck Lever wrote: >> On Fri, Jul 12, 2024 at 09:39:08AM -0400, Chuck Lever wrote: >>> On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote: >>>> Refactor the code in krb5_DK to return PTR_ERR when an error occurs. >>> >>> My understanding of the current code is that if either >>> crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize() >>> fails, then krb5_DK() returns -EINVAL. At the only call site for >>> krb5_DK(), that return code is unconditionally discarded. Thus I >>> don't see that the proposed change is necessary or improves >>> anything. >> >> My understanding is wrong ;-) > > True, but I think your conclusion was correct. > > krb5_DK() returns zero or -EINVAL. > It is only used by krb5_derive_key_v2(), which returns zero or -EINVAL, > or -ENOMEM. These are really the only three interesting return codes. Leaking other error codes to callers is not desirable, IMO. But looking at the current implementation of crypto_alloc_sync_skcipher(), it returns either ERR_PTR(-EINVAL) or a valid pointer; it doesn't return any other error value. Since it never returns -ENOMEM, there still doesn't seem to be a technical reason for modifying krb5_DK() to pass errors through. > krb4_derive_key_v2() is only used as a ->derive_key() method. > This is called from krb5_derive_key(), and various unit tests in > gss_krb5_tests.c > > krb5_derive_key() is only called in gss_krb5_mech.c, and each call site > is of the form: > if (krb5_derive_key(...)) goto out; > so it doesn't matter what error is returned. > > The unit test calls are all followed by > KUNIT_ASSERT_EQ(test, err, 0); > so the only place the err is used is (presumably) in failure reports > from the unit tests. > > So the proposed change seems unnecessary from a practical perspective. > > Maybe it is justified from an aesthetic perspective, but I think that > should be clearly stated in the commit message. e.g. > > This change has no practical effect as all non-zero error statuses > are treated equally, however the distinction between EINVAL and ENOMEM > may be relevant at some future time and it seems cleaner to maintain > the distinction. > > NeilBrown > > >> >> The return code isn't discarded. A non-zero return code from >> krb5_DK() is carried back up the call stack. The logic in >> krb5_derive_key_v2() does not use the kernel's usual error flow >> form, so I missed this. >> >> However, it still isn't clear to me why the error behavior here >> needs to change. It's possible, for example, that -EINVAL is >> perfectly adequate to indicate when sync_skcipher() can't find the >> specified encryption algorithm (gk5e->encrypt_name). >> >> Specifying the wrong encryption type: -EINVAL. That makes sense. >> >> >>>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >>>> --- >>>> v2: Update IS_ERR to PTR_ERR, thanks very much! >>>> net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c >>>> index 4eb19c3a54c7..5ac8d06ab2c0 100644 >>>> --- a/net/sunrpc/auth_gss/gss_krb5_keys.c >>>> +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c >>>> @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, >>>> goto err_return; >>>> >>>> cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); >>>> - if (IS_ERR(cipher)) >>>> + if (IS_ERR(cipher)) { >>>> + ret = PTR_ERR(cipher); >>>> goto err_return; >>>> + } >>>> + >>>> blocksize = crypto_sync_skcipher_blocksize(cipher); >>>> - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) >>>> + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); >>>> + if (ret) >>>> goto err_free_cipher; >>>> >>>> ret = -ENOMEM; >>>> -- >>>> 2.25.1 >>>> >>> >>> -- >>> Chuck Lever >>> >> >> -- >> Chuck Lever >> > -- Chuck Lever
But crypto_sync_skcipher_setkey maybe return -ENOMEM, Should this be modified? thanks! On 2024/7/15 0:18, Chuck Lever III wrote: > >> On Jul 14, 2024, at 12:31 AM, NeilBrown <neilb@suse.de> wrote: >> >> On Sat, 13 Jul 2024, Chuck Lever wrote: >>> On Fri, Jul 12, 2024 at 09:39:08AM -0400, Chuck Lever wrote: >>>> On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote: >>>>> Refactor the code in krb5_DK to return PTR_ERR when an error occurs. >>>> My understanding of the current code is that if either >>>> crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize() >>>> fails, then krb5_DK() returns -EINVAL. At the only call site for >>>> krb5_DK(), that return code is unconditionally discarded. Thus I >>>> don't see that the proposed change is necessary or improves >>>> anything. >>> My understanding is wrong ;-) >> True, but I think your conclusion was correct. >> >> krb5_DK() returns zero or -EINVAL. >> It is only used by krb5_derive_key_v2(), which returns zero or -EINVAL, >> or -ENOMEM. > These are really the only three interesting return codes. > Leaking other error codes to callers is not desirable, IMO. > > But looking at the current implementation of > crypto_alloc_sync_skcipher(), it returns either > ERR_PTR(-EINVAL) or a valid pointer; it doesn't return any > other error value. Since it never returns -ENOMEM, there > still doesn't seem to be a technical reason for modifying > krb5_DK() to pass errors through. > > >> krb4_derive_key_v2() is only used as a ->derive_key() method. >> This is called from krb5_derive_key(), and various unit tests in >> gss_krb5_tests.c >> >> krb5_derive_key() is only called in gss_krb5_mech.c, and each call site >> is of the form: >> if (krb5_derive_key(...)) goto out; >> so it doesn't matter what error is returned. >> >> The unit test calls are all followed by >> KUNIT_ASSERT_EQ(test, err, 0); >> so the only place the err is used is (presumably) in failure reports >> from the unit tests. >> >> So the proposed change seems unnecessary from a practical perspective. >> >> Maybe it is justified from an aesthetic perspective, but I think that >> should be clearly stated in the commit message. e.g. >> >> This change has no practical effect as all non-zero error statuses >> are treated equally, however the distinction between EINVAL and ENOMEM >> may be relevant at some future time and it seems cleaner to maintain >> the distinction. >> >> NeilBrown >> >> >>> The return code isn't discarded. A non-zero return code from >>> krb5_DK() is carried back up the call stack. The logic in >>> krb5_derive_key_v2() does not use the kernel's usual error flow >>> form, so I missed this. >>> >>> However, it still isn't clear to me why the error behavior here >>> needs to change. It's possible, for example, that -EINVAL is >>> perfectly adequate to indicate when sync_skcipher() can't find the >>> specified encryption algorithm (gk5e->encrypt_name). >>> >>> Specifying the wrong encryption type: -EINVAL. That makes sense. >>> >>> >>>>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >>>>> --- >>>>> v2: Update IS_ERR to PTR_ERR, thanks very much! >>>>> net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- >>>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c >>>>> index 4eb19c3a54c7..5ac8d06ab2c0 100644 >>>>> --- a/net/sunrpc/auth_gss/gss_krb5_keys.c >>>>> +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c >>>>> @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, >>>>> goto err_return; >>>>> >>>>> cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); >>>>> - if (IS_ERR(cipher)) >>>>> + if (IS_ERR(cipher)) { >>>>> + ret = PTR_ERR(cipher); >>>>> goto err_return; >>>>> + } >>>>> + >>>>> blocksize = crypto_sync_skcipher_blocksize(cipher); >>>>> - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) >>>>> + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); >>>>> + if (ret) >>>>> goto err_free_cipher; >>>>> >>>>> ret = -ENOMEM; >>>>> -- >>>>> 2.25.1 >>>>> >>>> -- >>>> Chuck Lever >>>> >>> -- >>> Chuck Lever >>> > -- > Chuck Lever > >
On Mon, Jul 15, 2024 at 09:44:30AM +0800, cuigaosheng wrote: > But crypto_sync_skcipher_setkey maybe return -ENOMEM, Should this be > modified? Auditing this path is a bit challenging. The obvious memory failure case is skcipher_setkey_unaligned(), but that is called only if the cipher does not provide its own ->setkey method. (Did you see a failure case that I missed?) So you'll have to generate a list of ciphers that krb5_DK() uses (which is only a few) and then check the ->setkey method of each of those ciphers. My guess is the skcipher_setkey fallback isn't used for any of the ciphers that SunRPC GSS currently cares about. > On 2024/7/15 0:18, Chuck Lever III wrote: > > > > > On Jul 14, 2024, at 12:31 AM, NeilBrown <neilb@suse.de> wrote: > > > > > > On Sat, 13 Jul 2024, Chuck Lever wrote: > > > > On Fri, Jul 12, 2024 at 09:39:08AM -0400, Chuck Lever wrote: > > > > > On Fri, Jul 12, 2024 at 03:24:23PM +0800, Gaosheng Cui wrote: > > > > > > Refactor the code in krb5_DK to return PTR_ERR when an error occurs. > > > > > My understanding of the current code is that if either > > > > > crypto_alloc_sync_skcipher() or crypto_sync_skcipher_blocksize() > > > > > fails, then krb5_DK() returns -EINVAL. At the only call site for > > > > > krb5_DK(), that return code is unconditionally discarded. Thus I > > > > > don't see that the proposed change is necessary or improves > > > > > anything. > > > > My understanding is wrong ;-) > > > True, but I think your conclusion was correct. > > > > > > krb5_DK() returns zero or -EINVAL. > > > It is only used by krb5_derive_key_v2(), which returns zero or -EINVAL, > > > or -ENOMEM. > > These are really the only three interesting return codes. > > Leaking other error codes to callers is not desirable, IMO. > > > > But looking at the current implementation of > > crypto_alloc_sync_skcipher(), it returns either > > ERR_PTR(-EINVAL) or a valid pointer; it doesn't return any > > other error value. Since it never returns -ENOMEM, there > > still doesn't seem to be a technical reason for modifying > > krb5_DK() to pass errors through. > > > > > > > krb4_derive_key_v2() is only used as a ->derive_key() method. > > > This is called from krb5_derive_key(), and various unit tests in > > > gss_krb5_tests.c > > > > > > krb5_derive_key() is only called in gss_krb5_mech.c, and each call site > > > is of the form: > > > if (krb5_derive_key(...)) goto out; > > > so it doesn't matter what error is returned. > > > > > > The unit test calls are all followed by > > > KUNIT_ASSERT_EQ(test, err, 0); > > > so the only place the err is used is (presumably) in failure reports > > > from the unit tests. > > > > > > So the proposed change seems unnecessary from a practical perspective. > > > > > > Maybe it is justified from an aesthetic perspective, but I think that > > > should be clearly stated in the commit message. e.g. > > > > > > This change has no practical effect as all non-zero error statuses > > > are treated equally, however the distinction between EINVAL and ENOMEM > > > may be relevant at some future time and it seems cleaner to maintain > > > the distinction. > > > > > > NeilBrown > > > > > > > > > > The return code isn't discarded. A non-zero return code from > > > > krb5_DK() is carried back up the call stack. The logic in > > > > krb5_derive_key_v2() does not use the kernel's usual error flow > > > > form, so I missed this. > > > > > > > > However, it still isn't clear to me why the error behavior here > > > > needs to change. It's possible, for example, that -EINVAL is > > > > perfectly adequate to indicate when sync_skcipher() can't find the > > > > specified encryption algorithm (gk5e->encrypt_name). > > > > > > > > Specifying the wrong encryption type: -EINVAL. That makes sense. > > > > > > > > > > > > > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > > > > > > --- > > > > > > v2: Update IS_ERR to PTR_ERR, thanks very much! > > > > > > net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c > > > > > > index 4eb19c3a54c7..5ac8d06ab2c0 100644 > > > > > > --- a/net/sunrpc/auth_gss/gss_krb5_keys.c > > > > > > +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c > > > > > > @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, > > > > > > goto err_return; > > > > > > > > > > > > cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); > > > > > > - if (IS_ERR(cipher)) > > > > > > + if (IS_ERR(cipher)) { > > > > > > + ret = PTR_ERR(cipher); > > > > > > goto err_return; > > > > > > + } > > > > > > + > > > > > > blocksize = crypto_sync_skcipher_blocksize(cipher); > > > > > > - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) > > > > > > + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); > > > > > > + if (ret) > > > > > > goto err_free_cipher; > > > > > > > > > > > > ret = -ENOMEM; > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > -- > > > > > Chuck Lever > > > > > > > > > -- > > > > Chuck Lever > > > > > > -- > > Chuck Lever > > > >
diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c b/net/sunrpc/auth_gss/gss_krb5_keys.c index 4eb19c3a54c7..5ac8d06ab2c0 100644 --- a/net/sunrpc/auth_gss/gss_krb5_keys.c +++ b/net/sunrpc/auth_gss/gss_krb5_keys.c @@ -164,10 +164,14 @@ static int krb5_DK(const struct gss_krb5_enctype *gk5e, goto err_return; cipher = crypto_alloc_sync_skcipher(gk5e->encrypt_name, 0, 0); - if (IS_ERR(cipher)) + if (IS_ERR(cipher)) { + ret = PTR_ERR(cipher); goto err_return; + } + blocksize = crypto_sync_skcipher_blocksize(cipher); - if (crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len)) + ret = crypto_sync_skcipher_setkey(cipher, inkey->data, inkey->len); + if (ret) goto err_free_cipher; ret = -ENOMEM;
Refactor the code in krb5_DK to return PTR_ERR when an error occurs. Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> --- v2: Update IS_ERR to PTR_ERR, thanks very much! net/sunrpc/auth_gss/gss_krb5_keys.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)