* [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 @ 2012-01-29 22:39 Jesper Juhl 2012-02-01 10:43 ` devendra.aaru 0 siblings, 1 reply; 8+ messages in thread From: Jesper Juhl @ 2012-01-29 22:39 UTC (permalink / raw) To: Herbert Xu, David S. Miller; +Cc: linux-kernel, linux-crypto, Steffen Klassert We declare 'exact' without initializing it and then do: [...] if (strlen(p->cru_driver_name)) exact = 1; if (priority && !exact) return -EINVAL; [...] If the first 'if' is not true, then the second will test an uninitialized 'exact'. As far as I can tell, what we want is for 'exact' to be initialized to 0 (zero/false). Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- crypto/crypto_user.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) Compile tested only. diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index 16f8693..36a2af7 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -304,7 +304,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh, static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh, struct nlattr **attrs) { - int exact; + int exact = 0; const char *name; struct crypto_alg *alg; struct crypto_user_alg *p = nlmsg_data(nlh); -- 1.7.8.4 -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 2012-01-29 22:39 [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 Jesper Juhl @ 2012-02-01 10:43 ` devendra.aaru 2012-02-01 20:21 ` Jesper Juhl 0 siblings, 1 reply; 8+ messages in thread From: devendra.aaru @ 2012-02-01 10:43 UTC (permalink / raw) To: Jesper Juhl Cc: Herbert Xu, David S. Miller, linux-kernel, linux-crypto, Steffen Klassert On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.net> wrote: > We declare 'exact' without initializing it and then do: > > [...] > if (strlen(p->cru_driver_name)) > exact = 1; > > if (priority && !exact) > return -EINVAL; > > [...] > > If the first 'if' is not true, then the second will test an > uninitialized 'exact'. not needed . as the cru_driver_name will always be present :). > As far as I can tell, what we want is for 'exact' to be initialized to > 0 (zero/false). > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > --- > crypto/crypto_user.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > Compile tested only. > > diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c > index 16f8693..36a2af7 100644 > --- a/crypto/crypto_user.c > +++ b/crypto/crypto_user.c > @@ -304,7 +304,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh, > static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh, > struct nlattr **attrs) > { > - int exact; > + int exact = 0; > const char *name; > struct crypto_alg *alg; > struct crypto_user_alg *p = nlmsg_data(nlh); > -- > 1.7.8.4 > > > -- > Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ > Don't top-post http://www.catb.org/jargon/html/T/top-post.html > Plain text mails only, please. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 2012-02-01 10:43 ` devendra.aaru @ 2012-02-01 20:21 ` Jesper Juhl 2012-02-01 20:36 ` Jesper Juhl 2012-02-02 7:51 ` Steffen Klassert 0 siblings, 2 replies; 8+ messages in thread From: Jesper Juhl @ 2012-02-01 20:21 UTC (permalink / raw) To: devendra.aaru Cc: Herbert Xu, David S. Miller, linux-kernel, linux-crypto, Steffen Klassert [-- Attachment #1: Type: TEXT/PLAIN, Size: 4981 bytes --] On Wed, 1 Feb 2012, devendra.aaru wrote: > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.net> wrote: > > We declare 'exact' without initializing it and then do: > > > > [...] > > if (strlen(p->cru_driver_name)) > > exact = 1; > > > > if (priority && !exact) > > return -EINVAL; > > > > [...] > > > > If the first 'if' is not true, then the second will test an > > uninitialized 'exact'. > > not needed . as the cru_driver_name will always be present :). If that is indeed the case, and we are guaranteed that, then it would seem that a patch like the following would be what we want instead?? Please note that this patch is intended just for discussion, nothing else (which is why I left out a Signed-off-by on purpose), since I've not tested it beyond checking that it compiles, nor have I verified your claim that cru_driver_name will always be present. Herbert, David, any input? Subject: [PATCH] Simplify crypto_add_alg() and crypto_alg_match() Since cru_driver_name will always be present, the test if (strlen(p->cru_driver_name)) exact = 1; will always be true. So there's no need to have the test at all, we can just unconditionally assign 1 to 'exact'. And if 'exact' is always 1, then we'll never take the true branch of if (priority && !exact) return -EINVAL; so we may as well just remove those two lines completely. At this point we may as well just remove 'exact' entirely and just use a hardcoded 1 in the call to crypto_alg_match(). The 'name' variable is also entirely redundant since with cru_driver_name always being present we'll always take the first branch in if (strlen(p->cru_driver_name)) name = p->cru_driver_name; else name = p->cru_name; and then we may as well just use 'p->cru_driver_name' itself the one place where it is needed in the call to crypto_alg_mod_lookup(). At this point all calls to the static crypto_alg crypto_alg_match() function pass 1 as the second argument, so there's not really any point any more in that function having that argument, so let's remove it and the code that tests it. And since strlen(p->cru_driver_name) will also always be true in that function, we can simplify the assignment to 'match'. --- crypto/crypto_user.c | 33 ++++++++------------------------- 1 files changed, 8 insertions(+), 25 deletions(-) diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index 16f8693..7760c22 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -38,23 +38,19 @@ struct crypto_dump_info { u16 nlmsg_flags; }; -static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p, int exact) +static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p) { struct crypto_alg *q, *alg = NULL; down_read(&crypto_alg_sem); list_for_each_entry(q, &crypto_alg_list, cra_list) { - int match = 0; + int match; if ((q->cra_flags ^ p->cru_type) & p->cru_mask) continue; - if (strlen(p->cru_driver_name)) - match = !strcmp(q->cra_driver_name, - p->cru_driver_name); - else if (!exact) - match = !strcmp(q->cra_name, p->cru_name); + match = !strcmp(q->cra_driver_name, p->cru_driver_name); if (match) { alg = q; @@ -195,7 +191,7 @@ static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh, if (!p->cru_driver_name) return -EINVAL; - alg = crypto_alg_match(p, 1); + alg = crypto_alg_match(p); if (!alg) return -ENOENT; @@ -259,7 +255,7 @@ static int crypto_update_alg(struct sk_buff *skb, struct nlmsghdr *nlh, if (priority && !strlen(p->cru_driver_name)) return -EINVAL; - alg = crypto_alg_match(p, 1); + alg = crypto_alg_match(p); if (!alg) return -ENOENT; @@ -283,7 +279,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh, struct crypto_alg *alg; struct crypto_user_alg *p = nlmsg_data(nlh); - alg = crypto_alg_match(p, 1); + alg = crypto_alg_match(p); if (!alg) return -ENOENT; @@ -304,28 +300,15 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh, static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh, struct nlattr **attrs) { - int exact; - const char *name; struct crypto_alg *alg; struct crypto_user_alg *p = nlmsg_data(nlh); struct nlattr *priority = attrs[CRYPTOCFGA_PRIORITY_VAL]; - if (strlen(p->cru_driver_name)) - exact = 1; - - if (priority && !exact) - return -EINVAL; - - alg = crypto_alg_match(p, exact); + alg = crypto_alg_match(p); if (alg) return -EEXIST; - if (strlen(p->cru_driver_name)) - name = p->cru_driver_name; - else - name = p->cru_name; - - alg = crypto_alg_mod_lookup(name, p->cru_type, p->cru_mask); + alg = crypto_alg_mod_lookup(p->cru_driver_name, p->cru_type, p->cru_mask); if (IS_ERR(alg)) return PTR_ERR(alg); -- 1.7.9 -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 2012-02-01 20:21 ` Jesper Juhl @ 2012-02-01 20:36 ` Jesper Juhl 2012-02-02 7:51 ` Steffen Klassert 1 sibling, 0 replies; 8+ messages in thread From: Jesper Juhl @ 2012-02-01 20:36 UTC (permalink / raw) To: devendra.aaru Cc: Herbert Xu, David S. Miller, linux-kernel, linux-crypto, Steffen Klassert [-- Attachment #1: Type: TEXT/PLAIN, Size: 5485 bytes --] On Wed, 1 Feb 2012, Jesper Juhl wrote: > On Wed, 1 Feb 2012, devendra.aaru wrote: > > > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.net> wrote: > > > We declare 'exact' without initializing it and then do: > > > > > > [...] > > > if (strlen(p->cru_driver_name)) > > > exact = 1; > > > > > > if (priority && !exact) > > > return -EINVAL; > > > > > > [...] > > > > > > If the first 'if' is not true, then the second will test an > > > uninitialized 'exact'. > > > > not needed . as the cru_driver_name will always be present :). > > If that is indeed the case, and we are guaranteed that, then it would seem > that a patch like the following would be what we want instead?? > > Please note that this patch is intended just for discussion, nothing else > (which is why I left out a Signed-off-by on purpose), since I've not > tested it beyond checking that it compiles, nor have I verified your claim > that cru_driver_name will always be present. > > Herbert, David, any input? > > > Subject: [PATCH] Simplify crypto_add_alg() and crypto_alg_match() > > Since cru_driver_name will always be present, the test > > if (strlen(p->cru_driver_name)) > exact = 1; > > will always be true. So there's no need to have the test at all, we > can just unconditionally assign 1 to 'exact'. > > And if 'exact' is always 1, then we'll never take the true branch of > > if (priority && !exact) > return -EINVAL; > > so we may as well just remove those two lines completely. > > At this point we may as well just remove 'exact' entirely and just use > a hardcoded 1 in the call to crypto_alg_match(). > > The 'name' variable is also entirely redundant since with > cru_driver_name always being present we'll always take the first > branch in > > if (strlen(p->cru_driver_name)) > name = p->cru_driver_name; > else > name = p->cru_name; > > and then we may as well just use 'p->cru_driver_name' itself the one > place where it is needed in the call to crypto_alg_mod_lookup(). > > At this point all calls to the static crypto_alg crypto_alg_match() > function pass 1 as the second argument, so there's not really any > point any more in that function having that argument, so let's remove > it and the code that tests it. > > And since strlen(p->cru_driver_name) will also always be true in that > function, we can simplify the assignment to 'match'. > > --- > crypto/crypto_user.c | 33 ++++++++------------------------- > 1 files changed, 8 insertions(+), 25 deletions(-) > > diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c > index 16f8693..7760c22 100644 > --- a/crypto/crypto_user.c > +++ b/crypto/crypto_user.c > @@ -38,23 +38,19 @@ struct crypto_dump_info { > u16 nlmsg_flags; > }; > > -static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p, int exact) > +static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p) > { > struct crypto_alg *q, *alg = NULL; > > down_read(&crypto_alg_sem); > > list_for_each_entry(q, &crypto_alg_list, cra_list) { > - int match = 0; > + int match; > > if ((q->cra_flags ^ p->cru_type) & p->cru_mask) > continue; > > - if (strlen(p->cru_driver_name)) > - match = !strcmp(q->cra_driver_name, > - p->cru_driver_name); > - else if (!exact) > - match = !strcmp(q->cra_name, p->cru_name); > + match = !strcmp(q->cra_driver_name, p->cru_driver_name); > > if (match) { Actually, we could just get rid of 'match' entirely as well and just do if (!strcmp(q->cra_driver_name, p->cru_driver_name)) { > alg = q; > @@ -195,7 +191,7 @@ static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh, > if (!p->cru_driver_name) > return -EINVAL; > > - alg = crypto_alg_match(p, 1); > + alg = crypto_alg_match(p); > if (!alg) > return -ENOENT; > > @@ -259,7 +255,7 @@ static int crypto_update_alg(struct sk_buff *skb, struct nlmsghdr *nlh, > if (priority && !strlen(p->cru_driver_name)) > return -EINVAL; > > - alg = crypto_alg_match(p, 1); > + alg = crypto_alg_match(p); > if (!alg) > return -ENOENT; > > @@ -283,7 +279,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh, > struct crypto_alg *alg; > struct crypto_user_alg *p = nlmsg_data(nlh); > > - alg = crypto_alg_match(p, 1); > + alg = crypto_alg_match(p); > if (!alg) > return -ENOENT; > > @@ -304,28 +300,15 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh, > static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh, > struct nlattr **attrs) > { > - int exact; > - const char *name; > struct crypto_alg *alg; > struct crypto_user_alg *p = nlmsg_data(nlh); > struct nlattr *priority = attrs[CRYPTOCFGA_PRIORITY_VAL]; > > - if (strlen(p->cru_driver_name)) > - exact = 1; > - > - if (priority && !exact) > - return -EINVAL; > - > - alg = crypto_alg_match(p, exact); > + alg = crypto_alg_match(p); > if (alg) > return -EEXIST; > > - if (strlen(p->cru_driver_name)) > - name = p->cru_driver_name; > - else > - name = p->cru_name; > - > - alg = crypto_alg_mod_lookup(name, p->cru_type, p->cru_mask); > + alg = crypto_alg_mod_lookup(p->cru_driver_name, p->cru_type, p->cru_mask); > if (IS_ERR(alg)) > return PTR_ERR(alg); > > -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 2012-02-01 20:21 ` Jesper Juhl 2012-02-01 20:36 ` Jesper Juhl @ 2012-02-02 7:51 ` Steffen Klassert 2012-02-02 14:42 ` Jesper Juhl 1 sibling, 1 reply; 8+ messages in thread From: Steffen Klassert @ 2012-02-02 7:51 UTC (permalink / raw) To: Jesper Juhl Cc: devendra.aaru, Herbert Xu, David S. Miller, linux-kernel, linux-crypto On Wed, Feb 01, 2012 at 09:21:39PM +0100, Jesper Juhl wrote: > On Wed, 1 Feb 2012, devendra.aaru wrote: > > > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.net> wrote: > > > We declare 'exact' without initializing it and then do: > > > > > > [...] > > > if (strlen(p->cru_driver_name)) > > > exact = 1; > > > > > > if (priority && !exact) > > > return -EINVAL; > > > > > > [...] > > > > > > If the first 'if' is not true, then the second will test an > > > uninitialized 'exact'. > > > > not needed . as the cru_driver_name will always be present :). > > If that is indeed the case, and we are guaranteed that, then it would seem > that a patch like the following would be what we want instead?? > > Please note that this patch is intended just for discussion, nothing else > (which is why I left out a Signed-off-by on purpose), since I've not > tested it beyond checking that it compiles, nor have I verified your claim > that cru_driver_name will always be present. > We get cru_driver_name from a netlink message that a user sends us. So it depends pretty much on the user whether cru_driver_name is set or not. Usually it is set when a user wants to instantiate a certain algorithm driver, like "cbc(aes-asm)". If the user just wants to instantiate the system default of an algorithm, he can set cru_name (e.g. to "cbc(aes)") instead of cru_driver_name. Your first patch is correct. Thanks, Steffen ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 2012-02-02 7:51 ` Steffen Klassert @ 2012-02-02 14:42 ` Jesper Juhl 2012-02-03 6:24 ` Steffen Klassert 0 siblings, 1 reply; 8+ messages in thread From: Jesper Juhl @ 2012-02-02 14:42 UTC (permalink / raw) To: Steffen Klassert Cc: devendra.aaru, Herbert Xu, David S. Miller, linux-kernel, linux-crypto [-- Attachment #1: Type: TEXT/PLAIN, Size: 1855 bytes --] On Thu, 2 Feb 2012, Steffen Klassert wrote: > On Wed, Feb 01, 2012 at 09:21:39PM +0100, Jesper Juhl wrote: > > On Wed, 1 Feb 2012, devendra.aaru wrote: > > > > > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <jj@chaosbits.net> wrote: > > > > We declare 'exact' without initializing it and then do: > > > > > > > > [...] > > > > if (strlen(p->cru_driver_name)) > > > > exact = 1; > > > > > > > > if (priority && !exact) > > > > return -EINVAL; > > > > > > > > [...] > > > > > > > > If the first 'if' is not true, then the second will test an > > > > uninitialized 'exact'. > > > > > > not needed . as the cru_driver_name will always be present :). > > > > If that is indeed the case, and we are guaranteed that, then it would seem > > that a patch like the following would be what we want instead?? > > > > Please note that this patch is intended just for discussion, nothing else > > (which is why I left out a Signed-off-by on purpose), since I've not > > tested it beyond checking that it compiles, nor have I verified your claim > > that cru_driver_name will always be present. > > > > We get cru_driver_name from a netlink message that a user sends us. > So it depends pretty much on the user whether cru_driver_name is > set or not. Usually it is set when a user wants to instantiate > a certain algorithm driver, like "cbc(aes-asm)". If the user just > wants to instantiate the system default of an algorithm, he can > set cru_name (e.g. to "cbc(aes)") instead of cru_driver_name. > > Your first patch is correct. > Thank you for the explanation. Can I take that to mean that I can add your Acked-by: if/when I resend the patch? -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 2012-02-02 14:42 ` Jesper Juhl @ 2012-02-03 6:24 ` Steffen Klassert 2012-02-05 4:12 ` Herbert Xu 0 siblings, 1 reply; 8+ messages in thread From: Steffen Klassert @ 2012-02-03 6:24 UTC (permalink / raw) To: Jesper Juhl Cc: devendra.aaru, Herbert Xu, David S. Miller, linux-kernel, linux-crypto On Thu, Feb 02, 2012 at 03:42:43PM +0100, Jesper Juhl wrote: > On Thu, 2 Feb 2012, Steffen Klassert wrote: > > > > Your first patch is correct. > > > Thank you for the explanation. > > Can I take that to mean that I can add your Acked-by: if/when I resend the > patch? > Sure, Acked-by: Steffen Klassert <steffen.klassert@secunet.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 2012-02-03 6:24 ` Steffen Klassert @ 2012-02-05 4:12 ` Herbert Xu 0 siblings, 0 replies; 8+ messages in thread From: Herbert Xu @ 2012-02-05 4:12 UTC (permalink / raw) To: Steffen Klassert Cc: Jesper Juhl, devendra.aaru, David S. Miller, linux-kernel, linux-crypto On Fri, Feb 03, 2012 at 07:24:25AM +0100, Steffen Klassert wrote: > On Thu, Feb 02, 2012 at 03:42:43PM +0100, Jesper Juhl wrote: > > On Thu, 2 Feb 2012, Steffen Klassert wrote: > > > > > > Your first patch is correct. > > > > > Thank you for the explanation. > > > > Can I take that to mean that I can add your Acked-by: if/when I resend the > > patch? > > > > Sure, > > Acked-by: Steffen Klassert <steffen.klassert@secunet.com> Patch applied. Thanks a lot! -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-02-05 4:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-29 22:39 [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0 Jesper Juhl 2012-02-01 10:43 ` devendra.aaru 2012-02-01 20:21 ` Jesper Juhl 2012-02-01 20:36 ` Jesper Juhl 2012-02-02 7:51 ` Steffen Klassert 2012-02-02 14:42 ` Jesper Juhl 2012-02-03 6:24 ` Steffen Klassert 2012-02-05 4:12 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).