[PATCH] Use Intel AES instruction set if it exists.

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
This is somewhat of a "go faster" switch for Samba
with SMB1/2/3 signing and encryption.

Originally developed by Justin @ Netgear this
adds the Intel AES instruction set code from
the Linux kernel into third_party (it's GPLv2+
so the licensing is good).

The first patch adds the new code to third_party
and sets the HAVE_AESNI_INTEL config to 1 if found.

The second patch prepares lib/crypto/aes.c to call
the AES instructions and the third one adds the
runtime check (as we may have been compiled on
a system with Intel AES but be running on a system
without) to switch us into the hardware AES instructions
if available. NB. This doesn't change the Heimdal
aes code, just the code in lib/crypto called by
smbd and others.

It's a pretty minimal patchset and hopefully
pretty easy to understand and review as it
doesn't change any of the existing crypto
calls (except to rename their aes key schedule
variable inside the struct).

It would be good to get this in as it has been
found to give up to a 200% performance improvement
on certain workloads, and some variant of this
patch is already being included by most Samba
NAS vendors.

This will make it available to all, and has been
sorely needed in stock Samba for a while.

I've tested with and without HAVE_AESNI_INTEL
being detected and it compiles and runs in
both cases.

I've logged a bug:

https://bugzilla.samba.org/show_bug.cgi?id=13008

for this so if it gets in we should be
able to get this into 4.7.x (for x = 0 or
greater :-).

Please review and push if happy !

Jeremy.

intel-aes-master3.patch (104K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH] Use Intel AES instruction set if it exists - v2

Samba - samba-technical mailing list
On Thu, Aug 31, 2017 at 01:32:36PM -0700, Jeremy Allison wrote:

> This is somewhat of a "go faster" switch for Samba
> with SMB1/2/3 signing and encryption.
>
> Originally developed by Justin @ Netgear this
> adds the Intel AES instruction set code from
> the Linux kernel into third_party (it's GPLv2+
> so the licensing is good).
>
> The first patch adds the new code to third_party
> and sets the HAVE_AESNI_INTEL config to 1 if found.
>
> The second patch prepares lib/crypto/aes.c to call
> the AES instructions and the third one adds the
> runtime check (as we may have been compiled on
> a system with Intel AES but be running on a system
> without) to switch us into the hardware AES instructions
> if available. NB. This doesn't change the Heimdal
> aes code, just the code in lib/crypto called by
> smbd and others.
>
> It's a pretty minimal patchset and hopefully
> pretty easy to understand and review as it
> doesn't change any of the existing crypto
> calls (except to rename their aes key schedule
> variable inside the struct).
>
> It would be good to get this in as it has been
> found to give up to a 200% performance improvement
> on certain workloads, and some variant of this
> patch is already being included by most Samba
> NAS vendors.
>
> This will make it available to all, and has been
> sorely needed in stock Samba for a while.
>
> I've tested with and without HAVE_AESNI_INTEL
> being detected and it compiles and runs in
> both cases.
>
> I've logged a bug:
>
> https://bugzilla.samba.org/show_bug.cgi?id=13008
>
> for this so if it gets in we should be
> able to get this into 4.7.x (for x = 0 or
> greater :-).
>
> Please review and push if happy !
Second version - Justin pointed out we need the following
change in patch #1 to fix for non-x86 systems.

diff --git a/third_party/aesni-intel/wscript b/third_party/aesni-intel/wscript
index 151892f6889..ee7be031fd0 100644
--- a/third_party/aesni-intel/wscript
+++ b/third_party/aesni-intel/wscript
@@ -5,6 +5,9 @@ def configure(conf):
                conf.DEFINE('HAVE_AESNI_INTEL', 1)
 
 def build(bld):
