Re: [PATCH] mac80211: aead api to reduce redundancy

[Xiang Gao]

2017-10-09 3:09 GMT-04:00 Johannes Berg :
> On Sun, 2017-10-08 at 01:43 -0400, Xiang Gao wrote:
>>
>> By the way, I'm still struggling on how to run unit tests. It might
>> take time for me to make it run on my machine.
>
> I can run it easily, so don't worry about it too much. Running it is of
> course much appreciated, but I don't really want to go and require that
> right now, it takes a long time to run.
>
> If you do want to set it up, I suggest the vm scripts (hostap
> repository in tests/hwsim/vm/ - you can use the kernel .config there as
> a base to compile a kernel and then just kick it off from there, but it
> can take a while to run.

Thanks for your help on this. This information is actually very helpful to me.

Since the unit test is not required, I will put working on this patch
higher priority than unit tests. I will send out patches without
running unit tests for now before I can make it run on my computer.
But I'm still interested in trying to run it on my computer after I
finish this patch.

I will send PATCH v3 soon.

Thanks

>
>> Hmm... good question. The reason is, aes_ccm.c and aes_gcm.c was
>> almost exact copy of each other. But they have different copyright
>> information.
>> The copyright of aes_ccm.c was:
>>
>> Copyright 2006, Devicescape Software, Inc.
>> Copyright 2003-2004, Instant802 Networks, Inc.
>>
>> and the copyright of aes_gcm.c was:
>>
>> Copyright 2014-2015, Qualcomm Atheros, Inc.
>>
>> I just don't know how to write the copyright for the new aead_api.c,
>> so I does not put anything there.
>
> Heh, good point. Well, I guess we can pretend it wasn't already copied
> before and just "keep" both.
>
> johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

On Sun, 2017-10-08 at 01:43 -0400, Xiang Gao wrote:
> 
> By the way, I'm still struggling on how to run unit tests. It might
> take time for me to make it run on my machine.

I can run it easily, so don't worry about it too much. Running it is of
course much appreciated, but I don't really want to go and require that
right now, it takes a long time to run.

If you do want to set it up, I suggest the vm scripts (hostap
repository in tests/hwsim/vm/ - you can use the kernel .config there as
a base to compile a kernel and then just kick it off from there, but it
can take a while to run.

> Hmm... good question. The reason is, aes_ccm.c and aes_gcm.c was
> almost exact copy of each other. But they have different copyright
> information.
> The copyright of aes_ccm.c was:
> 
> Copyright 2006, Devicescape Software, Inc.
> Copyright 2003-2004, Instant802 Networks, Inc.
> 
> and the copyright of aes_gcm.c was:
> 
> Copyright 2014-2015, Qualcomm Atheros, Inc.
> 
> I just don't know how to write the copyright for the new aead_api.c,
> so I does not put anything there.

Heh, good point. Well, I guess we can pretend it wasn't already copied
before and just "keep" both.

johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Xiang Gao]

Hi Johannes,

Thanks for your time on reviewing this. I will make changes following
your review. See details below.
By the way, I'm still struggling on how to run unit tests. It might
take time for me to make it run on my machine.

2017-10-02 8:04 GMT-04:00 Johannes Berg :
> Please use "v2" tag or so in the subject line, having the same patch
> again is really not helpful.
>
> The next should be v3, obviously.

Thanks for your patience to point this out. I will follow your instruction.

>
>> +++ b/net/mac80211/aead_api.c
>> @@ -1,7 +1,4 @@
>> -/*
>> - * Copyright 2014-2015, Qualcomm Atheros, Inc.
>> - *
>> - * This program is free software; you can redistribute it and/or
>> modify
>> +/* This program is free software; you can redistribute it and/or
>> modify
>
> I see no reason to make this change, why remove copyright?

Hmm... good question. The reason is, aes_ccm.c and aes_gcm.c was
almost exact copy of each other. But they have different copyright
information.
The copyright of aes_ccm.c was:

Copyright 2006, Devicescape Software, Inc.
Copyright 2003-2004, Instant802 Networks, Inc.

and the copyright of aes_gcm.c was:

Copyright 2014-2015, Qualcomm Atheros, Inc.

I just don't know how to write the copyright for the new aead_api.c,
so I does not put anything there.

These copyright information are still at aes_ccm.h and aes_gcm.h

What's your opinion on writing these copyright information? Do I write
all of them? like:

Copyright 2014-2015, Qualcomm Atheros, Inc.
Copyright 2006, Devicescape Software, Inc.
Copyright 2003-2004, Instant802 Networks, Inc.

>
>> +++ b/net/mac80211/wpa.c
>> @@ -464,7 +464,8 @@ static int ccmp_encrypt_skb(struct
>> ieee80211_tx_data *tx, struct sk_buff *skb,
>>   pos += IEEE80211_CCMP_HDR_LEN;
>>   ccmp_special_blocks(skb, pn, b_0, aad);
>>   return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad,
>> pos, len,
>> -  skb_put(skb, mic_len),
>> mic_len);
>> +  skb_put(skb,
>> +  key->u.ccmp.tfm-
>> >authsize));
>>  }
>
> I see no reason for the change from mic_len to authsize here?

This was because I was planning to put it to crypto directory, then I
changed it to the same name as in other crypto api. Now that it will
goes to the mac80211, I think it is time to revert this change.

>
>> @@ -540,10 +541,11 @@ ieee80211_crypto_ccmp_decrypt(struct
>> ieee80211_rx_data *rx,
>>   ccmp_special_blocks(skb, pn, b_0, aad);
>>
>>   if (ieee80211_aes_ccm_decrypt(
>> - key->u.ccmp.tfm, b_0, aad,
>> - skb->data + hdrlen + 
>> IEEE80211_CCMP_HDR_LEN,
>> - data_len,
>> - skb->data + skb->len - mic_len, mic_len))
>> + key->u.ccmp.tfm, b_0, aad,
>> + skb->data + hdrlen + IEEE80211_CCMP_HDR_LEN,
>> + data_len,
>> + skb->data + skb->len - 
>> key->u.ccmp.tfm->authsize
>> + ))
>>   return RX_DROP_UNUSABLE;
>
> That's a really really strange way of writing this ...
>
> Please reformat.

OK, I will reformat it.

>
> johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

Please use "v2" tag or so in the subject line, having the same patch
again is really not helpful.

The next should be v3, obviously.

> +++ b/net/mac80211/aead_api.c
> @@ -1,7 +1,4 @@
> -/*
> - * Copyright 2014-2015, Qualcomm Atheros, Inc.
> - *
> - * This program is free software; you can redistribute it and/or
> modify
> +/* This program is free software; you can redistribute it and/or
> modify

I see no reason to make this change, why remove copyright?

> +++ b/net/mac80211/wpa.c
> @@ -464,7 +464,8 @@ static int ccmp_encrypt_skb(struct
> ieee80211_tx_data *tx, struct sk_buff *skb,
>   pos += IEEE80211_CCMP_HDR_LEN;
>   ccmp_special_blocks(skb, pn, b_0, aad);
>   return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad,
> pos, len,
> -  skb_put(skb, mic_len),
> mic_len);
> +  skb_put(skb,
> +  key->u.ccmp.tfm-
> >authsize));
>  }

I see no reason for the change from mic_len to authsize here?

> @@ -540,10 +541,11 @@ ieee80211_crypto_ccmp_decrypt(struct
> ieee80211_rx_data *rx,
>   ccmp_special_blocks(skb, pn, b_0, aad);
>  
>   if (ieee80211_aes_ccm_decrypt(
> - key->u.ccmp.tfm, b_0, aad,
> - skb->data + hdrlen + IEEE80211_CCMP_HDR_LEN,
> - data_len,
> - skb->data + skb->len - mic_len, mic_len))
> + key->u.ccmp.tfm, b_0, aad,
> + skb->data + hdrlen + IEEE80211_CCMP_HDR_LEN,
> + data_len,
> + skb->data + skb->len - key->u.ccmp.tfm->authsize
> + ))
>   return RX_DROP_UNUSABLE;

That's a really really strange way of writing this ...

Please reformat.

johannes

[PATCH] mac80211: aead api to reduce redundancy

[Xiang Gao]

Currently, the aes_ccm.c and aes_gcm.c are almost line by line copy of
each other. This patch reduce code redundancy by moving the code in these
two files to crypto/aead_api.c to make it a higher level aead api. The
file aes_ccm.c and aes_gcm.c are removed and all the functions there are
now implemented in their headers using the newly added aead api.

Signed-off-by: Xiang Gao 
---
 net/mac80211/Makefile  |   3 +-
 net/mac80211/{aes_gcm.c => aead_api.c} |  49 +++---
 net/mac80211/aead_api.h|  26 
 net/mac80211/aes_ccm.c | 115 -
 net/mac80211/aes_ccm.h |  42 
 net/mac80211/aes_gcm.h |  38 ---
 net/mac80211/wpa.c |  12 ++--
 7 files changed, 118 insertions(+), 167 deletions(-)
 rename net/mac80211/{aes_gcm.c => aead_api.c} (55%)
 create mode 100644 net/mac80211/aead_api.h
 delete mode 100644 net/mac80211/aes_ccm.c

diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
index 282912245938..80f25ff2f24b 100644
--- a/net/mac80211/Makefile
+++ b/net/mac80211/Makefile
@@ -6,6 +6,7 @@ mac80211-y := \
driver-ops.o \
sta_info.o \
wep.o \
+   aead_api.o \
wpa.o \
scan.o offchannel.o \
ht.o agg-tx.o agg-rx.o \
@@ -15,8 +16,6 @@ mac80211-y := \
rate.o \
michael.o \
tkip.o \
-   aes_ccm.o \
-   aes_gcm.o \
aes_cmac.o \
aes_gmac.o \
fils_aead.o \
diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aead_api.c
similarity index 55%
rename from net/mac80211/aes_gcm.c
rename to net/mac80211/aead_api.c
index 8a4397cc1b08..6ac53d0c6977 100644
--- a/net/mac80211/aes_gcm.c
+++ b/net/mac80211/aead_api.c
@@ -1,7 +1,4 @@
-/*
- * Copyright 2014-2015, Qualcomm Atheros, Inc.
- *
- * This program is free software; you can redistribute it and/or modify
+/* This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
@@ -9,43 +6,43 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
-#include 
-#include "key.h"
-#include "aes_gcm.h"
+#include "aead_api.h"
 
-int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
- u8 *data, size_t data_len, u8 *mic)
+int aead_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
+u8 *data, size_t data_len, u8 *auth)
 {
struct scatterlist sg[3];
struct aead_request *aead_req;
int reqsize = sizeof(*aead_req) + crypto_aead_reqsize(tfm);
u8 *__aad;
 
-   aead_req = kzalloc(reqsize + GCM_AAD_LEN, GFP_ATOMIC);
+   aead_req = kzalloc(reqsize + aad_len, GFP_ATOMIC);
if (!aead_req)
return -ENOMEM;
 
__aad = (u8 *)aead_req + reqsize;
-   memcpy(__aad, aad, GCM_AAD_LEN);
+   memcpy(__aad, aad, aad_len);
 
sg_init_table(sg, 3);
-   sg_set_buf([0], &__aad[2], be16_to_cpup((__be16 *)__aad));
+   sg_set_buf([0], __aad, aad_len);
sg_set_buf([1], data, data_len);
-   sg_set_buf([2], mic, IEEE80211_GCMP_MIC_LEN);
+   sg_set_buf([2], auth, tfm->authsize);
 
aead_request_set_tfm(aead_req, tfm);
-   aead_request_set_crypt(aead_req, sg, sg, data_len, j_0);
+   aead_request_set_crypt(aead_req, sg, sg, data_len, b_0);
aead_request_set_ad(aead_req, sg[0].length);
 
crypto_aead_encrypt(aead_req);
kzfree(aead_req);
+
return 0;
 }
 
-int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
- u8 *data, size_t data_len, u8 *mic)
+int aead_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
+u8 *data, size_t data_len, u8 *auth)
 {
struct scatterlist sg[3];
struct aead_request *aead_req;
@@ -56,21 +53,20 @@ int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 
*j_0, u8 *aad,
if (data_len == 0)
return -EINVAL;
 
-   aead_req = kzalloc(reqsize + GCM_AAD_LEN, GFP_ATOMIC);
+   aead_req = kzalloc(reqsize + aad_len, GFP_ATOMIC);
if (!aead_req)
return -ENOMEM;
 
__aad = (u8 *)aead_req + reqsize;
-   memcpy(__aad, aad, GCM_AAD_LEN);
+   memcpy(__aad, aad, aad_len);
 
sg_init_table(sg, 3);
sg_set_buf([0], &__aad[2], be16_to_cpup((__be16 *)__aad));
sg_set_buf([1], data, data_len);
-   sg_set_buf([2], mic, IEEE80211_GCMP_MIC_LEN);
+   sg_set_buf([2], auth, tfm->authsize);
 
aead_request_set_tfm(aead_req, tfm);
-   aead_request_set_crypt(aead_req, sg, sg,
-  data_len + IEEE80211_GCMP_MIC_LEN, j_0);
+   aead_request_set_crypt(aead_req, sg, sg, data_len + tfm->authsize, b_0);
aead_request_set_ad(aead_req, sg[0].length);
 
err = 

Re: [PATCH] mac80211: aead api to reduce redundancy

[Herbert Xu]

On Mon, Sep 25, 2017 at 07:22:26AM +0200, Johannes Berg wrote:
> 
> The code moves to crypto/ though, and I'm not even sure I can vouch for
> the Makefile choice there.

Thanks, I missed that.  I don't think this belongs in crypto.
This proposed helper is only useful for wireless so it should
stay there.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

On Mon, 2017-09-25 at 12:56 +0800, Herbert Xu wrote:
> On Sun, Sep 24, 2017 at 07:42:46PM +0200, Johannes Berg wrote:
> > 
> > Unrelated to this, I'm not sure whose tree this should go through -
> > probably Herbert's (or DaveM's with his ACK? not sure if there's a
> > crypto tree?) or so?
> 
> Since you're just rearranging code invoking the crypto API, rather
> than touching actual crypto API code, I think you should handle it
> as you do with any other wireless patch.

The code moves to crypto/ though, and I'm not even sure I can vouch for
the Makefile choice there.

johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Herbert Xu]

On Sun, Sep 24, 2017 at 07:42:46PM +0200, Johannes Berg wrote:
>
> Unrelated to this, I'm not sure whose tree this should go through -
> probably Herbert's (or DaveM's with his ACK? not sure if there's a
> crypto tree?) or so?

Since you're just rearranging code invoking the crypto API, rather
than touching actual crypto API code, I think you should handle it
as you do with any other wireless patch.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] mac80211: aead api to reduce redundancy

[Xiang Gao]

2017-09-24 13:42 GMT-04:00 Johannes Berg :
> On Sun, 2017-09-24 at 13:21 -0400, Xiang Gao wrote:
>>
>> Do you mean to put more characters each line in the description
>>
> Huh, sorry, no - my bad. I was thinking of the code, not the
> description at all.

Oh yes, these indentation do looks ugly. Thank you for figuring this
out. The tab
width of my editor was set to 4. It should be 8... I will fix these
problems and resend
the patch soon, maybe after receiving a bit more feedback.

>
> For example here:
>
>> -int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
>> - u8 *data, size_t data_len, u8 *mic)
>> +int aead_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
>> +u8 *data, size_t data_len, u8 *auth)
>>
>
> I think you should adjust the indentation to match - or did it just get
> mangled in my mail? It looks *further* indented now, when it should be
> less (to after the opening parenthesis). Similarly in various other
> places.
>
> And perhaps for long things like
>
>> +static inline struct crypto_aead *ieee80211_aes_key_setup_encrypt(
>> +   const u8 key[], size_t key_len,
>> size_t mic_len)
>
>> +struct crypto_aead *aead_key_setup_encrypt(const char *alg,
>> +   const u8 key[], size_t key_len, size_t authsize);
>
> it might be better to write
>
> static inline struct crypto_aead *
> ieee80211_aes_key_setup_encrypt(const u8 key[], ...)
>
> and
>
> struct crypto_aead *
> aead_key_setup_encrypt(const char *alg, ...)
>
>
> respectively, depending on how far you have to indent to break lines
> etc.
>
> Anyway, I'm nitpicking.
>
> Unrelated to this, I'm not sure whose tree this should go through -
> probably Herbert's (or DaveM's with his ACK? not sure if there's a
> crypto tree?) or so?

Yes, there is one at
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/
I'm not sure which tree to go either. I'm also not sure about the
beginning of the patch title,
should it be "mac80211:" or "crypto:"?

Options are:
1. This whole patch goes to either mac80211 tree or crypto tree. I
don't know which is better.
2. Make the higher level api only for internal usage in mac80211, i.e.
move the aead_api.c and aead_api.h to net/mac80211, does not export
the symbol. And of course, this will go to the mac80211 tree. I
personally don't want this to be the final solution because I happen
to be writing a loadable kernel module that uses these higher level
api.
3. Maybe split this patch, one for changes in crypto, which will go to
crypto tree, and the other for mac80211 part, which goes to the
mac80211 tree?

>
> johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

On Sun, 2017-09-24 at 13:21 -0400, Xiang Gao wrote:
> 
> Do you mean to put more characters each line in the description
> 
Huh, sorry, no - my bad. I was thinking of the code, not the
description at all.

For example here:

> -int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
> - u8 *data, size_t data_len, u8 *mic)
> +int aead_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
> +u8 *data, size_t data_len, u8 *auth)
> 

I think you should adjust the indentation to match - or did it just get
mangled in my mail? It looks *further* indented now, when it should be
less (to after the opening parenthesis). Similarly in various other
places.

And perhaps for long things like

> +static inline struct crypto_aead *ieee80211_aes_key_setup_encrypt(
> +   const u8 key[], size_t key_len,
> size_t mic_len)

> +struct crypto_aead *aead_key_setup_encrypt(const char *alg,
> +   const u8 key[], size_t key_len, size_t authsize);

it might be better to write

static inline struct crypto_aead *
ieee80211_aes_key_setup_encrypt(const u8 key[], ...)

and

struct crypto_aead *
aead_key_setup_encrypt(const char *alg, ...)


respectively, depending on how far you have to indent to break lines
etc.

Anyway, I'm nitpicking.

Unrelated to this, I'm not sure whose tree this should go through -
probably Herbert's (or DaveM's with his ACK? not sure if there's a
crypto tree?) or so?

johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Xiang Gao]

2017-09-24 11:05 GMT-04:00 Johannes Berg :
> On Sun, 2017-09-24 at 01:40 -0400, Xiang Gao wrote:
>> Currently, the aes_ccm.c and aes_gcm.c are almost line by line
>> copy of each other. This patch reduce code redundancy by moving
>> the code in these two files to crypto/aead_api.c to make it a
>> higher level aead api. The aes_ccm.c and aes_gcm.c are removed
>> and all the functions are now implemented in their headers using
>> the newly added aead api.
>>
> No objection from me, though I'd ask you to respin with the indentation
> fixed up a bit.

Hi Johannes,

Thank you for you time for the suggestion. I'm not sure if I correctly
understand you point.
Do you mean to put more characters each line in the description? Something like:

> Currently, the aes_ccm.c and aes_gcm.c are almost line by line copy of
> each other. This patch reduce code redundancy by moving the code in these
> two files to crypto/aead_api.c to make it a higher level aead api. The
> file aes_ccm.c and aes_gcm.c are removed and all the functions there are
> now implemented in their headers using the newly added aead api.

instead of

> Currently, the aes_ccm.c and aes_gcm.c are almost line by line
> copy of each other. This patch reduce code redundancy by moving
> the code in these two files to crypto/aead_api.c to make it a
> higher level aead api. The aes_ccm.c and aes_gcm.c are removed
> and all the functions are now implemented in their headers using
> the newly added aead api.

Xiang Gao

>
> johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

On Sun, 2017-09-24 at 01:40 -0400, Xiang Gao wrote:
> Currently, the aes_ccm.c and aes_gcm.c are almost line by line
> copy of each other. This patch reduce code redundancy by moving
> the code in these two files to crypto/aead_api.c to make it a
> higher level aead api. The aes_ccm.c and aes_gcm.c are removed
> and all the functions are now implemented in their headers using
> the newly added aead api.
> 
No objection from me, though I'd ask you to respin with the indentation
fixed up a bit.

johannes