Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hi Tyler, Michael

Did you have a chance to look at my latest reply below?
If we split the patch into smaller pieces and provide testing module with
prints that registers for events, as was suggested previously, can we
submit it for review ?

Regards, Andrey

>> On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
>>> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>>> >> This is a hardware inline accelerator, meaning that it operates on
>>> much
>>> >> lower layer, block layer and device driver layer. The HW encrypts
>>> plain
>>> >> requests sent from block layer directly, thus doing it much more
>>> >> efficiently rather than using crypto API.
>>> >
>>>
>>> > I feel like basing this on eCryptfs is an odd choice. The overhead of
>>> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
>>> > duplication of page cache and dentry cache entries (for upper and
>>> lower
>>> > caches) seems like considerable baggage for such a nifty, new
>>> hardware
>>> > feature.
>>> >
>>>
>>> First of all, one of the leading companies on the mobile market uses
>>> eCryptfs as a solution for their file-based-encryption feature and they
>>> use it along with HW crypto engine. I believe we will soon see interest
>>> from additional companies.
>>>
>>> Secondly, eCryptfs is convenient in many ways for implementing
>>> file-based-encryption and there are not so many good alternatives as of
>>> today.
>>
>> EXT4 and F2FS both have native file-based encryption capabilities.
>> They don't suffer from a lot of the overhead and page ballooning
>> issues that eCryptfs has. Nobody should be putting development effort
>> toward using eCryptfs on top of EXT4 or F2FS.
>>
>> There's clear value in enabling inline hardware encryption on mobile
>> platforms. However I don't think trying to wedge it in via a stacked
>> file system is a good long-term strategy. We should be moving toward
>> integrating inline hardware encryption into file systems that are
>> deployed in those environments.
>>
>> Given the amount of code duplication between EXT4 and F2FS encryption,
>> we should also be thinking about more general support for encryption
>> at the VFS/MM layer.
>>
>
> 1. It is true that EXT4 kernel has recently received encryption
> capabilities and that it is more efficient in terms of page management. We
> do have plans on integrating it with our inline crypto engine once
> Ext4Crypt is officially part of Android kernel and is supported by Android
>
> 2. This does not mean that the same work can't be done in parallel on
> on eCryptfs, especially taking into account that this solution is
> already deployed by some of our customers, major mobile phone
> manufacturers, and has shown significant improvement in performance
> compared to regular encryption via crypto API's
>
> 3. Even though eCryptfs has extra overhead, it's main advantage is
> providing encryption on top of any major FS, thus until (and if at all)
> there is encryption support as part of VFS/MM, eCryptfs with relatively
> small changes that we provide can still do efficient inline encryption
>
>>> You are right regarding overhead, page duplication, etc., however
>>> enabling eCryptfs to work with HW encryption still makes it a very
>>> efficient solution. Also, it is not just inline HW crypto engine,
>>> any HW crypto engine operates more efficiently while working on long
>>> chunks of data. Our suggested solution uses the existing block layer
>>> (that is part of the standard Linux storage stack) feature that
>>> gathers number of block to into one request and then encrypt/decrypt
>>> the data, whereas eCryptfs can only perform the crypto operation on
>>> single pages. This feature was tested on our platforms and
>>> demonstrated very significance performance improvement comparing to
>>> existing SW based solutions
>>>
>>> >> In order to use such HW efficiently with eCryptfs, eCryptfs
>>> encryption
>>> >> has
>>> >> to be canceled and it will need to call for external module instead
>>> that
>>> >> will do the correct marking for the blocks to distinguish between
>>> those
>>> >> that need to be encrypted by the HW from those that do not need.
>>> >>
>>> >> We are considering posting the code that will call
>>> >> ecryptfs_register_to_events() as a separate patch, but haven't done
>>> so
>>> >> yet. It is HW specific.
>>> >> Currently we are only proposing framework change so that it can
>>> allow
>>> >> for
>>> >> external modules to be connected
>>> >
>>> > In that case, I cannot accept this patch. I will have no way to test
>>> the
>>> > external module functionality and will not be able to make changes to
>>> > that area of the code because it will not be possible for me to fix
>>> > anything that I've broken. I won't even be able to know if I've
>>> broken
>>> > anything.
>>> >
>>> > If you are able to upstream the external module code, then I'll do a
>>> > proper review of this patch. I should note that I've only glanced at
>>> the

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hi Tyler, Michael

Did you have a chance to look at my latest reply below?
If we split the patch into smaller pieces and provide testing module with
prints that registers for events, as was suggested previously, can we
submit it for review ?

Regards, Andrey

>> On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
>>> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>>> >> This is a hardware inline accelerator, meaning that it operates on
>>> much
>>> >> lower layer, block layer and device driver layer. The HW encrypts
>>> plain
>>> >> requests sent from block layer directly, thus doing it much more
>>> >> efficiently rather than using crypto API.
>>> >
>>>
>>> > I feel like basing this on eCryptfs is an odd choice. The overhead of
>>> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
>>> > duplication of page cache and dentry cache entries (for upper and
>>> lower
>>> > caches) seems like considerable baggage for such a nifty, new
>>> hardware
>>> > feature.
>>> >
>>>
>>> First of all, one of the leading companies on the mobile market uses
>>> eCryptfs as a solution for their file-based-encryption feature and they
>>> use it along with HW crypto engine. I believe we will soon see interest
>>> from additional companies.
>>>
>>> Secondly, eCryptfs is convenient in many ways for implementing
>>> file-based-encryption and there are not so many good alternatives as of
>>> today.
>>
>> EXT4 and F2FS both have native file-based encryption capabilities.
>> They don't suffer from a lot of the overhead and page ballooning
>> issues that eCryptfs has. Nobody should be putting development effort
>> toward using eCryptfs on top of EXT4 or F2FS.
>>
>> There's clear value in enabling inline hardware encryption on mobile
>> platforms. However I don't think trying to wedge it in via a stacked
>> file system is a good long-term strategy. We should be moving toward
>> integrating inline hardware encryption into file systems that are
>> deployed in those environments.
>>
>> Given the amount of code duplication between EXT4 and F2FS encryption,
>> we should also be thinking about more general support for encryption
>> at the VFS/MM layer.
>>
>
> 1. It is true that EXT4 kernel has recently received encryption
> capabilities and that it is more efficient in terms of page management. We
> do have plans on integrating it with our inline crypto engine once
> Ext4Crypt is officially part of Android kernel and is supported by Android
>
> 2. This does not mean that the same work can't be done in parallel on
> on eCryptfs, especially taking into account that this solution is
> already deployed by some of our customers, major mobile phone
> manufacturers, and has shown significant improvement in performance
> compared to regular encryption via crypto API's
>
> 3. Even though eCryptfs has extra overhead, it's main advantage is
> providing encryption on top of any major FS, thus until (and if at all)
> there is encryption support as part of VFS/MM, eCryptfs with relatively
> small changes that we provide can still do efficient inline encryption
>
>>> You are right regarding overhead, page duplication, etc., however
>>> enabling eCryptfs to work with HW encryption still makes it a very
>>> efficient solution. Also, it is not just inline HW crypto engine,
>>> any HW crypto engine operates more efficiently while working on long
>>> chunks of data. Our suggested solution uses the existing block layer
>>> (that is part of the standard Linux storage stack) feature that
>>> gathers number of block to into one request and then encrypt/decrypt
>>> the data, whereas eCryptfs can only perform the crypto operation on
>>> single pages. This feature was tested on our platforms and
>>> demonstrated very significance performance improvement comparing to
>>> existing SW based solutions
>>>
>>> >> In order to use such HW efficiently with eCryptfs, eCryptfs
>>> encryption
>>> >> has
>>> >> to be canceled and it will need to call for external module instead
>>> that
>>> >> will do the correct marking for the blocks to distinguish between
>>> those
>>> >> that need to be encrypted by the HW from those that do not need.
>>> >>
>>> >> We are considering posting the code that will call
>>> >> ecryptfs_register_to_events() as a separate patch, but haven't done
>>> so
>>> >> yet. It is HW specific.
>>> >> Currently we are only proposing framework change so that it can
>>> allow
>>> >> for
>>> >> external modules to be connected
>>> >
>>> > In that case, I cannot accept this patch. I will have no way to test
>>> the
>>> > external module functionality and will not be able to make changes to
>>> > that area of the code because it will not be possible for me to fix
>>> > anything that I've broken. I won't even be able to know if I've
>>> broken
>>> > anything.
>>> >
>>> > If you are able to upstream the external module code, then I'll do a
>>> > proper review of this patch. I should note that I've only glanced at
>>> the

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hi Tyler, Michael

Did you have a chance to look at my latest reply below?
If we split the patch into smaller pieces and provide testing module with
prints that registers for events, as was suggested previously, can we
submit it for review ?

Regards, Andrey

>> On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
>>> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>>> >> This is a hardware inline accelerator, meaning that it operates on
>>> much
>>> >> lower layer, block layer and device driver layer. The HW encrypts
>>> plain
>>> >> requests sent from block layer directly, thus doing it much more
>>> >> efficiently rather than using crypto API.
>>> >
>>>
>>> > I feel like basing this on eCryptfs is an odd choice. The overhead of
>>> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
>>> > duplication of page cache and dentry cache entries (for upper and
>>> lower
>>> > caches) seems like considerable baggage for such a nifty, new
>>> hardware
>>> > feature.
>>> >
>>>
>>> First of all, one of the leading companies on the mobile market uses
>>> eCryptfs as a solution for their file-based-encryption feature and they
>>> use it along with HW crypto engine. I believe we will soon see interest
>>> from additional companies.
>>>
>>> Secondly, eCryptfs is convenient in many ways for implementing
>>> file-based-encryption and there are not so many good alternatives as of
>>> today.
>>
>> EXT4 and F2FS both have native file-based encryption capabilities.
>> They don't suffer from a lot of the overhead and page ballooning
>> issues that eCryptfs has. Nobody should be putting development effort
>> toward using eCryptfs on top of EXT4 or F2FS.
>>
>> There's clear value in enabling inline hardware encryption on mobile
>> platforms. However I don't think trying to wedge it in via a stacked
>> file system is a good long-term strategy. We should be moving toward
>> integrating inline hardware encryption into file systems that are
>> deployed in those environments.
>>
>> Given the amount of code duplication between EXT4 and F2FS encryption,
>> we should also be thinking about more general support for encryption
>> at the VFS/MM layer.
>>
>
> 1. It is true that EXT4 kernel has recently received encryption
> capabilities and that it is more efficient in terms of page management. We
> do have plans on integrating it with our inline crypto engine once
> Ext4Crypt is officially part of Android kernel and is supported by Android
>
> 2. This does not mean that the same work can't be done in parallel on
> on eCryptfs, especially taking into account that this solution is
> already deployed by some of our customers, major mobile phone
> manufacturers, and has shown significant improvement in performance
> compared to regular encryption via crypto API's
>
> 3. Even though eCryptfs has extra overhead, it's main advantage is
> providing encryption on top of any major FS, thus until (and if at all)
> there is encryption support as part of VFS/MM, eCryptfs with relatively
> small changes that we provide can still do efficient inline encryption
>
>>> You are right regarding overhead, page duplication, etc., however
>>> enabling eCryptfs to work with HW encryption still makes it a very
>>> efficient solution. Also, it is not just inline HW crypto engine,
>>> any HW crypto engine operates more efficiently while working on long
>>> chunks of data. Our suggested solution uses the existing block layer
>>> (that is part of the standard Linux storage stack) feature that
>>> gathers number of block to into one request and then encrypt/decrypt
>>> the data, whereas eCryptfs can only perform the crypto operation on
>>> single pages. This feature was tested on our platforms and
>>> demonstrated very significance performance improvement comparing to
>>> existing SW based solutions
>>>
>>> >> In order to use such HW efficiently with eCryptfs, eCryptfs
>>> encryption
>>> >> has
>>> >> to be canceled and it will need to call for external module instead
>>> that
>>> >> will do the correct marking for the blocks to distinguish between
>>> those
>>> >> that need to be encrypted by the HW from those that do not need.
>>> >>
>>> >> We are considering posting the code that will call
>>> >> ecryptfs_register_to_events() as a separate patch, but haven't done
>>> so
>>> >> yet. It is HW specific.
>>> >> Currently we are only proposing framework change so that it can
>>> allow
>>> >> for
>>> >> external modules to be connected
>>> >
>>> > In that case, I cannot accept this patch. I will have no way to test
>>> the
>>> > external module functionality and will not be able to make changes to
>>> > that area of the code because it will not be possible for me to fix
>>> > anything that I've broken. I won't even be able to know if I've
>>> broken
>>> > anything.
>>> >
>>> > If you are able to upstream the external module code, then I'll do a
>>> > proper review of this patch. I should note that I've only glanced at
>>> the

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hi Tyler, Michael

Did you have a chance to look at my latest reply below?
If we split the patch into smaller pieces and provide testing module with
prints that registers for events, as was suggested previously, can we
submit it for review ?

Regards, Andrey