+    if not bld.CONFIG_SET('HAVE_AESNI_INTEL'):
+        return
+
     bld.SAMBA_LIBRARY('aesni-intel',
                       source='aesni-intel_asm.c',
                       cflags='-Wp,-E,-lang-asm',

New patchset includes this change.

Jeremy.

intel-aes-master4.patch (104K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thursday, 31 August 2017 22:32:36 CEST Jeremy Allison wrote:
> This is somewhat of a "go faster" switch for Samba
> with SMB1/2/3 signing and encryption.
>
> Originally developed by Justin @ Netgear this
> adds the Intel AES instruction set code from
> the Linux kernel into third_party (it's GPLv2+
> so the licensing is good).

I've discussed this already with Metze when I implemented support for MS
Catalog Files last year. We are currently using GnuTLS for various things
inside of Samba (backupkey, TLS).

GnuTLS uses the nettle crypto library [1] which has a very nice and clean API.
It has support for AES NI and also other improvements. It also supports some
other crypto functions we use and it is easy to get code upstream.

I would really prefer to use a crypto library for this stuff instead of
rolling out our own crypto. It also makes it harder for people who maintain
packages in distributions, because then Samba is implementing crypto and we
have a harder time to get it certified or FIPS compliant.

So please before pushing this, look at libnettle! We have a file:

lib/crypto/REQUIREMENTS

which has a summary what we need and crypto libraries provide!



Please see that as a NAK till you looked into libnettle and can convince me
that doing our own crypto is better. We aren't cryptographers and we should
not maintain a crypto library.


Just my 2 cents,


        Andreas


P.S.: Jeremy, the nettle maintainer is a collegue of yours!


[1] http://www.lysator.liu.se/~nisse/nettle/

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Fri, 2017-09-01 at 08:49 +0200, Andreas Schneider via samba-
technical wrote:

> I've discussed this already with Metze when I implemented support for MS
> Catalog Files last year. We are currently using GnuTLS for various things
> inside of Samba (backupkey, TLS).
>
> GnuTLS uses the nettle crypto library [1] which has a very nice and clean API.
> It has support for AES NI and also other improvements. It also supports some
> other crypto functions we use and it is easy to get code upstream.
>
> I would really prefer to use a crypto library for this stuff instead of
> rolling out our own crypto. It also makes it harder for people who maintain
> packages in distributions, because then Samba is implementing crypto and we
> have a harder time to get it certified or FIPS compliant.
>
> So please before pushing this, look at libnettle! We have a file:
>
> lib/crypto/REQUIREMENTS
>
> which has a summary what we need and crypto libraries provide!
>
>
>
> Please see that as a NAK till you looked into libnettle and can convince me
> that doing our own crypto is better. We aren't cryptographers and we should
> not maintain a crypto library.

Andreas, 

I said much the same to Jeremy when he mentioned this to me long ago in
our occasional phone calls.

As such, I strong agree, and would like to move us further towards
using GnuTLS for as much of our crypto as possible.  I understand the
argument about 'working patches trump', but still don't want to be
maintaining more crypto code.

Sorry,

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Friday, 1 September 2017 11:30:04 CEST Andrew Bartlett wrote:

> On Fri, 2017-09-01 at 08:49 +0200, Andreas Schneider via samba-
>
> technical wrote:
> > I've discussed this already with Metze when I implemented support for MS
> > Catalog Files last year. We are currently using GnuTLS for various things
> > inside of Samba (backupkey, TLS).
> >
> > GnuTLS uses the nettle crypto library [1] which has a very nice and clean
> > API. It has support for AES NI and also other improvements. It also
> > supports some other crypto functions we use and it is easy to get code
> > upstream.
> >
> > I would really prefer to use a crypto library for this stuff instead of
> > rolling out our own crypto. It also makes it harder for people who
> > maintain
> > packages in distributions, because then Samba is implementing crypto and
> > we
> > have a harder time to get it certified or FIPS compliant.
> >
> > So please before pushing this, look at libnettle! We have a file:
> >
> > lib/crypto/REQUIREMENTS
> >
> > which has a summary what we need and crypto libraries provide!
> >
> >
> >
> > Please see that as a NAK till you looked into libnettle and can convince
> > me
> > that doing our own crypto is better. We aren't cryptographers and we
> > should
> > not maintain a crypto library.
>
> Andreas,
>
> I said much the same to Jeremy when he mentioned this to me long ago in
> our occasional phone calls.
>
> As such, I strong agree, and would like to move us further towards
> using GnuTLS for as much of our crypto as possible.  I understand the
> argument about 'working patches trump', but still don't want to be
> maintaining more crypto code.

I think for SMB it is is better to use nettle directly. GnuTLS does memory
allocations where nettle doesn't need them. gnutls_hash_init() for example.


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Fri, 2017-09-01 at 11:48 +0200, Andreas Schneider via samba-
technical wrote:

> On Friday, 1 September 2017 11:30:04 CEST Andrew Bartlett wrote:
> > On Fri, 2017-09-01 at 08:49 +0200, Andreas Schneider via samba-
> >
> > technical wrote:
> > >
> > > So please before pushing this, look at libnettle! We have a file:
> > >
> > > lib/crypto/REQUIREMENTS
> > >
> > > which has a summary what we need and crypto libraries provide!
> > >
> > >
> > >
> > > Please see that as a NAK till you looked into libnettle and can convince
> > > me
> > > that doing our own crypto is better. We aren't cryptographers and we
> > > should
> > > not maintain a crypto library.
> >
> > Andreas,
> >
> > I said much the same to Jeremy when he mentioned this to me long ago in
> > our occasional phone calls.
> >
> > As such, I strong agree, and would like to move us further towards
> > using GnuTLS for as much of our crypto as possible.  I understand the
> > argument about 'working patches trump', but still don't want to be
> > maintaining more crypto code.
>
> I think for SMB it is is better to use nettle directly. GnuTLS does memory
> allocations where nettle doesn't need them. gnutls_hash_init() for example.

Sure.  I don't know how others feel about a mandatory dependency, but I
think we should work to get out of the crypto business, even if it
makes us a little harder to install on some systems.  

We could go third_party if we have to, but if Linux is well covered we
should be able to avoid that.

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
Am 01.09.2017 um 12:06 schrieb Andrew Bartlett:

> On Fri, 2017-09-01 at 11:48 +0200, Andreas Schneider via samba-
> technical wrote:
>> On Friday, 1 September 2017 11:30:04 CEST Andrew Bartlett wrote:
>>> On Fri, 2017-09-01 at 08:49 +0200, Andreas Schneider via samba-
>>>
>>> technical wrote:
>>>>
>>>> So please before pushing this, look at libnettle! We have a file:
>>>>
>>>> lib/crypto/REQUIREMENTS
>>>>
>>>> which has a summary what we need and crypto libraries provide!
>>>>
>>>>
>>>>
>>>> Please see that as a NAK till you looked into libnettle and can convince
>>>> me
>>>> that doing our own crypto is better. We aren't cryptographers and we
>>>> should
>>>> not maintain a crypto library.
>>>
>>> Andreas,
>>>
>>> I said much the same to Jeremy when he mentioned this to me long ago in
>>> our occasional phone calls.
>>>
>>> As such, I strong agree, and would like to move us further towards
>>> using GnuTLS for as much of our crypto as possible.  I understand the
>>> argument about 'working patches trump', but still don't want to be
>>> maintaining more crypto code.
>>
>> I think for SMB it is is better to use nettle directly. GnuTLS does memory
>> allocations where nettle doesn't need them. gnutls_hash_init() for example.
>
> Sure.  I don't know how others feel about a mandatory dependency, but I
> think we should work to get out of the crypto business, even if it
> makes us a little harder to install on some systems.  
>
> We could go third_party if we have to, but if Linux is well covered we
> should be able to avoid that.
There's also some work in progress here:
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-smb-crypto

https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=3759eb23b38c
makes use of libnettle.

But is only replaces the low level aes and xor functions.

But what we really need are APIs like this (our current one):

void aes_gcm_128_init(struct aes_gcm_128_context *ctx,
                      const uint8_t K[AES_BLOCK_SIZE],
                      const uint8_t IV[AES_GCM_128_IV_SIZE]);
void aes_gcm_128_updateA(struct aes_gcm_128_context *ctx,
                         const uint8_t *a, size_t a_len);
void aes_gcm_128_updateC(struct aes_gcm_128_context *ctx,
                         const uint8_t *c, size_t c_len);
void aes_gcm_128_crypt(struct aes_gcm_128_context *ctx,
                       uint8_t *m, size_t m_len);
void aes_gcm_128_digest(struct aes_gcm_128_context *ctx,
                        uint8_t T[AES_BLOCK_SIZE]);

Or maybe even better:

void aes_gcm_128_init(struct aes_gcm_128_context *ctx,
                      const uint8_t K[AES_BLOCK_SIZE],
                      const uint8_t IV[AES_GCM_128_IV_SIZE]);
void aes_gcm_128_update(struct aes_gcm_128_context *ctx,
                        const struct iovec *a_iov, size_t a_iov_count,
                        struct iovec *m, size_t m_iov_count,
                        bool forward);
void aes_gcm_128_digest(struct aes_gcm_128_context *ctx,
                        uint8_t T[AES_BLOCK_SIZE]);

and give it a chance to be completely optimized by the hardware,
by doing parallel encryption, but still allow it to be passed
in chunks.

https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=1f83cbefd1b
gives the best performance with aes_gcm_128 using openssl as
it does almost everything with hardware instructions.

If I remember correctly aes_block_rshift and aes_block_shift consumed
the most cycles.

If we would rely an a crypto library, we'll have to extent that ourself
first and then live with a third_party copy for quite some time.

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Sep 01, 2017 at 11:48:22AM +0200, Andreas Schneider wrote:

> On Friday, 1 September 2017 11:30:04 CEST Andrew Bartlett wrote:
> > On Fri, 2017-09-01 at 08:49 +0200, Andreas Schneider via samba-
> >
> > technical wrote:
> > > I've discussed this already with Metze when I implemented support for MS
> > > Catalog Files last year. We are currently using GnuTLS for various things
> > > inside of Samba (backupkey, TLS).
> > >
> > > GnuTLS uses the nettle crypto library [1] which has a very nice and clean
> > > API. It has support for AES NI and also other improvements. It also
> > > supports some other crypto functions we use and it is easy to get code
> > > upstream.
> > >
> > > I would really prefer to use a crypto library for this stuff instead of
> > > rolling out our own crypto. It also makes it harder for people who
> > > maintain
> > > packages in distributions, because then Samba is implementing crypto and
> > > we
> > > have a harder time to get it certified or FIPS compliant.
> > >
> > > So please before pushing this, look at libnettle! We have a file:
> > >
> > > lib/crypto/REQUIREMENTS
> > >
> > > which has a summary what we need and crypto libraries provide!
> > >
> > >
> > >
> > > Please see that as a NAK till you looked into libnettle and can convince
> > > me
> > > that doing our own crypto is better. We aren't cryptographers and we
> > > should
> > > not maintain a crypto library.
> >
> > Andreas,
> >
> > I said much the same to Jeremy when he mentioned this to me long ago in
> > our occasional phone calls.
> >
> > As such, I strong agree, and would like to move us further towards
> > using GnuTLS for as much of our crypto as possible.  I understand the
> > argument about 'working patches trump', but still don't want to be
> > maintaining more crypto code.
>
> I think for SMB it is is better to use nettle directly. GnuTLS does memory
> allocations where nettle doesn't need them. gnutls_hash_init() for example.

Andreas and Andrew, this is an *OUT OF THE BOX* 2x performance improvement
when using signing and sealing, which is becoming an increasingly important
workload.

I understand wanting to use clean API's, and am willing to work
towards that. I have no problems working towards using libnettle.

But we are already using our own crypto, and this moves us to
a third_party library directly sourced from the Linux kernel,
so I disagree that this is 'maintaining our own crypto'. In
fact it's allowing the possibity of moving of the crypto *out*
of our code and into third_party.

As it doesn't change any of our API's this is an easy drop in
replacement.

Don't let the perfect be the enemy of working code that brings
great benefits RIGHT NOW !

Andreas, if you're NAKing this I'd like to see your commitment
to moving us to libnettle, and your timeframe for creating this
code please.

Jeremy.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Sep 01, 2017 at 10:06:46PM +1200, Andrew Bartlett wrote:
>
> Sure.  I don't know how others feel about a mandatory dependency, but I
> think we should work to get out of the crypto business, even if it

Great ! What is the timeframe for your Catalyst Team to
create the libnettle code we need ?

We've been sitting on a severe performance deficiency
for many years now (the Intel crypto code was added
to the Linux kernel in *2008*) and we have done
*NOTHING* to fix this in this time.

Netgear has offered us a solution (needed a little
work, but I was able to fix this up in a couple of
days) that fixes this issue right now.

I don't think it's sensible to ignore working code
in this way.

I'm happy to move us to libnettle, but how much
work is this going to take ? Who is committing
to do this work ? What timeframe ?

Continuing to ignore performance issues for the sake of
"jam tomorrow" is not wise IMHO.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Sep 01, 2017 at 12:30:08PM +0200, Stefan Metzmacher wrote:

>
> There's also some work in progress here:
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-smb-crypto
>
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=3759eb23b38c
> makes use of libnettle.
>
> But is only replaces the low level aes and xor functions.
>
> But what we really need are APIs like this (our current one):
>
> void aes_gcm_128_init(struct aes_gcm_128_context *ctx,
>                       const uint8_t K[AES_BLOCK_SIZE],
>                       const uint8_t IV[AES_GCM_128_IV_SIZE]);
> void aes_gcm_128_updateA(struct aes_gcm_128_context *ctx,
>                          const uint8_t *a, size_t a_len);
> void aes_gcm_128_updateC(struct aes_gcm_128_context *ctx,
>                          const uint8_t *c, size_t c_len);
> void aes_gcm_128_crypt(struct aes_gcm_128_context *ctx,
>                        uint8_t *m, size_t m_len);
> void aes_gcm_128_digest(struct aes_gcm_128_context *ctx,
>                         uint8_t T[AES_BLOCK_SIZE]);
>
> Or maybe even better:
>
> void aes_gcm_128_init(struct aes_gcm_128_context *ctx,
>                       const uint8_t K[AES_BLOCK_SIZE],
>                       const uint8_t IV[AES_GCM_128_IV_SIZE]);
> void aes_gcm_128_update(struct aes_gcm_128_context *ctx,
>                         const struct iovec *a_iov, size_t a_iov_count,
>                         struct iovec *m, size_t m_iov_count,
> bool forward);
> void aes_gcm_128_digest(struct aes_gcm_128_context *ctx,
>                         uint8_t T[AES_BLOCK_SIZE]);
>
> and give it a chance to be completely optimized by the hardware,
> by doing parallel encryption, but still allow it to be passed
> in chunks.
>
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=1f83cbefd1b
> gives the best performance with aes_gcm_128 using openssl as
> it does almost everything with hardware instructions.
>
> If I remember correctly aes_block_rshift and aes_block_shift consumed
> the most cycles.
>
> If we would rely an a crypto library, we'll have to extent that ourself
> first and then live with a third_party copy for quite some time.

Andreas and Andrew - PLEASE READ THE ABOVE..

"we'll have to extent that ourself first and then
live with a third_party copy for quite some time."

On what planet is that not VENDING OUR OWN crypto ?????

So right now I see you NAK'ing working code
that gives immediate performance improvements that
is being used by a vendor because it's vending our
own crypto, in favour of a plan that involves
vending our own crypto.

Please explain your logic here.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Friday, 1 September 2017 17:42:37 CEST Jeremy Allison wrote:

> On Fri, Sep 01, 2017 at 11:48:22AM +0200, Andreas Schneider wrote:
> > On Friday, 1 September 2017 11:30:04 CEST Andrew Bartlett wrote:
> > > On Fri, 2017-09-01 at 08:49 +0200, Andreas Schneider via samba-
> > >
> > > technical wrote:
> > > > I've discussed this already with Metze when I implemented support for
> > > > MS
> > > > Catalog Files last year. We are currently using GnuTLS for various
> > > > things
> > > > inside of Samba (backupkey, TLS).
> > > >
> > > > GnuTLS uses the nettle crypto library [1] which has a very nice and
> > > > clean
> > > > API. It has support for AES NI and also other improvements. It also
> > > > supports some other crypto functions we use and it is easy to get code
> > > > upstream.
> > > >
> > > > I would really prefer to use a crypto library for this stuff instead
> > > > of
> > > > rolling out our own crypto. It also makes it harder for people who
> > > > maintain
> > > > packages in distributions, because then Samba is implementing crypto
> > > > and
> > > > we
> > > > have a harder time to get it certified or FIPS compliant.
> > > >
> > > > So please before pushing this, look at libnettle! We have a file:
> > > >
> > > > lib/crypto/REQUIREMENTS
> > > >
> > > > which has a summary what we need and crypto libraries provide!
> > > >
> > > >
> > > >
> > > > Please see that as a NAK till you looked into libnettle and can
> > > > convince
> > > > me
> > > > that doing our own crypto is better. We aren't cryptographers and we
> > > > should
> > > > not maintain a crypto library.
> > >
> > > Andreas,
> > >
> > > I said much the same to Jeremy when he mentioned this to me long ago in
> > > our occasional phone calls.
> > >
> > > As such, I strong agree, and would like to move us further towards
> > > using GnuTLS for as much of our crypto as possible.  I understand the
> > > argument about 'working patches trump', but still don't want to be
> > > maintaining more crypto code.
> >
> > I think for SMB it is is better to use nettle directly. GnuTLS does memory
> > allocations where nettle doesn't need them. gnutls_hash_init() for
> > example.
>
> Andreas and Andrew, this is an *OUT OF THE BOX* 2x performance improvement
> when using signing and sealing, which is becoming an increasingly important
> workload.
>
> I understand wanting to use clean API's, and am willing to work
> towards that. I have no problems working towards using libnettle.

I'm ok if you add it but then start working on moving us in the direction to
libnettle or GnuTLS.

Günther and I normally have a major pain when Samba adds new crypto in its
source code.


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Fri, Sep 01, 2017 at 06:45:53PM +0200, Andreas Schneider wrote:

> On Friday, 1 September 2017 17:42:37 CEST Jeremy Allison wrote:
> >
> > Andreas and Andrew, this is an *OUT OF THE BOX* 2x performance improvement
> > when using signing and sealing, which is becoming an increasingly important
> > workload.
> >
> > I understand wanting to use clean API's, and am willing to work
> > towards that. I have no problems working towards using libnettle.
>
> I'm ok if you add it but then start working on moving us in the direction to
> libnettle or GnuTLS.

That's fair - I'm willing to do that.

> Günther and I normally have a major pain when Samba adds new crypto in its
> source code.

I understand. I won't add this until I've spoken to you in person
(or over the phone :-). Will you be at the SNIA SDC Conference next
week ? If not let's chat on the phone.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Friday, 1 September 2017 18:51:04 CEST Jeremy Allison wrote:

> On Fri, Sep 01, 2017 at 06:45:53PM +0200, Andreas Schneider wrote:
> > On Friday, 1 September 2017 17:42:37 CEST Jeremy Allison wrote:
> > > Andreas and Andrew, this is an *OUT OF THE BOX* 2x performance
> > > improvement
> > > when using signing and sealing, which is becoming an increasingly
> > > important
> > > workload.
> > >
> > > I understand wanting to use clean API's, and am willing to work
> > > towards that. I have no problems working towards using libnettle.
> >
> > I'm ok if you add it but then start working on moving us in the direction
> > to libnettle or GnuTLS.
>
> That's fair - I'm willing to do that.
>
> > Günther and I normally have a major pain when Samba adds new crypto in its
> > source code.
>
> I understand. I won't add this until I've spoken to you in person
> (or over the phone :-). Will you be at the SNIA SDC Conference next
> week ? If not let's chat on the phone.

I wont be at SNIA SDC. I'm going on vacation soon.



        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Fri, Sep 01, 2017 at 07:00:17PM +0200, Andreas Schneider wrote:

> On Friday, 1 September 2017 18:51:04 CEST Jeremy Allison wrote:
> > On Fri, Sep 01, 2017 at 06:45:53PM +0200, Andreas Schneider wrote:
> > > On Friday, 1 September 2017 17:42:37 CEST Jeremy Allison wrote:
> > > > Andreas and Andrew, this is an *OUT OF THE BOX* 2x performance
> > > > improvement
> > > > when using signing and sealing, which is becoming an increasingly
> > > > important
> > > > workload.
> > > >
> > > > I understand wanting to use clean API's, and am willing to work
> > > > towards that. I have no problems working towards using libnettle.
> > >
> > > I'm ok if you add it but then start working on moving us in the direction
> > > to libnettle or GnuTLS.
> >
> > That's fair - I'm willing to do that.
> >
> > > Günther and I normally have a major pain when Samba adds new crypto in its
> > > source code.
> >
> > I understand. I won't add this until I've spoken to you in person
> > (or over the phone :-). Will you be at the SNIA SDC Conference next
> > week ? If not let's chat on the phone.
>
> I wont be at SNIA SDC. I'm going on vacation soon.

OK, let's chat. In the meantime, parsing Metze's cryptic words
of wisdom... :-)

Isn't this:

https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=3759eb23b38c

*exactly* what we're both talking about ?

This replaces the low-level AES functions:

AES_set_encrypt_key()
AES_set_decrypt_key()
AES_encrypt()
AES_decrypt()

with the libnettle calls in pretty much
the same way as the proposed NetGear patchset.

If that libnettle code uses the Intel AES instruction
set directly this patch should produce exactly the
same beenfits as the one I posted....

It doesn't do the sophisticated changes, but
right now all we're taling about is replacing
the low-level calls.

It wouldn't surprise me if metze had already done
99% of the work I've just repeated, and just
hadn't told anyone about it :-).

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Fri, Sep 01, 2017 at 10:05:18AM -0700, Jeremy Allison via samba-technical wrote:
>
> OK, let's chat. In the meantime, parsing Metze's cryptic words
> of wisdom... :-)
>
> Isn't this:
>
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=3759eb23b38c
>
> *exactly* what we're both talking about ?