>> On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
>>> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>>> >> This is a hardware inline accelerator, meaning that it operates on
>>> much
>>> >> lower layer, block layer and device driver layer. The HW encrypts
>>> plain
>>> >> requests sent from block layer directly, thus doing it much more
>>> >> efficiently rather than using crypto API.
>>> >
>>>
>>> > I feel like basing this on eCryptfs is an odd choice. The overhead of
>>> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
>>> > duplication of page cache and dentry cache entries (for upper and
>>> lower
>>> > caches) seems like considerable baggage for such a nifty, new
>>> hardware
>>> > feature.
>>> >
>>>
>>> First of all, one of the leading companies on the mobile market uses
>>> eCryptfs as a solution for their file-based-encryption feature and they
>>> use it along with HW crypto engine. I believe we will soon see interest
>>> from additional companies.
>>>
>>> Secondly, eCryptfs is convenient in many ways for implementing
>>> file-based-encryption and there are not so many good alternatives as of
>>> today.
>>
>> EXT4 and F2FS both have native file-based encryption capabilities.
>> They don't suffer from a lot of the overhead and page ballooning
>> issues that eCryptfs has. Nobody should be putting development effort
>> toward using eCryptfs on top of EXT4 or F2FS.
>>
>> There's clear value in enabling inline hardware encryption on mobile
>> platforms. However I don't think trying to wedge it in via a stacked
>> file system is a good long-term strategy. We should be moving toward
>> integrating inline hardware encryption into file systems that are
>> deployed in those environments.
>>
>> Given the amount of code duplication between EXT4 and F2FS encryption,
>> we should also be thinking about more general support for encryption
>> at the VFS/MM layer.
>>
>
> 1. It is true that EXT4 kernel has recently received encryption
> capabilities and that it is more efficient in terms of page management. We
> do have plans on integrating it with our inline crypto engine once
> Ext4Crypt is officially part of Android kernel and is supported by Android
>
> 2. This does not mean that the same work can't be done in parallel on
> on eCryptfs, especially taking into account that this solution is
> already deployed by some of our customers, major mobile phone
> manufacturers, and has shown significant improvement in performance
> compared to regular encryption via crypto API's
>
> 3. Even though eCryptfs has extra overhead, it's main advantage is
> providing encryption on top of any major FS, thus until (and if at all)
> there is encryption support as part of VFS/MM, eCryptfs with relatively
> small changes that we provide can still do efficient inline encryption
>
>>> You are right regarding overhead, page duplication, etc., however
>>> enabling eCryptfs to work with HW encryption still makes it a very
>>> efficient solution. Also, it is not just inline HW crypto engine,
>>> any HW crypto engine operates more efficiently while working on long
>>> chunks of data. Our suggested solution uses the existing block layer
>>> (that is part of the standard Linux storage stack) feature that
>>> gathers number of block to into one request and then encrypt/decrypt
>>> the data, whereas eCryptfs can only perform the crypto operation on
>>> single pages. This feature was tested on our platforms and
>>> demonstrated very significance performance improvement comparing to
>>> existing SW based solutions
>>>
>>> >> In order to use such HW efficiently with eCryptfs, eCryptfs
>>> encryption
>>> >> has
>>> >> to be canceled and it will need to call for external module instead
>>> that
>>> >> will do the correct marking for the blocks to distinguish between
>>> those
>>> >> that need to be encrypted by the HW from those that do not need.
>>> >>
>>> >> We are considering posting the code that will call
>>> >> ecryptfs_register_to_events() as a separate patch, but haven't done
>>> so
>>> >> yet. It is HW specific.
>>> >> Currently we are only proposing framework change so that it can
>>> allow
>>> >> for
>>> >> external modules to be connected
>>> >
>>> > In that case, I cannot accept this patch. I will have no way to test
>>> the
>>> > external module functionality and will not be able to make changes to
>>> > that area of the code because it will not be possible for me to fix
>>> > anything that I've broken. I won't even be able to know if I've
>>> broken
>>> > anything.
>>> >
>>> > If you are able to upstream the external module code, then I'll do a
>>> > proper review of this patch. I should note that I've only glanced at
>>> the

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

> On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
>> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>> >> This is a hardware inline accelerator, meaning that it operates on
>> much
>> >> lower layer, block layer and device driver layer. The HW encrypts
>> plain
>> >> requests sent from block layer directly, thus doing it much more
>> >> efficiently rather than using crypto API.
>> >
>>
>> > I feel like basing this on eCryptfs is an odd choice. The overhead of
>> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
>> > duplication of page cache and dentry cache entries (for upper and
>> lower
>> > caches) seems like considerable baggage for such a nifty, new hardware
>> > feature.
>> >
>>
>> First of all, one of the leading companies on the mobile market uses
>> eCryptfs as a solution for their file-based-encryption feature and they
>> use it along with HW crypto engine. I believe we will soon see interest
>> from additional companies.
>>
>> Secondly, eCryptfs is convenient in many ways for implementing
>> file-based-encryption and there are not so many good alternatives as of
>> today.
>
> EXT4 and F2FS both have native file-based encryption capabilities.
> They don't suffer from a lot of the overhead and page ballooning
> issues that eCryptfs has. Nobody should be putting development effort
> toward using eCryptfs on top of EXT4 or F2FS.
>
> There's clear value in enabling inline hardware encryption on mobile
> platforms. However I don't think trying to wedge it in via a stacked
> file system is a good long-term strategy. We should be moving toward
> integrating inline hardware encryption into file systems that are
> deployed in those environments.
>
> Given the amount of code duplication between EXT4 and F2FS encryption,
> we should also be thinking about more general support for encryption
> at the VFS/MM layer.
>

1. It is true that EXT4 kernel has recently received encryption
capabilities and that it is more efficient in terms of page management. We
do have plans on integrating it with our inline crypto engine once
Ext4Crypt is officially part of Android kernel and is supported by Android

2. This does not mean that the same work can't be done in parallel on
on eCryptfs, especially taking into account that this solution is
already deployed by some of our customers, major mobile phone
manufacturers, and has shown significant improvement in performance
compared to regular encryption via crypto API's

3. Even though eCryptfs has extra overhead, it's main advantage is
providing encryption on top of any major FS, thus until (and if at all)
there is encryption support as part of VFS/MM, eCryptfs with relatively
small changes that we provide can still do efficient inline encryption

>> You are right regarding overhead, page duplication, etc., however
>> enabling eCryptfs to work with HW encryption still makes it a very
>> efficient solution. Also, it is not just inline HW crypto engine,
>> any HW crypto engine operates more efficiently while working on long
>> chunks of data. Our suggested solution uses the existing block layer
>> (that is part of the standard Linux storage stack) feature that
>> gathers number of block to into one request and then encrypt/decrypt
>> the data, whereas eCryptfs can only perform the crypto operation on
>> single pages. This feature was tested on our platforms and
>> demonstrated very significance performance improvement comparing to
>> existing SW based solutions
>>
>> >> In order to use such HW efficiently with eCryptfs, eCryptfs
>> encryption
>> >> has
>> >> to be canceled and it will need to call for external module instead
>> that
>> >> will do the correct marking for the blocks to distinguish between
>> those
>> >> that need to be encrypted by the HW from those that do not need.
>> >>
>> >> We are considering posting the code that will call
>> >> ecryptfs_register_to_events() as a separate patch, but haven't done
>> so
>> >> yet. It is HW specific.
>> >> Currently we are only proposing framework change so that it can allow
>> >> for
>> >> external modules to be connected
>> >
>> > In that case, I cannot accept this patch. I will have no way to test
>> the
>> > external module functionality and will not be able to make changes to
>> > that area of the code because it will not be possible for me to fix
>> > anything that I've broken. I won't even be able to know if I've broken
>> > anything.
>> >
>> > If you are able to upstream the external module code, then I'll do a
>> > proper review of this patch. I should note that I've only glanced at
>> the
>> > patch but if feels like it should be broken up into smaller, easier to
>> > review patches before it can be properly reviewed.
>> >
>> > Tyler
>>
>> We can upstream the external module code, however as I mentioned this
>> module is specific for our HW, so you won't be able to test it either.
>> However we can pretty easily turn it into some dummy module for testing
>> 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

> On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
>> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>> >> This is a hardware inline accelerator, meaning that it operates on
>> much
>> >> lower layer, block layer and device driver layer. The HW encrypts
>> plain
>> >> requests sent from block layer directly, thus doing it much more
>> >> efficiently rather than using crypto API.
>> >
>>
>> > I feel like basing this on eCryptfs is an odd choice. The overhead of
>> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
>> > duplication of page cache and dentry cache entries (for upper and
>> lower
>> > caches) seems like considerable baggage for such a nifty, new hardware
>> > feature.
>> >
>>
>> First of all, one of the leading companies on the mobile market uses
>> eCryptfs as a solution for their file-based-encryption feature and they
>> use it along with HW crypto engine. I believe we will soon see interest
>> from additional companies.
>>
>> Secondly, eCryptfs is convenient in many ways for implementing
>> file-based-encryption and there are not so many good alternatives as of
>> today.
>
> EXT4 and F2FS both have native file-based encryption capabilities.
> They don't suffer from a lot of the overhead and page ballooning
> issues that eCryptfs has. Nobody should be putting development effort
> toward using eCryptfs on top of EXT4 or F2FS.
>
> There's clear value in enabling inline hardware encryption on mobile
> platforms. However I don't think trying to wedge it in via a stacked
> file system is a good long-term strategy. We should be moving toward
> integrating inline hardware encryption into file systems that are
> deployed in those environments.
>
> Given the amount of code duplication between EXT4 and F2FS encryption,
> we should also be thinking about more general support for encryption
> at the VFS/MM layer.
>

1. It is true that EXT4 kernel has recently received encryption
capabilities and that it is more efficient in terms of page management. We
do have plans on integrating it with our inline crypto engine once
Ext4Crypt is officially part of Android kernel and is supported by Android

2. This does not mean that the same work can't be done in parallel on
on eCryptfs, especially taking into account that this solution is
already deployed by some of our customers, major mobile phone
manufacturers, and has shown significant improvement in performance
compared to regular encryption via crypto API's

3. Even though eCryptfs has extra overhead, it's main advantage is
providing encryption on top of any major FS, thus until (and if at all)
there is encryption support as part of VFS/MM, eCryptfs with relatively
small changes that we provide can still do efficient inline encryption

>> You are right regarding overhead, page duplication, etc., however
>> enabling eCryptfs to work with HW encryption still makes it a very
>> efficient solution. Also, it is not just inline HW crypto engine,
>> any HW crypto engine operates more efficiently while working on long
>> chunks of data. Our suggested solution uses the existing block layer
>> (that is part of the standard Linux storage stack) feature that
>> gathers number of block to into one request and then encrypt/decrypt
>> the data, whereas eCryptfs can only perform the crypto operation on
>> single pages. This feature was tested on our platforms and
>> demonstrated very significance performance improvement comparing to
>> existing SW based solutions
>>
>> >> In order to use such HW efficiently with eCryptfs, eCryptfs
>> encryption
>> >> has
>> >> to be canceled and it will need to call for external module instead
>> that
>> >> will do the correct marking for the blocks to distinguish between
>> those
>> >> that need to be encrypted by the HW from those that do not need.
>> >>
>> >> We are considering posting the code that will call
>> >> ecryptfs_register_to_events() as a separate patch, but haven't done
>> so
>> >> yet. It is HW specific.
>> >> Currently we are only proposing framework change so that it can allow
>> >> for
>> >> external modules to be connected
>> >
>> > In that case, I cannot accept this patch. I will have no way to test
>> the
>> > external module functionality and will not be able to make changes to
>> > that area of the code because it will not be possible for me to fix
>> > anything that I've broken. I won't even be able to know if I've broken
>> > anything.
>> >
>> > If you are able to upstream the external module code, then I'll do a
>> > proper review of this patch. I should note that I've only glanced at
>> the
>> > patch but if feels like it should be broken up into smaller, easier to
>> > review patches before it can be properly reviewed.
>> >
>> > Tyler
>>
>> We can upstream the external module code, however as I mentioned this
>> module is specific for our HW, so you won't be able to test it either.
>> However we can pretty easily turn it into some dummy module for testing
>> 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Michael Halcrow]

On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
> >> This is a hardware inline accelerator, meaning that it operates on much
> >> lower layer, block layer and device driver layer. The HW encrypts plain
> >> requests sent from block layer directly, thus doing it much more
> >> efficiently rather than using crypto API.
> >
> 
> > I feel like basing this on eCryptfs is an odd choice. The overhead of
> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
> > duplication of page cache and dentry cache entries (for upper and lower
> > caches) seems like considerable baggage for such a nifty, new hardware
> > feature.
> >
> 
> First of all, one of the leading companies on the mobile market uses
> eCryptfs as a solution for their file-based-encryption feature and they
> use it along with HW crypto engine. I believe we will soon see interest
> from additional companies.
> 
> Secondly, eCryptfs is convenient in many ways for implementing
> file-based-encryption and there are not so many good alternatives as of
> today.

EXT4 and F2FS both have native file-based encryption capabilities.
They don't suffer from a lot of the overhead and page ballooning
issues that eCryptfs has. Nobody should be putting development effort
toward using eCryptfs on top of EXT4 or F2FS.

There's clear value in enabling inline hardware encryption on mobile
platforms. However I don't think trying to wedge it in via a stacked
file system is a good long-term strategy. We should be moving toward
integrating inline hardware encryption into file systems that are
deployed in those environments.

Given the amount of code duplication between EXT4 and F2FS encryption,
we should also be thinking about more general support for encryption
at the VFS/MM layer.

> You are right regarding overhead, page duplication, etc., however
> enabling eCryptfs to work with HW encryption still makes it a very
> efficient solution. Also, it is not just inline HW crypto engine,
> any HW crypto engine operates more efficiently while working on long
> chunks of data. Our suggested solution uses the existing block layer
> (that is part of the standard Linux storage stack) feature that
> gathers number of block to into one request and then encrypt/decrypt
> the data, whereas eCryptfs can only perform the crypto operation on
> single pages. This feature was tested on our platforms and
> demonstrated very significance performance improvement comparing to
> existing SW based solutions
> 
> >> In order to use such HW efficiently with eCryptfs, eCryptfs encryption
> >> has
> >> to be canceled and it will need to call for external module instead that
> >> will do the correct marking for the blocks to distinguish between those
> >> that need to be encrypted by the HW from those that do not need.
> >>
> >> We are considering posting the code that will call
> >> ecryptfs_register_to_events() as a separate patch, but haven't done so
> >> yet. It is HW specific.
> >> Currently we are only proposing framework change so that it can allow
> >> for
> >> external modules to be connected
> >
> > In that case, I cannot accept this patch. I will have no way to test the
> > external module functionality and will not be able to make changes to
> > that area of the code because it will not be possible for me to fix
> > anything that I've broken. I won't even be able to know if I've broken
> > anything.
> >
> > If you are able to upstream the external module code, then I'll do a
> > proper review of this patch. I should note that I've only glanced at the
> > patch but if feels like it should be broken up into smaller, easier to
> > review patches before it can be properly reviewed.
> >
> > Tyler
> 
> We can upstream the external module code, however as I mentioned this
> module is specific for our HW, so you won't be able to test it either.
> However we can pretty easily turn it into some dummy module for testing
> purposes only that will just do prints instead of HW specific calls. Would
> this be sufficient ? If so, where in project tree should I put it ?
> 
> >
> >>
> >> > On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
> >> >> Hello, Tyler
> >> >>
> >> >> I'll try to provide more detailed explanation, should it be
> >> satisfactory
> >> >> enough I will update the patch description.
> >> >>
> >> >> The problem with current eCryptfs is that it has total control on how
> >> >> and
> >> >> when the encryption is performed and this control can't be altered.
> >> One
> >> >> example when this can be a problem is when we want to utilize an
> >> >> underlying inline HW encryption engine which allows encrypting blocks
> >> >> 'on
> >> >> the fly' as they are being written to the storage. In such a case
> >> >> relevant
> >> >> blocks just need to be marked as 'should be encrypted'. No actual
> >> >> encryption should be done by eCryptfs as it will be much slower.
> >> >
> >> > Is this a hardware 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

> On Sun, Nov 08, 2015 at 10:10:00AM +0200, Andrey Markovytch wrote:
>> +++ b/fs/ecryptfs/caches_utils.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>
> Really?  This looks like copy and paste from core code that defintively
> was not written by the Linux Foundation.  In addition this patch comes
> from Qualcomm so something very fishy is going on here, which if I'd
> call copyrght fraud if I'd want to be be mean.
>
> Please a) stop pointlessly copy and pasting code and b) have a word with
> your lawyers on how to attribute code both that your wrote and which has
> been copy and pasted.
>

Hi Christoph,
Regarding the license, after double checking this, it seems that there has
been a mistake and we unproperly attributed Linux copyright for the code
that actually came from open source files having a different license. I
appologise for that, I am checking this internally and will update patch
with proper license.
As for the 'pointless copy paste', the code was taken from files where
those functions are declared as internal static, we had no choise but to
copy paste them, other option would be alternating the original code and
make those 2 functions public which seems unjustified in this case.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

> On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>> This is a hardware inline accelerator, meaning that it operates on much
>> lower layer, block layer and device driver layer. The HW encrypts plain
>> requests sent from block layer directly, thus doing it much more
>> efficiently rather than using crypto API.
>

> I feel like basing this on eCryptfs is an odd choice. The overhead of
> setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
> duplication of page cache and dentry cache entries (for upper and lower
> caches) seems like considerable baggage for such a nifty, new hardware
> feature.
>

First of all, one of the leading companies on the mobile market uses
eCryptfs as a solution for their file-based-encryption feature and they
use it along with HW crypto engine. I believe we will soon see interest
from additional companies.

Secondly, eCryptfs is convenient in many ways for implementing
file-based-encryption and there are not so many good alternatives as of
today. You are right regarding overhead, page duplication, etc., however
enabling eCryptfs to work with HW encryption still makes it a very
efficient solution. Also, it is not just inline HW crypto engine, any HW
crypto engine operates more efficiently while working on long chunks of
data. Our suggested solution uses the existing block layer (that is part
of the standard Linux storage stack) feature that gathers number of block
to into one request and then encrypt/decrypt the data, whereas eCryptfs
can only perform the crypto operation on single pages. This feature was
tested on our platforms and demonstrated very significance performance
improvement comparing to existing SW based solutions

>> In order to use such HW efficiently with eCryptfs, eCryptfs encryption
>> has
>> to be canceled and it will need to call for external module instead that
>> will do the correct marking for the blocks to distinguish between those
>> that need to be encrypted by the HW from those that do not need.
>>
>> We are considering posting the code that will call
>> ecryptfs_register_to_events() as a separate patch, but haven't done so
>> yet. It is HW specific.
>> Currently we are only proposing framework change so that it can allow
>> for
>> external modules to be connected
>
> In that case, I cannot accept this patch. I will have no way to test the
> external module functionality and will not be able to make changes to
> that area of the code because it will not be possible for me to fix
> anything that I've broken. I won't even be able to know if I've broken
> anything.
>
> If you are able to upstream the external module code, then I'll do a
> proper review of this patch. I should note that I've only glanced at the
> patch but if feels like it should be broken up into smaller, easier to
> review patches before it can be properly reviewed.
>
> Tyler

We can upstream the external module code, however as I mentioned this
module is specific for our HW, so you won't be able to test it either.
However we can pretty easily turn it into some dummy module for testing
purposes only that will just do prints instead of HW specific calls. Would
this be sufficient ? If so, where in project tree should I put it ?

>
>>
>> > On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
>> >> Hello, Tyler
>> >>
>> >> I'll try to provide more detailed explanation, should it be
>> satisfactory
>> >> enough I will update the patch description.
>> >>
>> >> The problem with current eCryptfs is that it has total control on how
>> >> and
>> >> when the encryption is performed and this control can't be altered.
>> One
>> >> example when this can be a problem is when we want to utilize an
>> >> underlying inline HW encryption engine which allows encrypting blocks
>> >> 'on
>> >> the fly' as they are being written to the storage. In such a case
>> >> relevant
>> >> blocks just need to be marked as 'should be encrypted'. No actual
>> >> encryption should be done by eCryptfs as it will be much slower.
>> >
>> > Is this a hardware crypto accelerator? If so, why not create a crypto
>> > api driver so all subsystems can take advantage of the acceleration
>> > instead of baking support into individual subsystems?
>> >
>> >> The provided framework allows transferring this control (if needed)
>> to
>> >> some external module which will do the encryption itfelf or just mark
>> >> the
>> >> appropriate blocks.
>> >>
>> >> There is no caller for ecryptfs_register_to_events() since this
>> change
>> >> only provides framework, it doesn't provide the module itself, the
>> >> module
>> >> could be HW dependent.
>> >
>> > Will the code that you plan to call ecryptfs_register_to_events() be
>> > upstream? If so, have you posted it?
>> >
>> > Tyler
>> >
>> >> Regarding the mounting option, it merely serves as example of new
>> cipher
>> >> mode that can be served by registered module.
>> >> There is a special callback function that should be implemented by
>> the
>> >> registered module that 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

> On Sun, Nov 08, 2015 at 10:10:00AM +0200, Andrey Markovytch wrote:
>> +++ b/fs/ecryptfs/caches_utils.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>
> Really?  This looks like copy and paste from core code that defintively
> was not written by the Linux Foundation.  In addition this patch comes
> from Qualcomm so something very fishy is going on here, which if I'd
> call copyrght fraud if I'd want to be be mean.
>
> Please a) stop pointlessly copy and pasting code and b) have a word with
> your lawyers on how to attribute code both that your wrote and which has
> been copy and pasted.
>

Hi Christoph,
Regarding the license, after double checking this, it seems that there has
been a mistake and we unproperly attributed Linux copyright for the code
that actually came from open source files having a different license. I
appologise for that, I am checking this internally and will update patch
with proper license.
As for the 'pointless copy paste', the code was taken from files where
those functions are declared as internal static, we had no choise but to
copy paste them, other option would be alternating the original code and
make those 2 functions public which seems unjustified in this case.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

> On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
>> This is a hardware inline accelerator, meaning that it operates on much
>> lower layer, block layer and device driver layer. The HW encrypts plain
>> requests sent from block layer directly, thus doing it much more
>> efficiently rather than using crypto API.
>

> I feel like basing this on eCryptfs is an odd choice. The overhead of
> setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
> duplication of page cache and dentry cache entries (for upper and lower
> caches) seems like considerable baggage for such a nifty, new hardware
> feature.
>

First of all, one of the leading companies on the mobile market uses
eCryptfs as a solution for their file-based-encryption feature and they
use it along with HW crypto engine. I believe we will soon see interest
from additional companies.

Secondly, eCryptfs is convenient in many ways for implementing
file-based-encryption and there are not so many good alternatives as of
today. You are right regarding overhead, page duplication, etc., however
enabling eCryptfs to work with HW encryption still makes it a very
efficient solution. Also, it is not just inline HW crypto engine, any HW
crypto engine operates more efficiently while working on long chunks of
data. Our suggested solution uses the existing block layer (that is part
of the standard Linux storage stack) feature that gathers number of block
to into one request and then encrypt/decrypt the data, whereas eCryptfs
can only perform the crypto operation on single pages. This feature was
tested on our platforms and demonstrated very significance performance
improvement comparing to existing SW based solutions

>> In order to use such HW efficiently with eCryptfs, eCryptfs encryption
>> has
>> to be canceled and it will need to call for external module instead that
>> will do the correct marking for the blocks to distinguish between those
>> that need to be encrypted by the HW from those that do not need.
>>
>> We are considering posting the code that will call
>> ecryptfs_register_to_events() as a separate patch, but haven't done so
>> yet. It is HW specific.
>> Currently we are only proposing framework change so that it can allow
>> for
>> external modules to be connected
>
> In that case, I cannot accept this patch. I will have no way to test the
> external module functionality and will not be able to make changes to
> that area of the code because it will not be possible for me to fix
> anything that I've broken. I won't even be able to know if I've broken
> anything.
>
> If you are able to upstream the external module code, then I'll do a
> proper review of this patch. I should note that I've only glanced at the
> patch but if feels like it should be broken up into smaller, easier to
> review patches before it can be properly reviewed.
>
> Tyler

We can upstream the external module code, however as I mentioned this
module is specific for our HW, so you won't be able to test it either.
However we can pretty easily turn it into some dummy module for testing
purposes only that will just do prints instead of HW specific calls. Would
this be sufficient ? If so, where in project tree should I put it ?

>
>>
>> > On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
>> >> Hello, Tyler
>> >>
>> >> I'll try to provide more detailed explanation, should it be
>> satisfactory
>> >> enough I will update the patch description.
>> >>
>> >> The problem with current eCryptfs is that it has total control on how
>> >> and
>> >> when the encryption is performed and this control can't be altered.
>> One
>> >> example when this can be a problem is when we want to utilize an
>> >> underlying inline HW encryption engine which allows encrypting blocks
>> >> 'on
>> >> the fly' as they are being written to the storage. In such a case
>> >> relevant
>> >> blocks just need to be marked as 'should be encrypted'. No actual
>> >> encryption should be done by eCryptfs as it will be much slower.
>> >
>> > Is this a hardware crypto accelerator? If so, why not create a crypto
>> > api driver so all subsystems can take advantage of the acceleration
>> > instead of baking support into individual subsystems?
>> >
>> >> The provided framework allows transferring this control (if needed)
>> to
>> >> some external module which will do the encryption itfelf or just mark
>> >> the
>> >> appropriate blocks.
>> >>
>> >> There is no caller for ecryptfs_register_to_events() since this
>> change
>> >> only provides framework, it doesn't provide the module itself, the
>> >> module
>> >> could be HW dependent.
>> >
>> > Will the code that you plan to call ecryptfs_register_to_events() be
>> > upstream? If so, have you posted it?
>> >
>> > Tyler
>> >
>> >> Regarding the mounting option, it merely serves as example of new
>> cipher
>> >> mode that can be served by registered module.
>> >> There is a special callback function that should be implemented by
>> the
>> >> registered module that 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Michael Halcrow]

On Wed, Nov 11, 2015 at 12:03:35PM -, andr...@codeaurora.org wrote:
> > On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
> >> This is a hardware inline accelerator, meaning that it operates on much
> >> lower layer, block layer and device driver layer. The HW encrypts plain
> >> requests sent from block layer directly, thus doing it much more
> >> efficiently rather than using crypto API.
> >
> 
> > I feel like basing this on eCryptfs is an odd choice. The overhead of
> > setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
> > duplication of page cache and dentry cache entries (for upper and lower
> > caches) seems like considerable baggage for such a nifty, new hardware
> > feature.
> >
> 
> First of all, one of the leading companies on the mobile market uses
> eCryptfs as a solution for their file-based-encryption feature and they
> use it along with HW crypto engine. I believe we will soon see interest
> from additional companies.
> 
> Secondly, eCryptfs is convenient in many ways for implementing
> file-based-encryption and there are not so many good alternatives as of
> today.