Indeed it was...

So as Metze already essentially *did* the libnettle work (just didn't
mention it :-) here is a version that uses libnettle for the 4 AES
functions (I added Metze's 'Signed-off-by:' as well as mine as it's
99% his work):

AES_set_encrypt_key()
AES_set_decrypt_key()
AES_encrypt()
AES_decrypt()

which (if libnettle has been compiled correctly) should use the Intel
AESNI instructions - giving the same speed benefits as the direct patch.

I'm getting Justin @ Netgear to test, so I'm not proposing this for
inclusion until I get the results - just wanted to report back sooner
rather than later (I got a bit snotty asking for resource and schedules
'cos I was pissed over people looking a gift horse in the mouth - sorry
about that). Seems I was looking Metze's gift horse in the mouth myself
(although in my defense I didn't know about it :-) :-).

Cheers,

Jeremy.

0001-lib-crypto-Add-the-ability-to-call-AES-implementatio.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
On Friday, 1 September 2017 21:12:08 CEST Jeremy Allison via samba-technical
wrote:
> On Fri, Sep 01, 2017 at 10:05:18AM -0700, Jeremy Allison via samba-technical
wrote:

> > OK, let's chat. In the meantime, parsing Metze's cryptic words
> > of wisdom... :-)
> >
> > Isn't this:
> >
> > https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=3759eb23b38c
> >
> > *exactly* what we're both talking about ?
>
> Indeed it was...
>
> So as Metze already essentially *did* the libnettle work (just didn't
> mention it :-) here is a version that uses libnettle for the 4 AES
> functions (I added Metze's 'Signed-off-by:' as well as mine as it's
> 99% his work):
>
> AES_set_encrypt_key()
> AES_set_decrypt_key()
> AES_encrypt()
> AES_decrypt()
>
> which (if libnettle has been compiled correctly) should use the Intel
> AESNI instructions - giving the same speed benefits as the direct patch.
>
> I'm getting Justin @ Netgear to test, so I'm not proposing this for
> inclusion until I get the results - just wanted to report back sooner
> rather than later (I got a bit snotty asking for resource and schedules
> 'cos I was pissed over people looking a gift horse in the mouth - sorry
> about that). Seems I was looking Metze's gift horse in the mouth myself
> (although in my defense I didn't know about it :-) :-).

The nettle implementation *could be* a slower than the one from the Kernel.
Niels wrote on the nettle mailinglist:

~~~~ quote ~~~~
* Nettle's AESNI assembly routines were written for simplicity and small
  code size, without putting a lot of effort into it. They could
  probably be sped up by some unrolling or more careful instruction
  scheduling. Patches welcome (but we shouldn't use excessive unrolling
  unless there's a significant speedup).
~~~~ quote ~~~~

http://lists.lysator.liu.se/pipermail/nettle-bugs/2017/003295.html

Looks like GnuTLS uses the OpenSSL assembler files (because they are BSD
licensed).



        Andreas



--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use Intel AES instruction set if it exists.

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi,

>> Sure.  I don't know how others feel about a mandatory dependency, but I
>> think we should work to get out of the crypto business, even if it
>> makes us a little harder to install on some systems.  
>>
>> We could go third_party if we have to, but if Linux is well covered we
>> should be able to avoid that.
>
> There's also some work in progress here:
> https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-smb-crypto
>
> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=3759eb23b38c
> makes use of libnettle.
>
> But is only replaces the low level aes and xor functions.
>
> But what we really need are APIs like this (our current one):
>
> void aes_gcm_128_init(struct aes_gcm_128_context *ctx,
>                       const uint8_t K[AES_BLOCK_SIZE],
>                       const uint8_t IV[AES_GCM_128_IV_SIZE]);
> void aes_gcm_128_updateA(struct aes_gcm_128_context *ctx,
>                          const uint8_t *a, size_t a_len);
> void aes_gcm_128_updateC(struct aes_gcm_128_context *ctx,
>                          const uint8_t *c, size_t c_len);
> void aes_gcm_128_crypt(struct aes_gcm_128_context *ctx,
>                        uint8_t *m, size_t m_len);
> void aes_gcm_128_digest(struct aes_gcm_128_context *ctx,
>                         uint8_t T[AES_BLOCK_SIZE]);
>
> Or maybe even better:
>
> void aes_gcm_128_init(struct aes_gcm_128_context *ctx,
>                       const uint8_t K[AES_BLOCK_SIZE],
>                       const uint8_t IV[AES_GCM_128_IV_SIZE]);
> void aes_gcm_128_update(struct aes_gcm_128_context *ctx,
>                         const struct iovec *a_iov, size_t a_iov_count,
>                         struct iovec *m, size_t m_iov_count,
> bool forward);
> void aes_gcm_128_digest(struct aes_gcm_128_context *ctx,
>                         uint8_t T[AES_BLOCK_SIZE]);
>
> and give it a chance to be completely optimized by the hardware,
> by doing parallel encryption, but still allow it to be passed
> in chunks.
This is important in order to avoid useless talloc and memcpy calls,
which are partly required because of the limited APIs provided
by existing crypto libraries.

> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=1f83cbefd1b
> gives the best performance with aes_gcm_128 using openssl as
> it does almost everything with hardware instructions.

By having better APIs the overhead would be even less if we could
avoid talloc and memcpy.

> If I remember correctly aes_block_rshift and aes_block_shift consumed
> the most cycles.

aes_block_xor() was also very high on the list...

> If we would rely an a crypto library, we'll have to extent that ourself
> first and then live with a third_party copy for quite some time.

metze



signature.asc (853 bytes) Download Attachment