EXT4 and F2FS both have native file-based encryption capabilities.
They don't suffer from a lot of the overhead and page ballooning
issues that eCryptfs has. Nobody should be putting development effort
toward using eCryptfs on top of EXT4 or F2FS.

There's clear value in enabling inline hardware encryption on mobile
platforms. However I don't think trying to wedge it in via a stacked
file system is a good long-term strategy. We should be moving toward
integrating inline hardware encryption into file systems that are
deployed in those environments.

Given the amount of code duplication between EXT4 and F2FS encryption,
we should also be thinking about more general support for encryption
at the VFS/MM layer.

> You are right regarding overhead, page duplication, etc., however
> enabling eCryptfs to work with HW encryption still makes it a very
> efficient solution. Also, it is not just inline HW crypto engine,
> any HW crypto engine operates more efficiently while working on long
> chunks of data. Our suggested solution uses the existing block layer
> (that is part of the standard Linux storage stack) feature that
> gathers number of block to into one request and then encrypt/decrypt
> the data, whereas eCryptfs can only perform the crypto operation on
> single pages. This feature was tested on our platforms and
> demonstrated very significance performance improvement comparing to
> existing SW based solutions
> 
> >> In order to use such HW efficiently with eCryptfs, eCryptfs encryption
> >> has
> >> to be canceled and it will need to call for external module instead that
> >> will do the correct marking for the blocks to distinguish between those
> >> that need to be encrypted by the HW from those that do not need.
> >>
> >> We are considering posting the code that will call
> >> ecryptfs_register_to_events() as a separate patch, but haven't done so
> >> yet. It is HW specific.
> >> Currently we are only proposing framework change so that it can allow
> >> for
> >> external modules to be connected
> >
> > In that case, I cannot accept this patch. I will have no way to test the
> > external module functionality and will not be able to make changes to
> > that area of the code because it will not be possible for me to fix
> > anything that I've broken. I won't even be able to know if I've broken
> > anything.
> >
> > If you are able to upstream the external module code, then I'll do a
> > proper review of this patch. I should note that I've only glanced at the
> > patch but if feels like it should be broken up into smaller, easier to
> > review patches before it can be properly reviewed.
> >
> > Tyler
> 
> We can upstream the external module code, however as I mentioned this
> module is specific for our HW, so you won't be able to test it either.
> However we can pretty easily turn it into some dummy module for testing
> purposes only that will just do prints instead of HW specific calls. Would
> this be sufficient ? If so, where in project tree should I put it ?
> 
> >
> >>
> >> > On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
> >> >> Hello, Tyler
> >> >>
> >> >> I'll try to provide more detailed explanation, should it be
> >> satisfactory
> >> >> enough I will update the patch description.
> >> >>
> >> >> The problem with current eCryptfs is that it has total control on how
> >> >> and
> >> >> when the encryption is performed and this control can't be altered.
> >> One
> >> >> example when this can be a problem is when we want to utilize an
> >> >> underlying inline HW encryption engine which allows encrypting blocks
> >> >> 'on
> >> >> the fly' as they are being written to the storage. In such a case
> >> >> relevant
> >> >> blocks just need to be marked as 'should be encrypted'. No actual
> >> >> encryption should be done by eCryptfs as it will be much slower.
> >> >
> >> > Is this a hardware 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Tyler Hicks]

On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
> This is a hardware inline accelerator, meaning that it operates on much
> lower layer, block layer and device driver layer. The HW encrypts plain
> requests sent from block layer directly, thus doing it much more
> efficiently rather than using crypto API.

I feel like basing this on eCryptfs is an odd choice. The overhead of
setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
duplication of page cache and dentry cache entries (for upper and lower
caches) seems like considerable baggage for such a nifty, new hardware
feature.

> In order to use such HW efficiently with eCryptfs, eCryptfs encryption has
> to be canceled and it will need to call for external module instead that
> will do the correct marking for the blocks to distinguish between those
> that need to be encrypted by the HW from those that do not need.
> 
> We are considering posting the code that will call
> ecryptfs_register_to_events() as a separate patch, but haven't done so
> yet. It is HW specific.
> Currently we are only proposing framework change so that it can allow for
> external modules to be connected

In that case, I cannot accept this patch. I will have no way to test the
external module functionality and will not be able to make changes to
that area of the code because it will not be possible for me to fix
anything that I've broken. I won't even be able to know if I've broken
anything.

If you are able to upstream the external module code, then I'll do a
proper review of this patch. I should note that I've only glanced at the
patch but if feels like it should be broken up into smaller, easier to
review patches before it can be properly reviewed.

Tyler

> 
> > On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
> >> Hello, Tyler
> >>
> >> I'll try to provide more detailed explanation, should it be satisfactory
> >> enough I will update the patch description.
> >>
> >> The problem with current eCryptfs is that it has total control on how
> >> and
> >> when the encryption is performed and this control can't be altered. One
> >> example when this can be a problem is when we want to utilize an
> >> underlying inline HW encryption engine which allows encrypting blocks
> >> 'on
> >> the fly' as they are being written to the storage. In such a case
> >> relevant
> >> blocks just need to be marked as 'should be encrypted'. No actual
> >> encryption should be done by eCryptfs as it will be much slower.
> >
> > Is this a hardware crypto accelerator? If so, why not create a crypto
> > api driver so all subsystems can take advantage of the acceleration
> > instead of baking support into individual subsystems?
> >
> >> The provided framework allows transferring this control (if needed) to
> >> some external module which will do the encryption itfelf or just mark
> >> the
> >> appropriate blocks.
> >>
> >> There is no caller for ecryptfs_register_to_events() since this change
> >> only provides framework, it doesn't provide the module itself, the
> >> module
> >> could be HW dependent.
> >
> > Will the code that you plan to call ecryptfs_register_to_events() be
> > upstream? If so, have you posted it?
> >
> > Tyler
> >
> >> Regarding the mounting option, it merely serves as example of new cipher
> >> mode that can be served by registered module.
> >> There is a special callback function that should be implemented by the
> >> registered module that tells whether a particular cipher is supported by
> >> it :
> >> is_cipher_supported_cb()
> >>
> >> The mounting option itself is not necessary, I can remove it
> >>
> >> > Hello Andrey!
> >> >
> >> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> >> >> From: Andrey Markovytch 
> >> >>
> >> >> Currently eCryptfs is responsible for page encryption/decryption.
> >> >> This approach will not work when there is HW inline encryption.
> >> >> The proposed change allows external module to register with eCryptfs
> >> >> and provide alternative encryption mechanism and also decide whether
> >> >> encryption should be performed at all, or deferred to a later stage
> >> via
> >> >> the inline HW engine.
> >> >> Additional cipher option was introduced to support the HW/external
> >> mode
> >> >> under that name of "aes-xts". If no external module has registered
> >> >> to support this cipher, eCryptfs will fall back to the usual "aes"
> >> >
> >> > What is "HW/external mode"? There's no description in the commit
> >> message
> >> > and ecryptfs_register_to_events() does not have a caller so I'm not
> >> sure
> >> > of the purpose. Please provide more context.
> >> >
> >> > Despite not yet understanding the purpose of this patch, I think that
> >> I
> >> > can safely say that "aes-xts" is not an appropriate mount option to
> >> use
> >> > when enabling this mode. eCryptfs may support XTS mode one day, using
> >> > the Crypto API, so "HW/external mode" should not own the mount option.
> >> >
> >> > Tyler
> >> >
> >> >>
> >> >> 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

This is a hardware inline accelerator, meaning that it operates on much
lower layer, block layer and device driver layer. The HW encrypts plain
requests sent from block layer directly, thus doing it much more
efficiently rather than using crypto API.
In order to use such HW efficiently with eCryptfs, eCryptfs encryption has
to be canceled and it will need to call for external module instead that
will do the correct marking for the blocks to distinguish between those
that need to be encrypted by the HW from those that do not need.

We are considering posting the code that will call
ecryptfs_register_to_events() as a separate patch, but haven't done so
yet. It is HW specific.
Currently we are only proposing framework change so that it can allow for
external modules to be connected

> On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
>> Hello, Tyler
>>
>> I'll try to provide more detailed explanation, should it be satisfactory
>> enough I will update the patch description.
>>
>> The problem with current eCryptfs is that it has total control on how
>> and
>> when the encryption is performed and this control can't be altered. One
>> example when this can be a problem is when we want to utilize an
>> underlying inline HW encryption engine which allows encrypting blocks
>> 'on
>> the fly' as they are being written to the storage. In such a case
>> relevant
>> blocks just need to be marked as 'should be encrypted'. No actual
>> encryption should be done by eCryptfs as it will be much slower.
>
> Is this a hardware crypto accelerator? If so, why not create a crypto
> api driver so all subsystems can take advantage of the acceleration
> instead of baking support into individual subsystems?
>
>> The provided framework allows transferring this control (if needed) to
>> some external module which will do the encryption itfelf or just mark
>> the
>> appropriate blocks.
>>
>> There is no caller for ecryptfs_register_to_events() since this change
>> only provides framework, it doesn't provide the module itself, the
>> module
>> could be HW dependent.
>
> Will the code that you plan to call ecryptfs_register_to_events() be
> upstream? If so, have you posted it?
>
> Tyler
>
>> Regarding the mounting option, it merely serves as example of new cipher
>> mode that can be served by registered module.
>> There is a special callback function that should be implemented by the
>> registered module that tells whether a particular cipher is supported by
>> it :
>> is_cipher_supported_cb()
>>
>> The mounting option itself is not necessary, I can remove it
>>
>> > Hello Andrey!
>> >
>> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
>> >> From: Andrey Markovytch 
>> >>
>> >> Currently eCryptfs is responsible for page encryption/decryption.
>> >> This approach will not work when there is HW inline encryption.
>> >> The proposed change allows external module to register with eCryptfs
>> >> and provide alternative encryption mechanism and also decide whether
>> >> encryption should be performed at all, or deferred to a later stage
>> via
>> >> the inline HW engine.
>> >> Additional cipher option was introduced to support the HW/external
>> mode
>> >> under that name of "aes-xts". If no external module has registered
>> >> to support this cipher, eCryptfs will fall back to the usual "aes"
>> >
>> > What is "HW/external mode"? There's no description in the commit
>> message
>> > and ecryptfs_register_to_events() does not have a caller so I'm not
>> sure
>> > of the purpose. Please provide more context.
>> >
>> > Despite not yet understanding the purpose of this patch, I think that
>> I
>> > can safely say that "aes-xts" is not an appropriate mount option to
>> use
>> > when enabling this mode. eCryptfs may support XTS mode one day, using
>> > the Crypto API, so "HW/external mode" should not own the mount option.
>> >
>> > Tyler
>> >
>> >>
>> >> Signed-off-by: Lina Zarivach 
>> >> Signed-off-by: Andrey Markovytch 
>> >> ---
>> >>  fs/ecryptfs/Makefile  |   4 +-
>> >>  fs/ecryptfs/caches_utils.c|  78 +
>> >>  fs/ecryptfs/crypto.c  | 200 +++
>> >>  fs/ecryptfs/debug.c   |  13 ++
>> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>> >>  fs/ecryptfs/events.c  | 361
>> >> ++
>> >>  fs/ecryptfs/file.c|  36 +
>> >>  fs/ecryptfs/inode.c   |  11 ++
>> >>  fs/ecryptfs/keystore.c| 101 
>> >>  fs/ecryptfs/main.c|  60 +--
>> >>  fs/ecryptfs/mmap.c|   6 +
>> >>  fs/ecryptfs/super.c   |  14 +-
>> >>  include/linux/ecryptfs.h  |  47 ++
>> >>  13 files changed, 940 insertions(+), 69 deletions(-)
>> >>  create mode 100644 fs/ecryptfs/caches_utils.c
>> >>  create mode 100644 fs/ecryptfs/events.c
>> >>
>> >> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
>> >> index 49678a6..995719c 100644
>> >> --- a/fs/ecryptfs/Makefile
>> >> +++ 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hi Michael,

First of all, the original mechanism is still there and is used by
default, unless someone registers for external module and then indeed the
encryption is replaced for ciphers that this module decides to support.
Currently the only suggestion is to extend the framework that will allow
such modules to be added in the future and of course each such module will
have to be audited and follow all the requirements that you mentioned.
As on what this module is supposed to do, please see my responses to Tyler.

> This is a hardware inline accelerator, meaning that it operates on much
> lower layer, block layer and device driver layer. The HW encrypts plain
> requests sent from block layer directly, thus doing it much more
> efficiently rather than using crypto API.
> In order to use such HW efficiently with eCryptfs, eCryptfs encryption has
> to be canceled and it will need to call for external module instead that
> will do the correct marking for the blocks to distinguish between those
> that need to be encrypted by the HW from those that do not need.
>
> We are considering posting the code that will call
> ecryptfs_register_to_events() as a separate patch, but haven't done so
> yet. It is HW specific.
> Currently we are only proposing framework change so that it can allow for
> external modules to be connected
>
>> On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
>>> Hello, Tyler
>>>
>>> I'll try to provide more detailed explanation, should it be
>>> satisfactory
>>> enough I will update the patch description.
>>>
>>> The problem with current eCryptfs is that it has total control on how
>>> and
>>> when the encryption is performed and this control can't be altered. One
>>> example when this can be a problem is when we want to utilize an
>>> underlying inline HW encryption engine which allows encrypting blocks
>>> 'on
>>> the fly' as they are being written to the storage. In such a case
>>> relevant
>>> blocks just need to be marked as 'should be encrypted'. No actual
>>> encryption should be done by eCryptfs as it will be much slower.
>>
>> Is this a hardware crypto accelerator? If so, why not create a crypto
>> api driver so all subsystems can take advantage of the acceleration
>> instead of baking support into individual subsystems?
>>
>>> The provided framework allows transferring this control (if needed) to
>>> some external module which will do the encryption itfelf or just mark
>>> the
>>> appropriate blocks.
>>>
>>> There is no caller for ecryptfs_register_to_events() since this change
>>> only provides framework, it doesn't provide the module itself, the
>>> module
>>> could be HW dependent.
>>
>> Will the code that you plan to call ecryptfs_register_to_events() be
>> upstream? If so, have you posted it?
>>
>> Tyler
>>
>>> Regarding the mounting option, it merely serves as example of new
>>> cipher
>>> mode that can be served by registered module.
>>> There is a special callback function that should be implemented by the
>>> registered module that tells whether a particular cipher is supported
>>> by
>>> it :
>>> is_cipher_supported_cb()
>>>
>>> The mounting option itself is not necessary, I can remove it
>>>
>>> > Hello Andrey!
>>> >
>>> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
>>> >> From: Andrey Markovytch 
>>> >>
>>> >> Currently eCryptfs is responsible for page encryption/decryption.
>>> >> This approach will not work when there is HW inline encryption.
>>> >> The proposed change allows external module to register with eCryptfs
>>> >> and provide alternative encryption mechanism and also decide whether
>>> >> encryption should be performed at all, or deferred to a later stage
>>> via
>>> >> the inline HW engine.
>>> >> Additional cipher option was introduced to support the HW/external
>>> mode
>>> >> under that name of "aes-xts". If no external module has registered
>>> >> to support this cipher, eCryptfs will fall back to the usual "aes"
>>> >
>>> > What is "HW/external mode"? There's no description in the commit
>>> message
>>> > and ecryptfs_register_to_events() does not have a caller so I'm not
>>> sure
>>> > of the purpose. Please provide more context.
>>> >
>>> > Despite not yet understanding the purpose of this patch, I think that
>>> I
>>> > can safely say that "aes-xts" is not an appropriate mount option to
>>> use
>>> > when enabling this mode. eCryptfs may support XTS mode one day, using
>>> > the Crypto API, so "HW/external mode" should not own the mount
>>> option.
>>> >
>>> > Tyler
>>> >
>>> >>
>>> >> Signed-off-by: Lina Zarivach 
>>> >> Signed-off-by: Andrey Markovytch 
>>> >> ---
>>> >>  fs/ecryptfs/Makefile  |   4 +-
>>> >>  fs/ecryptfs/caches_utils.c|  78 +
>>> >>  fs/ecryptfs/crypto.c  | 200 +++
>>> >>  fs/ecryptfs/debug.c   |  13 ++
>>> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>>> >>  fs/ecryptfs/events.c  | 361
>>> >> ++
>>> >>  

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hi Michael,

First of all, the original mechanism is still there and is used by
default, unless someone registers for external module and then indeed the
encryption is replaced for ciphers that this module decides to support.
Currently the only suggestion is to extend the framework that will allow
such modules to be added in the future and of course each such module will
have to be audited and follow all the requirements that you mentioned.
As on what this module is supposed to do, please see my responses to Tyler.

> This is a hardware inline accelerator, meaning that it operates on much
> lower layer, block layer and device driver layer. The HW encrypts plain
> requests sent from block layer directly, thus doing it much more
> efficiently rather than using crypto API.
> In order to use such HW efficiently with eCryptfs, eCryptfs encryption has
> to be canceled and it will need to call for external module instead that
> will do the correct marking for the blocks to distinguish between those
> that need to be encrypted by the HW from those that do not need.
>
> We are considering posting the code that will call
> ecryptfs_register_to_events() as a separate patch, but haven't done so
> yet. It is HW specific.
> Currently we are only proposing framework change so that it can allow for
> external modules to be connected
>
>> On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
>>> Hello, Tyler
>>>
>>> I'll try to provide more detailed explanation, should it be
>>> satisfactory
>>> enough I will update the patch description.
>>>
>>> The problem with current eCryptfs is that it has total control on how
>>> and
>>> when the encryption is performed and this control can't be altered. One
>>> example when this can be a problem is when we want to utilize an
>>> underlying inline HW encryption engine which allows encrypting blocks
>>> 'on
>>> the fly' as they are being written to the storage. In such a case
>>> relevant
>>> blocks just need to be marked as 'should be encrypted'. No actual
>>> encryption should be done by eCryptfs as it will be much slower.
>>
>> Is this a hardware crypto accelerator? If so, why not create a crypto
>> api driver so all subsystems can take advantage of the acceleration
>> instead of baking support into individual subsystems?
>>
>>> The provided framework allows transferring this control (if needed) to
>>> some external module which will do the encryption itfelf or just mark
>>> the
>>> appropriate blocks.
>>>
>>> There is no caller for ecryptfs_register_to_events() since this change
>>> only provides framework, it doesn't provide the module itself, the
>>> module
>>> could be HW dependent.
>>
>> Will the code that you plan to call ecryptfs_register_to_events() be
>> upstream? If so, have you posted it?
>>
>> Tyler
>>
>>> Regarding the mounting option, it merely serves as example of new
>>> cipher
>>> mode that can be served by registered module.
>>> There is a special callback function that should be implemented by the
>>> registered module that tells whether a particular cipher is supported
>>> by
>>> it :
>>> is_cipher_supported_cb()
>>>
>>> The mounting option itself is not necessary, I can remove it
>>>
>>> > Hello Andrey!
>>> >
>>> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
>>> >> From: Andrey Markovytch 
>>> >>
>>> >> Currently eCryptfs is responsible for page encryption/decryption.
>>> >> This approach will not work when there is HW inline encryption.
>>> >> The proposed change allows external module to register with eCryptfs
>>> >> and provide alternative encryption mechanism and also decide whether
>>> >> encryption should be performed at all, or deferred to a later stage
>>> via
>>> >> the inline HW engine.
>>> >> Additional cipher option was introduced to support the HW/external
>>> mode
>>> >> under that name of "aes-xts". If no external module has registered
>>> >> to support this cipher, eCryptfs will fall back to the usual "aes"
>>> >
>>> > What is "HW/external mode"? There's no description in the commit
>>> message
>>> > and ecryptfs_register_to_events() does not have a caller so I'm not
>>> sure
>>> > of the purpose. Please provide more context.
>>> >
>>> > Despite not yet understanding the purpose of this patch, I think that
>>> I
>>> > can safely say that "aes-xts" is not an appropriate mount option to
>>> use
>>> > when enabling this mode. eCryptfs may support XTS mode one day, using
>>> > the Crypto API, so "HW/external mode" should not own the mount
>>> option.
>>> >
>>> > Tyler
>>> >
>>> >>
>>> >> Signed-off-by: Lina Zarivach 
>>> >> Signed-off-by: Andrey Markovytch 
>>> >> ---
>>> >>  fs/ecryptfs/Makefile  |   4 +-
>>> >>  fs/ecryptfs/caches_utils.c|  78 +
>>> >>  fs/ecryptfs/crypto.c  | 200 +++
>>> >>  fs/ecryptfs/debug.c   |  13 ++
>>> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>>> >>  fs/ecryptfs/events.c  | 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

This is a hardware inline accelerator, meaning that it operates on much
lower layer, block layer and device driver layer. The HW encrypts plain
requests sent from block layer directly, thus doing it much more
efficiently rather than using crypto API.
In order to use such HW efficiently with eCryptfs, eCryptfs encryption has
to be canceled and it will need to call for external module instead that
will do the correct marking for the blocks to distinguish between those
that need to be encrypted by the HW from those that do not need.

We are considering posting the code that will call
ecryptfs_register_to_events() as a separate patch, but haven't done so
yet. It is HW specific.
Currently we are only proposing framework change so that it can allow for
external modules to be connected

> On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
>> Hello, Tyler
>>
>> I'll try to provide more detailed explanation, should it be satisfactory
>> enough I will update the patch description.
>>
>> The problem with current eCryptfs is that it has total control on how
>> and
>> when the encryption is performed and this control can't be altered. One
>> example when this can be a problem is when we want to utilize an
>> underlying inline HW encryption engine which allows encrypting blocks
>> 'on
>> the fly' as they are being written to the storage. In such a case
>> relevant
>> blocks just need to be marked as 'should be encrypted'. No actual
>> encryption should be done by eCryptfs as it will be much slower.
>
> Is this a hardware crypto accelerator? If so, why not create a crypto
> api driver so all subsystems can take advantage of the acceleration
> instead of baking support into individual subsystems?
>
>> The provided framework allows transferring this control (if needed) to
>> some external module which will do the encryption itfelf or just mark
>> the
>> appropriate blocks.
>>
>> There is no caller for ecryptfs_register_to_events() since this change
>> only provides framework, it doesn't provide the module itself, the
>> module
>> could be HW dependent.
>
> Will the code that you plan to call ecryptfs_register_to_events() be
> upstream? If so, have you posted it?
>
> Tyler
>
>> Regarding the mounting option, it merely serves as example of new cipher
>> mode that can be served by registered module.
>> There is a special callback function that should be implemented by the
>> registered module that tells whether a particular cipher is supported by
>> it :
>> is_cipher_supported_cb()
>>
>> The mounting option itself is not necessary, I can remove it
>>
>> > Hello Andrey!
>> >
>> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
>> >> From: Andrey Markovytch 
>> >>
>> >> Currently eCryptfs is responsible for page encryption/decryption.
>> >> This approach will not work when there is HW inline encryption.
>> >> The proposed change allows external module to register with eCryptfs
>> >> and provide alternative encryption mechanism and also decide whether
>> >> encryption should be performed at all, or deferred to a later stage
>> via
>> >> the inline HW engine.
>> >> Additional cipher option was introduced to support the HW/external
>> mode
>> >> under that name of "aes-xts". If no external module has registered
>> >> to support this cipher, eCryptfs will fall back to the usual "aes"
>> >
>> > What is "HW/external mode"? There's no description in the commit
>> message
>> > and ecryptfs_register_to_events() does not have a caller so I'm not
>> sure
>> > of the purpose. Please provide more context.
>> >
>> > Despite not yet understanding the purpose of this patch, I think that
>> I
>> > can safely say that "aes-xts" is not an appropriate mount option to
>> use
>> > when enabling this mode. eCryptfs may support XTS mode one day, using
>> > the Crypto API, so "HW/external mode" should not own the mount option.
>> >
>> > Tyler
>> >
>> >>
>> >> Signed-off-by: Lina Zarivach 
>> >> Signed-off-by: Andrey Markovytch 
>> >> ---
>> >>  fs/ecryptfs/Makefile  |   4 +-
>> >>  fs/ecryptfs/caches_utils.c|  78 +
>> >>  fs/ecryptfs/crypto.c  | 200 +++
>> >>  fs/ecryptfs/debug.c   |  13 ++
>> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>> >>  fs/ecryptfs/events.c  | 361
>> >> ++
>> >>  fs/ecryptfs/file.c|  36 +
>> >>  fs/ecryptfs/inode.c   |  11 ++
>> >>  fs/ecryptfs/keystore.c| 101 
>> >>  fs/ecryptfs/main.c|  60 +--
>> >>  fs/ecryptfs/mmap.c|   6 +
>> >>  fs/ecryptfs/super.c   |  14 +-
>> >>  include/linux/ecryptfs.h  |  47 ++
>> >>  13 files changed, 940 insertions(+), 69 deletions(-)
>> >>  create mode 100644 fs/ecryptfs/caches_utils.c
>> >>  create mode 100644 fs/ecryptfs/events.c
>> >>
>> >> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
>> >> index 49678a6..995719c 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Tyler Hicks]

On 2015-11-10 15:20:59, andr...@codeaurora.org wrote:
> This is a hardware inline accelerator, meaning that it operates on much
> lower layer, block layer and device driver layer. The HW encrypts plain
> requests sent from block layer directly, thus doing it much more
> efficiently rather than using crypto API.

I feel like basing this on eCryptfs is an odd choice. The overhead of
setting up an eCryptfs mount (and requiring CAP_SYS_ADMIN) and the
duplication of page cache and dentry cache entries (for upper and lower
caches) seems like considerable baggage for such a nifty, new hardware
feature.

> In order to use such HW efficiently with eCryptfs, eCryptfs encryption has
> to be canceled and it will need to call for external module instead that
> will do the correct marking for the blocks to distinguish between those
> that need to be encrypted by the HW from those that do not need.
> 
> We are considering posting the code that will call
> ecryptfs_register_to_events() as a separate patch, but haven't done so
> yet. It is HW specific.
> Currently we are only proposing framework change so that it can allow for
> external modules to be connected

In that case, I cannot accept this patch. I will have no way to test the
external module functionality and will not be able to make changes to
that area of the code because it will not be possible for me to fix
anything that I've broken. I won't even be able to know if I've broken
anything.

If you are able to upstream the external module code, then I'll do a
proper review of this patch. I should note that I've only glanced at the
patch but if feels like it should be broken up into smaller, easier to
review patches before it can be properly reviewed.

Tyler

> 
> > On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
> >> Hello, Tyler
> >>
> >> I'll try to provide more detailed explanation, should it be satisfactory
> >> enough I will update the patch description.
> >>
> >> The problem with current eCryptfs is that it has total control on how
> >> and
> >> when the encryption is performed and this control can't be altered. One
> >> example when this can be a problem is when we want to utilize an
> >> underlying inline HW encryption engine which allows encrypting blocks
> >> 'on
> >> the fly' as they are being written to the storage. In such a case
> >> relevant
> >> blocks just need to be marked as 'should be encrypted'. No actual
> >> encryption should be done by eCryptfs as it will be much slower.
> >
> > Is this a hardware crypto accelerator? If so, why not create a crypto
> > api driver so all subsystems can take advantage of the acceleration
> > instead of baking support into individual subsystems?
> >
> >> The provided framework allows transferring this control (if needed) to
> >> some external module which will do the encryption itfelf or just mark
> >> the
> >> appropriate blocks.
> >>
> >> There is no caller for ecryptfs_register_to_events() since this change
> >> only provides framework, it doesn't provide the module itself, the
> >> module
> >> could be HW dependent.
> >
> > Will the code that you plan to call ecryptfs_register_to_events() be
> > upstream? If so, have you posted it?
> >
> > Tyler
> >
> >> Regarding the mounting option, it merely serves as example of new cipher
> >> mode that can be served by registered module.
> >> There is a special callback function that should be implemented by the
> >> registered module that tells whether a particular cipher is supported by
> >> it :
> >> is_cipher_supported_cb()
> >>
> >> The mounting option itself is not necessary, I can remove it
> >>
> >> > Hello Andrey!
> >> >
> >> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> >> >> From: Andrey Markovytch 
> >> >>
> >> >> Currently eCryptfs is responsible for page encryption/decryption.
> >> >> This approach will not work when there is HW inline encryption.
> >> >> The proposed change allows external module to register with eCryptfs
> >> >> and provide alternative encryption mechanism and also decide whether
> >> >> encryption should be performed at all, or deferred to a later stage
> >> via
> >> >> the inline HW engine.
> >> >> Additional cipher option was introduced to support the HW/external
> >> mode
> >> >> under that name of "aes-xts". If no external module has registered
> >> >> to support this cipher, eCryptfs will fall back to the usual "aes"
> >> >
> >> > What is "HW/external mode"? There's no description in the commit
> >> message
> >> > and ecryptfs_register_to_events() does not have a caller so I'm not
> >> sure
> >> > of the purpose. Please provide more context.
> >> >
> >> > Despite not yet understanding the purpose of this patch, I think that
> >> I
> >> > can safely say that "aes-xts" is not an appropriate mount option to
> >> use
> >> > when enabling this mode. eCryptfs may support XTS mode one day, using
> >> > the Crypto API, so "HW/external mode" should not own the mount option.
> >> >
> >> > Tyler
> >> >

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Michael Halcrow]

On Mon, Nov 09, 2015 at 08:56:02PM -, andr...@codeaurora.org wrote:
> Hello, Tyler
> 
> I'll try to provide more detailed explanation, should it be satisfactory
> enough I will update the patch description.
> 
> The problem with current eCryptfs is that it has total control on how and
> when the encryption is performed and this control can't be altered.

That's by design.

eCryptfs has received at least one audit in 2014, and the auditor
didn't find any serious security flaws. eCryptfs users have
expectations about its security because of that.

It looks like you are replacing the encryption mechanism with an
out-of-kernel mechanism that implements a different encryption
mode. Users on this platform who use "eCryptfs" are actually getting
something different than what has been reviewed and audited.

On a first glance through the patch, it's not clear to me what this
out-of-kernel encryption mechanism is supposed to do. Can you point to
any design documents? From this patch, I gather that it implements
AES-256-XTS. Does the implementation conform to guidance in NIST
Special Publication 800-38E and IEEE P1619/D16?

> One example when this can be a problem is when we want to utilize an
> underlying inline HW encryption engine which allows encrypting blocks 'on
> the fly' as they are being written to the storage. In such a case relevant
> blocks just need to be marked as 'should be encrypted'. No actual
> encryption should be done by eCryptfs as it will be much slower.

What's the motivation for exposing this functionality through
eCryptfs? If eCryptfs really is the best place for this, then what are
the plans for EXT4 and F2FS, which both have per-file encryption? What
do your usage scenarios look like? What's your adversarial model?

> The provided framework allows transferring this control (if needed) to
> some external module which will do the encryption itfelf or just mark the
> appropriate blocks.
> 
> There is no caller for ecryptfs_register_to_events() since this change
> only provides framework, it doesn't provide the module itself, the module
> could be HW dependent.
> 
> Regarding the mounting option, it merely serves as example of new cipher
> mode that can be served by registered module.

If anyone can just easily swap out encryption modes, it's harder to
make any meaningful security statements about eCryptfs. Especially
given that pressure to hit benchmark targets can motivate unsafe
choices.

> There is a special callback function that should be implemented by the
> registered module that tells whether a particular cipher is supported by
> it :
> is_cipher_supported_cb()
> 
> The mounting option itself is not necessary, I can remove it
> 
> > Hello Andrey!
> >
> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> >> From: Andrey Markovytch 
> >>
> >> Currently eCryptfs is responsible for page encryption/decryption.
> >> This approach will not work when there is HW inline encryption.
> >> The proposed change allows external module to register with eCryptfs
> >> and provide alternative encryption mechanism and also decide whether
> >> encryption should be performed at all, or deferred to a later stage via
> >> the inline HW engine.
> >> Additional cipher option was introduced to support the HW/external mode
> >> under that name of "aes-xts". If no external module has registered
> >> to support this cipher, eCryptfs will fall back to the usual "aes"
> >
> > What is "HW/external mode"? There's no description in the commit message
> > and ecryptfs_register_to_events() does not have a caller so I'm not sure
> > of the purpose. Please provide more context.
> >
> > Despite not yet understanding the purpose of this patch, I think that I
> > can safely say that "aes-xts" is not an appropriate mount option to use
> > when enabling this mode. eCryptfs may support XTS mode one day, using
> > the Crypto API, so "HW/external mode" should not own the mount option.
> >
> > Tyler
> >
> >>
> >> Signed-off-by: Lina Zarivach 
> >> Signed-off-by: Andrey Markovytch 
> >> ---
> >>  fs/ecryptfs/Makefile  |   4 +-
> >>  fs/ecryptfs/caches_utils.c|  78 +
> >>  fs/ecryptfs/crypto.c  | 200 +++
> >>  fs/ecryptfs/debug.c   |  13 ++
> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
> >>  fs/ecryptfs/events.c  | 361
> >> ++
> >>  fs/ecryptfs/file.c|  36 +
> >>  fs/ecryptfs/inode.c   |  11 ++
> >>  fs/ecryptfs/keystore.c| 101 
> >>  fs/ecryptfs/main.c|  60 +--
> >>  fs/ecryptfs/mmap.c|   6 +
> >>  fs/ecryptfs/super.c   |  14 +-
> >>  include/linux/ecryptfs.h  |  47 ++
> >>  13 files changed, 940 insertions(+), 69 deletions(-)
> >>  create mode 100644 fs/ecryptfs/caches_utils.c
> >>  create mode 100644 fs/ecryptfs/events.c
> >>
> >> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
> >> index 49678a6..995719c 100644
> >> --- 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Tyler Hicks]

On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
> Hello, Tyler
> 
> I'll try to provide more detailed explanation, should it be satisfactory
> enough I will update the patch description.
> 
> The problem with current eCryptfs is that it has total control on how and
> when the encryption is performed and this control can't be altered. One
> example when this can be a problem is when we want to utilize an
> underlying inline HW encryption engine which allows encrypting blocks 'on
> the fly' as they are being written to the storage. In such a case relevant
> blocks just need to be marked as 'should be encrypted'. No actual
> encryption should be done by eCryptfs as it will be much slower.

Is this a hardware crypto accelerator? If so, why not create a crypto
api driver so all subsystems can take advantage of the acceleration
instead of baking support into individual subsystems?

> The provided framework allows transferring this control (if needed) to
> some external module which will do the encryption itfelf or just mark the
> appropriate blocks.
> 
> There is no caller for ecryptfs_register_to_events() since this change
> only provides framework, it doesn't provide the module itself, the module
> could be HW dependent.

Will the code that you plan to call ecryptfs_register_to_events() be
upstream? If so, have you posted it?

Tyler

> Regarding the mounting option, it merely serves as example of new cipher
> mode that can be served by registered module.
> There is a special callback function that should be implemented by the
> registered module that tells whether a particular cipher is supported by
> it :
> is_cipher_supported_cb()
> 
> The mounting option itself is not necessary, I can remove it
> 
> > Hello Andrey!
> >
> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> >> From: Andrey Markovytch 
> >>
> >> Currently eCryptfs is responsible for page encryption/decryption.
> >> This approach will not work when there is HW inline encryption.
> >> The proposed change allows external module to register with eCryptfs
> >> and provide alternative encryption mechanism and also decide whether
> >> encryption should be performed at all, or deferred to a later stage via
> >> the inline HW engine.
> >> Additional cipher option was introduced to support the HW/external mode
> >> under that name of "aes-xts". If no external module has registered
> >> to support this cipher, eCryptfs will fall back to the usual "aes"
> >
> > What is "HW/external mode"? There's no description in the commit message
> > and ecryptfs_register_to_events() does not have a caller so I'm not sure
> > of the purpose. Please provide more context.
> >
> > Despite not yet understanding the purpose of this patch, I think that I
> > can safely say that "aes-xts" is not an appropriate mount option to use
> > when enabling this mode. eCryptfs may support XTS mode one day, using
> > the Crypto API, so "HW/external mode" should not own the mount option.
> >
> > Tyler
> >
> >>
> >> Signed-off-by: Lina Zarivach 
> >> Signed-off-by: Andrey Markovytch 
> >> ---
> >>  fs/ecryptfs/Makefile  |   4 +-
> >>  fs/ecryptfs/caches_utils.c|  78 +
> >>  fs/ecryptfs/crypto.c  | 200 +++
> >>  fs/ecryptfs/debug.c   |  13 ++
> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
> >>  fs/ecryptfs/events.c  | 361
> >> ++
> >>  fs/ecryptfs/file.c|  36 +
> >>  fs/ecryptfs/inode.c   |  11 ++
> >>  fs/ecryptfs/keystore.c| 101 
> >>  fs/ecryptfs/main.c|  60 +--
> >>  fs/ecryptfs/mmap.c|   6 +
> >>  fs/ecryptfs/super.c   |  14 +-
> >>  include/linux/ecryptfs.h  |  47 ++
> >>  13 files changed, 940 insertions(+), 69 deletions(-)
> >>  create mode 100644 fs/ecryptfs/caches_utils.c
> >>  create mode 100644 fs/ecryptfs/events.c
> >>
> >> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
> >> index 49678a6..995719c 100644
> >> --- a/fs/ecryptfs/Makefile
> >> +++ b/fs/ecryptfs/Makefile
> >> @@ -4,7 +4,7 @@
> >>
> >>  obj-$(CONFIG_ECRYPT_FS) += ecryptfs.o
> >>
> >> -ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
> >> read_write.o \
> >> -crypto.o keystore.o kthread.o debug.o
> >> +ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
> >> read_write.o events.o \
> >> +crypto.o keystore.o kthread.o debug.o caches_utils.o
> >>
> >>  ecryptfs-$(CONFIG_ECRYPT_FS_MESSAGING) += messaging.o miscdev.o
> >> diff --git a/fs/ecryptfs/caches_utils.c b/fs/ecryptfs/caches_utils.c
> >> new file mode 100644
> >> index 000..c599c96
> >> --- /dev/null
> >> +++ b/fs/ecryptfs/caches_utils.c
> >> @@ -0,0 +1,78 @@
> >> +/*
> >> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 and
> >> + * only 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hello, Tyler

I'll try to provide more detailed explanation, should it be satisfactory
enough I will update the patch description.

The problem with current eCryptfs is that it has total control on how and
when the encryption is performed and this control can't be altered. One
example when this can be a problem is when we want to utilize an
underlying inline HW encryption engine which allows encrypting blocks 'on
the fly' as they are being written to the storage. In such a case relevant
blocks just need to be marked as 'should be encrypted'. No actual
encryption should be done by eCryptfs as it will be much slower.

The provided framework allows transferring this control (if needed) to
some external module which will do the encryption itfelf or just mark the
appropriate blocks.

There is no caller for ecryptfs_register_to_events() since this change
only provides framework, it doesn't provide the module itself, the module
could be HW dependent.

Regarding the mounting option, it merely serves as example of new cipher
mode that can be served by registered module.
There is a special callback function that should be implemented by the
registered module that tells whether a particular cipher is supported by
it :
is_cipher_supported_cb()

The mounting option itself is not necessary, I can remove it

> Hello Andrey!
>
> On 2015-11-08 10:10:00, Andrey Markovytch wrote:
>> From: Andrey Markovytch 
>>
>> Currently eCryptfs is responsible for page encryption/decryption.
>> This approach will not work when there is HW inline encryption.
>> The proposed change allows external module to register with eCryptfs
>> and provide alternative encryption mechanism and also decide whether
>> encryption should be performed at all, or deferred to a later stage via
>> the inline HW engine.
>> Additional cipher option was introduced to support the HW/external mode
>> under that name of "aes-xts". If no external module has registered
>> to support this cipher, eCryptfs will fall back to the usual "aes"
>
> What is "HW/external mode"? There's no description in the commit message
> and ecryptfs_register_to_events() does not have a caller so I'm not sure
> of the purpose. Please provide more context.
>
> Despite not yet understanding the purpose of this patch, I think that I
> can safely say that "aes-xts" is not an appropriate mount option to use
> when enabling this mode. eCryptfs may support XTS mode one day, using
> the Crypto API, so "HW/external mode" should not own the mount option.
>
> Tyler
>
>>
>> Signed-off-by: Lina Zarivach 
>> Signed-off-by: Andrey Markovytch 
>> ---
>>  fs/ecryptfs/Makefile  |   4 +-
>>  fs/ecryptfs/caches_utils.c|  78 +
>>  fs/ecryptfs/crypto.c  | 200 +++
>>  fs/ecryptfs/debug.c   |  13 ++
>>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>>  fs/ecryptfs/events.c  | 361
>> ++
>>  fs/ecryptfs/file.c|  36 +
>>  fs/ecryptfs/inode.c   |  11 ++
>>  fs/ecryptfs/keystore.c| 101 
>>  fs/ecryptfs/main.c|  60 +--
>>  fs/ecryptfs/mmap.c|   6 +
>>  fs/ecryptfs/super.c   |  14 +-
>>  include/linux/ecryptfs.h  |  47 ++
>>  13 files changed, 940 insertions(+), 69 deletions(-)
>>  create mode 100644 fs/ecryptfs/caches_utils.c
>>  create mode 100644 fs/ecryptfs/events.c
>>
>> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
>> index 49678a6..995719c 100644
>> --- a/fs/ecryptfs/Makefile
>> +++ b/fs/ecryptfs/Makefile
>> @@ -4,7 +4,7 @@
>>
>>  obj-$(CONFIG_ECRYPT_FS) += ecryptfs.o
>>
>> -ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
>> read_write.o \
>> -  crypto.o keystore.o kthread.o debug.o
>> +ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
>> read_write.o events.o \
>> +  crypto.o keystore.o kthread.o debug.o caches_utils.o
>>
>>  ecryptfs-$(CONFIG_ECRYPT_FS_MESSAGING) += messaging.o miscdev.o
>> diff --git a/fs/ecryptfs/caches_utils.c b/fs/ecryptfs/caches_utils.c
>> new file mode 100644
>> index 000..c599c96
>> --- /dev/null
>> +++ b/fs/ecryptfs/caches_utils.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "../internal.h"
>> +
>> +void ecryptfs_drop_pagecache_sb(struct super_block *sb, void *unused)
>> +{
>> +struct inode *inode, 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Tyler Hicks]

Hello Andrey!

On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> From: Andrey Markovytch 
> 
> Currently eCryptfs is responsible for page encryption/decryption.
> This approach will not work when there is HW inline encryption.
> The proposed change allows external module to register with eCryptfs
> and provide alternative encryption mechanism and also decide whether
> encryption should be performed at all, or deferred to a later stage via
> the inline HW engine.
> Additional cipher option was introduced to support the HW/external mode
> under that name of "aes-xts". If no external module has registered
> to support this cipher, eCryptfs will fall back to the usual "aes"

What is "HW/external mode"? There's no description in the commit message
and ecryptfs_register_to_events() does not have a caller so I'm not sure
of the purpose. Please provide more context.

Despite not yet understanding the purpose of this patch, I think that I
can safely say that "aes-xts" is not an appropriate mount option to use
when enabling this mode. eCryptfs may support XTS mode one day, using
the Crypto API, so "HW/external mode" should not own the mount option.

Tyler

> 
> Signed-off-by: Lina Zarivach 
> Signed-off-by: Andrey Markovytch 
> ---
>  fs/ecryptfs/Makefile  |   4 +-
>  fs/ecryptfs/caches_utils.c|  78 +
>  fs/ecryptfs/crypto.c  | 200 +++
>  fs/ecryptfs/debug.c   |  13 ++
>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>  fs/ecryptfs/events.c  | 361 
> ++
>  fs/ecryptfs/file.c|  36 +
>  fs/ecryptfs/inode.c   |  11 ++
>  fs/ecryptfs/keystore.c| 101 
>  fs/ecryptfs/main.c|  60 +--
>  fs/ecryptfs/mmap.c|   6 +
>  fs/ecryptfs/super.c   |  14 +-
>  include/linux/ecryptfs.h  |  47 ++
>  13 files changed, 940 insertions(+), 69 deletions(-)
>  create mode 100644 fs/ecryptfs/caches_utils.c
>  create mode 100644 fs/ecryptfs/events.c
> 
> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
> index 49678a6..995719c 100644
> --- a/fs/ecryptfs/Makefile
> +++ b/fs/ecryptfs/Makefile
> @@ -4,7 +4,7 @@
>  
>  obj-$(CONFIG_ECRYPT_FS) += ecryptfs.o
>  
> -ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o read_write.o \
> -   crypto.o keystore.o kthread.o debug.o
> +ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o read_write.o 
> events.o \
> +   crypto.o keystore.o kthread.o debug.o caches_utils.o
>  
>  ecryptfs-$(CONFIG_ECRYPT_FS_MESSAGING) += messaging.o miscdev.o
> diff --git a/fs/ecryptfs/caches_utils.c b/fs/ecryptfs/caches_utils.c
> new file mode 100644
> index 000..c599c96
> --- /dev/null
> +++ b/fs/ecryptfs/caches_utils.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "../internal.h"
> +
> +void ecryptfs_drop_pagecache_sb(struct super_block *sb, void *unused)
> +{
> + struct inode *inode, *toput_inode = NULL;
> +
> + spin_lock(>s_inode_list_lock);
> + list_for_each_entry(inode, >s_inodes, i_sb_list) {
> + spin_lock(>i_lock);
> + if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) ||
> + (inode->i_mapping->nrpages == 0)) {
> + spin_unlock(>i_lock);
> + continue;
> + }
> + __iget(inode);
> + spin_unlock(>i_lock);
> + spin_unlock(>s_inode_list_lock);
> +
> + invalidate_mapping_pages(inode->i_mapping, 0, -1);
> + iput(toput_inode);
> + toput_inode = inode;
> +
> + spin_lock(>s_inode_list_lock);
> + }
> + spin_unlock(>s_inode_list_lock);
> + iput(toput_inode);
> +}
> +
> +void clean_inode_pages(struct address_space *mapping,
> + pgoff_t start, pgoff_t end)
> +{
> + struct pagevec pvec;
> + pgoff_t index = start;
> + int i;
> +
> + pagevec_init(, 0);
> + while (index <= end && pagevec_lookup(, mapping, index,
> + min(end - index,
> + (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
> + for (i = 0; i < pagevec_count(); i++) {
> + struct page *page = pvec.pages[i];
> +
> + /* We rely upon deletion
> +   

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Tyler Hicks]

On 2015-11-09 20:56:02, andr...@codeaurora.org wrote:
> Hello, Tyler
> 
> I'll try to provide more detailed explanation, should it be satisfactory
> enough I will update the patch description.
> 
> The problem with current eCryptfs is that it has total control on how and
> when the encryption is performed and this control can't be altered. One
> example when this can be a problem is when we want to utilize an
> underlying inline HW encryption engine which allows encrypting blocks 'on
> the fly' as they are being written to the storage. In such a case relevant
> blocks just need to be marked as 'should be encrypted'. No actual
> encryption should be done by eCryptfs as it will be much slower.

Is this a hardware crypto accelerator? If so, why not create a crypto
api driver so all subsystems can take advantage of the acceleration
instead of baking support into individual subsystems?

> The provided framework allows transferring this control (if needed) to
> some external module which will do the encryption itfelf or just mark the
> appropriate blocks.
> 
> There is no caller for ecryptfs_register_to_events() since this change
> only provides framework, it doesn't provide the module itself, the module
> could be HW dependent.

Will the code that you plan to call ecryptfs_register_to_events() be
upstream? If so, have you posted it?

Tyler

> Regarding the mounting option, it merely serves as example of new cipher
> mode that can be served by registered module.
> There is a special callback function that should be implemented by the
> registered module that tells whether a particular cipher is supported by
> it :
> is_cipher_supported_cb()
> 
> The mounting option itself is not necessary, I can remove it
> 
> > Hello Andrey!
> >
> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> >> From: Andrey Markovytch 
> >>
> >> Currently eCryptfs is responsible for page encryption/decryption.
> >> This approach will not work when there is HW inline encryption.
> >> The proposed change allows external module to register with eCryptfs
> >> and provide alternative encryption mechanism and also decide whether
> >> encryption should be performed at all, or deferred to a later stage via
> >> the inline HW engine.
> >> Additional cipher option was introduced to support the HW/external mode
> >> under that name of "aes-xts". If no external module has registered
> >> to support this cipher, eCryptfs will fall back to the usual "aes"
> >
> > What is "HW/external mode"? There's no description in the commit message
> > and ecryptfs_register_to_events() does not have a caller so I'm not sure
> > of the purpose. Please provide more context.
> >
> > Despite not yet understanding the purpose of this patch, I think that I
> > can safely say that "aes-xts" is not an appropriate mount option to use
> > when enabling this mode. eCryptfs may support XTS mode one day, using
> > the Crypto API, so "HW/external mode" should not own the mount option.
> >
> > Tyler
> >
> >>
> >> Signed-off-by: Lina Zarivach 
> >> Signed-off-by: Andrey Markovytch 
> >> ---
> >>  fs/ecryptfs/Makefile  |   4 +-
> >>  fs/ecryptfs/caches_utils.c|  78 +
> >>  fs/ecryptfs/crypto.c  | 200 +++
> >>  fs/ecryptfs/debug.c   |  13 ++
> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
> >>  fs/ecryptfs/events.c  | 361
> >> ++
> >>  fs/ecryptfs/file.c|  36 +
> >>  fs/ecryptfs/inode.c   |  11 ++
> >>  fs/ecryptfs/keystore.c| 101 
> >>  fs/ecryptfs/main.c|  60 +--
> >>  fs/ecryptfs/mmap.c|   6 +
> >>  fs/ecryptfs/super.c   |  14 +-
> >>  include/linux/ecryptfs.h  |  47 ++
> >>  13 files changed, 940 insertions(+), 69 deletions(-)
> >>  create mode 100644 fs/ecryptfs/caches_utils.c
> >>  create mode 100644 fs/ecryptfs/events.c
> >>
> >> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
> >> index 49678a6..995719c 100644
> >> --- a/fs/ecryptfs/Makefile
> >> +++ b/fs/ecryptfs/Makefile
> >> @@ -4,7 +4,7 @@
> >>
> >>  obj-$(CONFIG_ECRYPT_FS) += ecryptfs.o
> >>
> >> -ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
> >> read_write.o \
> >> -crypto.o keystore.o kthread.o debug.o
> >> +ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
> >> read_write.o events.o \
> >> +crypto.o keystore.o kthread.o debug.o caches_utils.o
> >>
> >>  ecryptfs-$(CONFIG_ECRYPT_FS_MESSAGING) += messaging.o miscdev.o
> >> diff --git a/fs/ecryptfs/caches_utils.c b/fs/ecryptfs/caches_utils.c
> >> new file mode 100644
> >> index 000..c599c96
> >> --- /dev/null
> >> +++ b/fs/ecryptfs/caches_utils.c
> >> @@ -0,0 +1,78 @@
> >> +/*
> >> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Michael Halcrow]

On Mon, Nov 09, 2015 at 08:56:02PM -, andr...@codeaurora.org wrote:
> Hello, Tyler
> 
> I'll try to provide more detailed explanation, should it be satisfactory
> enough I will update the patch description.
> 
> The problem with current eCryptfs is that it has total control on how and
> when the encryption is performed and this control can't be altered.

That's by design.

eCryptfs has received at least one audit in 2014, and the auditor
didn't find any serious security flaws. eCryptfs users have
expectations about its security because of that.

It looks like you are replacing the encryption mechanism with an
out-of-kernel mechanism that implements a different encryption
mode. Users on this platform who use "eCryptfs" are actually getting
something different than what has been reviewed and audited.

On a first glance through the patch, it's not clear to me what this
out-of-kernel encryption mechanism is supposed to do. Can you point to
any design documents? From this patch, I gather that it implements
AES-256-XTS. Does the implementation conform to guidance in NIST
Special Publication 800-38E and IEEE P1619/D16?

> One example when this can be a problem is when we want to utilize an
> underlying inline HW encryption engine which allows encrypting blocks 'on
> the fly' as they are being written to the storage. In such a case relevant
> blocks just need to be marked as 'should be encrypted'. No actual
> encryption should be done by eCryptfs as it will be much slower.

What's the motivation for exposing this functionality through
eCryptfs? If eCryptfs really is the best place for this, then what are
the plans for EXT4 and F2FS, which both have per-file encryption? What
do your usage scenarios look like? What's your adversarial model?

> The provided framework allows transferring this control (if needed) to
> some external module which will do the encryption itfelf or just mark the
> appropriate blocks.
> 
> There is no caller for ecryptfs_register_to_events() since this change
> only provides framework, it doesn't provide the module itself, the module
> could be HW dependent.
> 
> Regarding the mounting option, it merely serves as example of new cipher
> mode that can be served by registered module.

If anyone can just easily swap out encryption modes, it's harder to
make any meaningful security statements about eCryptfs. Especially
given that pressure to hit benchmark targets can motivate unsafe
choices.

> There is a special callback function that should be implemented by the
> registered module that tells whether a particular cipher is supported by
> it :
> is_cipher_supported_cb()
> 
> The mounting option itself is not necessary, I can remove it
> 
> > Hello Andrey!
> >
> > On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> >> From: Andrey Markovytch 
> >>
> >> Currently eCryptfs is responsible for page encryption/decryption.
> >> This approach will not work when there is HW inline encryption.
> >> The proposed change allows external module to register with eCryptfs
> >> and provide alternative encryption mechanism and also decide whether
> >> encryption should be performed at all, or deferred to a later stage via
> >> the inline HW engine.
> >> Additional cipher option was introduced to support the HW/external mode
> >> under that name of "aes-xts". If no external module has registered
> >> to support this cipher, eCryptfs will fall back to the usual "aes"
> >
> > What is "HW/external mode"? There's no description in the commit message
> > and ecryptfs_register_to_events() does not have a caller so I'm not sure
> > of the purpose. Please provide more context.
> >
> > Despite not yet understanding the purpose of this patch, I think that I
> > can safely say that "aes-xts" is not an appropriate mount option to use
> > when enabling this mode. eCryptfs may support XTS mode one day, using
> > the Crypto API, so "HW/external mode" should not own the mount option.
> >
> > Tyler
> >
> >>
> >> Signed-off-by: Lina Zarivach 
> >> Signed-off-by: Andrey Markovytch 
> >> ---
> >>  fs/ecryptfs/Makefile  |   4 +-
> >>  fs/ecryptfs/caches_utils.c|  78 +
> >>  fs/ecryptfs/crypto.c  | 200 +++
> >>  fs/ecryptfs/debug.c   |  13 ++
> >>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
> >>  fs/ecryptfs/events.c  | 361
> >> ++
> >>  fs/ecryptfs/file.c|  36 +
> >>  fs/ecryptfs/inode.c   |  11 ++
> >>  fs/ecryptfs/keystore.c| 101 
> >>  fs/ecryptfs/main.c|  60 +--
> >>  fs/ecryptfs/mmap.c|   6 +
> >>  fs/ecryptfs/super.c   |  14 +-
> >>  include/linux/ecryptfs.h  |  47 ++
> >>  13 files changed, 940 insertions(+), 69 deletions(-)
> >>  create mode 100644 fs/ecryptfs/caches_utils.c
> >>  create mode 100644 fs/ecryptfs/events.c
> >>
> >> diff --git 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Tyler Hicks]

Hello Andrey!

On 2015-11-08 10:10:00, Andrey Markovytch wrote:
> From: Andrey Markovytch 
> 
> Currently eCryptfs is responsible for page encryption/decryption.
> This approach will not work when there is HW inline encryption.
> The proposed change allows external module to register with eCryptfs
> and provide alternative encryption mechanism and also decide whether
> encryption should be performed at all, or deferred to a later stage via
> the inline HW engine.
> Additional cipher option was introduced to support the HW/external mode
> under that name of "aes-xts". If no external module has registered
> to support this cipher, eCryptfs will fall back to the usual "aes"

What is "HW/external mode"? There's no description in the commit message
and ecryptfs_register_to_events() does not have a caller so I'm not sure
of the purpose. Please provide more context.

Despite not yet understanding the purpose of this patch, I think that I
can safely say that "aes-xts" is not an appropriate mount option to use
when enabling this mode. eCryptfs may support XTS mode one day, using
the Crypto API, so "HW/external mode" should not own the mount option.

Tyler

> 
> Signed-off-by: Lina Zarivach 
> Signed-off-by: Andrey Markovytch 
> ---
>  fs/ecryptfs/Makefile  |   4 +-
>  fs/ecryptfs/caches_utils.c|  78 +
>  fs/ecryptfs/crypto.c  | 200 +++
>  fs/ecryptfs/debug.c   |  13 ++
>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>  fs/ecryptfs/events.c  | 361 
> ++
>  fs/ecryptfs/file.c|  36 +
>  fs/ecryptfs/inode.c   |  11 ++
>  fs/ecryptfs/keystore.c| 101 
>  fs/ecryptfs/main.c|  60 +--
>  fs/ecryptfs/mmap.c|   6 +
>  fs/ecryptfs/super.c   |  14 +-
>  include/linux/ecryptfs.h  |  47 ++
>  13 files changed, 940 insertions(+), 69 deletions(-)
>  create mode 100644 fs/ecryptfs/caches_utils.c
>  create mode 100644 fs/ecryptfs/events.c
> 
> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
> index 49678a6..995719c 100644
> --- a/fs/ecryptfs/Makefile
> +++ b/fs/ecryptfs/Makefile
> @@ -4,7 +4,7 @@
>  
>  obj-$(CONFIG_ECRYPT_FS) += ecryptfs.o
>  
> -ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o read_write.o \
> -   crypto.o keystore.o kthread.o debug.o
> +ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o read_write.o 
> events.o \
> +   crypto.o keystore.o kthread.o debug.o caches_utils.o
>  
>  ecryptfs-$(CONFIG_ECRYPT_FS_MESSAGING) += messaging.o miscdev.o
> diff --git a/fs/ecryptfs/caches_utils.c b/fs/ecryptfs/caches_utils.c
> new file mode 100644
> index 000..c599c96
> --- /dev/null
> +++ b/fs/ecryptfs/caches_utils.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "../internal.h"
> +
> +void ecryptfs_drop_pagecache_sb(struct super_block *sb, void *unused)
> +{
> + struct inode *inode, *toput_inode = NULL;
> +
> + spin_lock(>s_inode_list_lock);
> + list_for_each_entry(inode, >s_inodes, i_sb_list) {
> + spin_lock(>i_lock);
> + if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) ||
> + (inode->i_mapping->nrpages == 0)) {
> + spin_unlock(>i_lock);
> + continue;
> + }
> + __iget(inode);
> + spin_unlock(>i_lock);
> + spin_unlock(>s_inode_list_lock);
> +
> + invalidate_mapping_pages(inode->i_mapping, 0, -1);
> + iput(toput_inode);
> + toput_inode = inode;
> +
> + spin_lock(>s_inode_list_lock);
> + }
> + spin_unlock(>s_inode_list_lock);
> + iput(toput_inode);
> +}
> +
> +void clean_inode_pages(struct address_space *mapping,
> + pgoff_t start, pgoff_t end)
> +{
> + struct pagevec pvec;
> + pgoff_t index = start;
> + int i;
> +
> + pagevec_init(, 0);
> + while (index <= end && pagevec_lookup(, mapping, index,
> + min(end - index,
> + (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
> + for (i = 0; i < pagevec_count(); i++) {
> + struct page *page = 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[andreym]

Hello, Tyler

I'll try to provide more detailed explanation, should it be satisfactory
enough I will update the patch description.

The problem with current eCryptfs is that it has total control on how and
when the encryption is performed and this control can't be altered. One
example when this can be a problem is when we want to utilize an
underlying inline HW encryption engine which allows encrypting blocks 'on
the fly' as they are being written to the storage. In such a case relevant
blocks just need to be marked as 'should be encrypted'. No actual
encryption should be done by eCryptfs as it will be much slower.

The provided framework allows transferring this control (if needed) to
some external module which will do the encryption itfelf or just mark the
appropriate blocks.

There is no caller for ecryptfs_register_to_events() since this change
only provides framework, it doesn't provide the module itself, the module
could be HW dependent.

Regarding the mounting option, it merely serves as example of new cipher
mode that can be served by registered module.
There is a special callback function that should be implemented by the
registered module that tells whether a particular cipher is supported by
it :
is_cipher_supported_cb()

The mounting option itself is not necessary, I can remove it

> Hello Andrey!
>
> On 2015-11-08 10:10:00, Andrey Markovytch wrote:
>> From: Andrey Markovytch 
>>
>> Currently eCryptfs is responsible for page encryption/decryption.
>> This approach will not work when there is HW inline encryption.
>> The proposed change allows external module to register with eCryptfs
>> and provide alternative encryption mechanism and also decide whether
>> encryption should be performed at all, or deferred to a later stage via
>> the inline HW engine.
>> Additional cipher option was introduced to support the HW/external mode
>> under that name of "aes-xts". If no external module has registered
>> to support this cipher, eCryptfs will fall back to the usual "aes"
>
> What is "HW/external mode"? There's no description in the commit message
> and ecryptfs_register_to_events() does not have a caller so I'm not sure
> of the purpose. Please provide more context.
>
> Despite not yet understanding the purpose of this patch, I think that I
> can safely say that "aes-xts" is not an appropriate mount option to use
> when enabling this mode. eCryptfs may support XTS mode one day, using
> the Crypto API, so "HW/external mode" should not own the mount option.
>
> Tyler
>
>>
>> Signed-off-by: Lina Zarivach 
>> Signed-off-by: Andrey Markovytch 
>> ---
>>  fs/ecryptfs/Makefile  |   4 +-
>>  fs/ecryptfs/caches_utils.c|  78 +
>>  fs/ecryptfs/crypto.c  | 200 +++
>>  fs/ecryptfs/debug.c   |  13 ++
>>  fs/ecryptfs/ecryptfs_kernel.h |  78 +
>>  fs/ecryptfs/events.c  | 361
>> ++
>>  fs/ecryptfs/file.c|  36 +
>>  fs/ecryptfs/inode.c   |  11 ++
>>  fs/ecryptfs/keystore.c| 101 
>>  fs/ecryptfs/main.c|  60 +--
>>  fs/ecryptfs/mmap.c|   6 +
>>  fs/ecryptfs/super.c   |  14 +-
>>  include/linux/ecryptfs.h  |  47 ++
>>  13 files changed, 940 insertions(+), 69 deletions(-)
>>  create mode 100644 fs/ecryptfs/caches_utils.c
>>  create mode 100644 fs/ecryptfs/events.c
>>
>> diff --git a/fs/ecryptfs/Makefile b/fs/ecryptfs/Makefile
>> index 49678a6..995719c 100644
>> --- a/fs/ecryptfs/Makefile
>> +++ b/fs/ecryptfs/Makefile
>> @@ -4,7 +4,7 @@
>>
>>  obj-$(CONFIG_ECRYPT_FS) += ecryptfs.o
>>
>> -ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
>> read_write.o \
>> -  crypto.o keystore.o kthread.o debug.o
>> +ecryptfs-y := dentry.o file.o inode.o main.o super.o mmap.o
>> read_write.o events.o \
>> +  crypto.o keystore.o kthread.o debug.o caches_utils.o
>>
>>  ecryptfs-$(CONFIG_ECRYPT_FS_MESSAGING) += messaging.o miscdev.o
>> diff --git a/fs/ecryptfs/caches_utils.c b/fs/ecryptfs/caches_utils.c
>> new file mode 100644
>> index 000..c599c96
>> --- /dev/null
>> +++ b/fs/ecryptfs/caches_utils.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "../internal.h"
>> +
>> +void ecryptfs_drop_pagecache_sb(struct 

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[kbuild test robot]

Hi Andrey,

[auto build test ERROR on: v4.3-rc7]
[also build test ERROR on: next-20151106]

url:
https://github.com/0day-ci/linux/commits/Andrey-Markovytch/eCryptfs-enhancing-eCryptfs-to-be-used-with-external-crypto-engine/20151108-161722
config: i386-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "__iget" undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[kbuild test robot]

Hi Andrey,

[auto build test ERROR on: v4.3-rc7]
[also build test ERROR on: next-20151106]

url:
https://github.com/0day-ci/linux/commits/Andrey-Markovytch/eCryptfs-enhancing-eCryptfs-to-be-used-with-external-crypto-engine/20151108-161722
config: x86_64-randconfig-s0-11081650 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "__iget" [fs/ecryptfs/ecryptfs.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Christoph Hellwig]

On Sun, Nov 08, 2015 at 10:10:00AM +0200, Andrey Markovytch wrote:
> +++ b/fs/ecryptfs/caches_utils.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.

Really?  This looks like copy and paste from core code that defintively
was not written by the Linux Foundation.  In addition this patch comes
from Qualcomm so something very fishy is going on here, which if I'd
call copyrght fraud if I'd want to be be mean.

Please a) stop pointlessly copy and pasting code and b) have a word with
your lawyers on how to attribute code both that your wrote and which has
been copy and pasted.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[kbuild test robot]

Hi Andrey,

[auto build test ERROR on: v4.3-rc7]
[also build test ERROR on: next-20151106]

url:
https://github.com/0day-ci/linux/commits/Andrey-Markovytch/eCryptfs-enhancing-eCryptfs-to-be-used-with-external-crypto-engine/20151108-161722
config: x86_64-randconfig-s0-11081650 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "__iget" [fs/ecryptfs/ecryptfs.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[Christoph Hellwig]

On Sun, Nov 08, 2015 at 10:10:00AM +0200, Andrey Markovytch wrote:
> +++ b/fs/ecryptfs/caches_utils.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.

Really?  This looks like copy and paste from core code that defintively
was not written by the Linux Foundation.  In addition this patch comes
from Qualcomm so something very fishy is going on here, which if I'd
call copyrght fraud if I'd want to be be mean.

Please a) stop pointlessly copy and pasting code and b) have a word with
your lawyers on how to attribute code both that your wrote and which has
been copy and pasted.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1] eCryptfs: enhancing eCryptfs to be used with external crypto engine

[kbuild test robot]

Hi Andrey,

[auto build test ERROR on: v4.3-rc7]
[also build test ERROR on: next-20151106]

url:
https://github.com/0day-ci/linux/commits/Andrey-Markovytch/eCryptfs-enhancing-eCryptfs-to-be-used-with-external-crypto-engine/20151108-161722
config: i386-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "__iget" undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data