Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Eric Biggers]

On Thu, Oct 31, 2019 at 10:52:19AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Oct 30, 2019 at 2:59 PM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers  wrote:
> > >
> > > FWIW, from reading the Chrome OS code, I think the code you linked to 
> > > isn't
> > > where the breakage actually is.  I think it's actually at
> > > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
> > > ... where an init script is using the error message printed by 'e4crypt
> > > get_policy' to decide whether to add -O encrypt to the filesystem or not.
> > >
> > > It really should check instead:
> > >
> > > [ -e /sys/fs/ext4/features/encryption ]
> >
> > OK, I filed  and CCed all the people listed
> > in the cryptohome "OWNERS" file.  Hopefully one of them can pick this
> > up as a general cleanup.  Thanks!
> 
> Just to follow-up: I did a quick test here to see if I could fix
> "chromeos-common.sh" as you suggested.  Then I got rid of the Revert
> and tried to login.  No joy.
> 
> Digging a little deeper, the ext4_dir_encryption_supported() function
> is called in two places:
> * chromeos-install
> * chromeos_startup
> 
> In my test case I had a machine that I'd already logged into (on a
> previous kernel version) and I was trying to log into it a second
> time.  Thus there's no way that chromeos-install could be involved.
> Looking at chromeos_startup:
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/init/chromeos_startup
> 
> ...the function is only used for setting up the "encrypted stateful"
> partition.  That wasn't where my failure was.  My failure was with
> logging in AKA with cryptohome.  Thus I think it's plausible that my
> original commit message pointing at cryptohome may have been correct.
> It's possible that there were _also_ problems with encrypted stateful
> that I wasn't noticing, but if so they were not the only problems.
> 
> It still may be wise to make Chrome OS use different tests, but it
> might not be quite as simple as hoped...
> 

Ah, I think I found it:

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/2cbdedd5eca0a57d9596671a99da5fab8e60722b/sys-apps/upstart/files/upstart-1.2-dircrypto.patch

The init process does EXT4_IOC_GET_ENCRYPTION_POLICY on /, and if the error is
EOPNOTSUPP, it skips creating the "dircrypto" keyring.  So then cryptohome can't
add keys later.  (Note the error message you got, "Error adding dircrypto key".)

So it looks like the kernel patch broke both that and
ext4_dir_encryption_supported().

I don't see how it could have broken cryptohome by itself, since AFAICS
cryptohome only uses EXT4_IOC_GET_ENCRYPTION_POLICY on the partition which is
supposed to have the 'encrypt' feature set.

- Eric


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support

[Eric Biggers]

On Thu, Oct 31, 2019 at 02:21:03PM -0700, Christoph Hellwig wrote:
> On Thu, Oct 31, 2019 at 01:21:26PM -0700, Eric Biggers wrote:
> > > > +   /* The file must need contents encryption, not filenames 
> > > > encryption */
> > > > +   if (!S_ISREG(inode->i_mode))
> > > > +   return false;
> > > 
> > > But that isn't really what the check checks for..
> > 
> > This is how fscrypt has always worked.  The type of encryption to do is
> > determined as follows:
> > 
> > S_ISREG() => contents encryption
> > S_ISDIR() || S_ISLNK() => filenames encryption
> > 
> > See e.g. select_encryption_mode(), and similar checks elsewhere in
> > fs/{crypto,ext4,f2fs}/.
> > 
> > Do you have a suggestion to make it clearer?
> 
> Maybe have a fscrypt_content_encryption helper that currently
> evaluates to S_ISREG(inode->i_mode) as that documents the intent?

It's more important to clean up the IS_ENCRYPTED(inode) &&
S_ISREG(inode->i_mode) checks that are duplicated in fs/{ext4,f2fs}/, so I've
been thinking of adding a helper:

static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
{
return IS_ENABLED(CONFIG_FS_ENCRYPTION) && IS_ENCRYPTED(inode) &&
   S_ISREG(inode->i_mode);
}

It could be used here too, though only S_ISREG() is actually needed here, so it
wouldn't be as good of a fit.

Anyway, this will need to be a separate cleanup.

> 
> > > > +   /* The filesystem must be mounted with -o inlinecrypt */
> > > > +   if (!sb->s_cop->inline_crypt_enabled ||
> > > > +   !sb->s_cop->inline_crypt_enabled(sb))
> > > > +   return false;
> > > 
> > > So please add a SB_* flag for that option instead of the weird
> > > indirection.
> > 
> > IMO it's not really "weird" given that the point of the fscrypt_operations 
> > is to
> > expose filesystem-specific stuff to fs/crypto/.  But yes, using one of the 
> > SB_*
> > bits would make it simpler, so if people are fine with that we'll do that.
> 
> I think that is much simpler.  Additionally it could also replace the
> need for the has_stable_inodes and get_ino_and_lblk_bits methods, which
> are pretty weird.  Instead just document the requirements for the
> SB_INLINE_CRYPT flag and handle the rest in the file system.  That is
> for f2f always set it, and for ext4 set it based on s_inodes_count.
> Which brings me to:
> 

I don't think combining these things is a good idea because it would restrict
the use of inline encryption to filesystems that allow IV_INO_LBLK_64 encryption
policies, i.e. filesystems that have stable inode numbers, 32-bit inodes, and
32-bit file logical block numbers.

The on-disk format (i.e. the type of encryption policy chosen) and the
implementation (inline or filesystem-layer crypto) are really two separate
things.  This was one of the changes in v4 => v5 of this patchset; these two
things used to be conflated but now they are separate.  Now you can use inline
encryption with the existing fscrypt policies too.

We could use two separate SB_* flags, like SB_INLINE_CRYPT and
SB_IV_INO_LBLK_64_SUPPORT.  However, the ->has_stable_inodes() and
->get_ino_and_lblk_bits() methods are nice because they separate the filesystem
properties from the question of "is this encryption policy supported".
Declaring the filesystem properties is easier to do because it doesn't require
any fscrypt-specific knowledge.  Also, fs/crypto/ could use these properties in
different ways in the future, e.g. if another IV generation scheme is added.

> > > 
> > > Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
> > > That really should be a parameter, not hardcoded.
> > 
> > To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/, 
> > but
> > rather in what the blk-crypto API provides.  If blk-crypto were to provide
> > longer IV support, fs/crypto/ would pretty easily be able to make use of it.
> 
> That's what I meant - we hardcode the value in fscrypt.  Instead we need
> to expose the size from blk-crypt and check for it.
> 

If we add variable-length IV support, I think it should be handled in the same
way as crypto algorithms.  The current model is that the bio submitter doesn't
need to check whether the crypto algorithm is supported by the device, because
blk-crypto will fall back to the crypto API if needed.  Unsupported IV lengths
could be handled in the same way.

- Eric


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [RFC] errno.h: Provide EFSCORRUPTED for everybody

[Theodore Y. Ts'o]

On Wed, Oct 30, 2019 at 09:07:33PM -0400, Valdis Kletnieks wrote:
> Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> (c) if one patch, who gets to shepherd it through?

Acked-by: Theodore Ts'o 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 3/9] block: blk-crypto for Inline Encryption

[Christoph Hellwig]

On Thu, Oct 31, 2019 at 04:50:45PM -0400, Theodore Y. Ts'o wrote:
> One of the reasons I really want this is so I (as an upstream
> maintainer of ext4 and fscrypt) can test the new code paths using
> xfstests on GCE, without needing special pre-release hardware that has
> the ICE support.
> 
> Yeah, I could probably get one of those dev boards internally at
> Google, but they're a pain in the tuckus to use, and I'd much rather
> be able to have my normal test infrastructure using gce-xfstests and
> kvm-xfstests be able to test inline-crypto.  So in terms of CI
> testing, having the blk-crypto is really going to be helpful.

Implementing the support in qemu or a special device mapper mode
seems like a much better idea for that use case over carrying the
code in the block layer and severely bloating the per-I/O data
structure.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support

[Christoph Hellwig]

On Thu, Oct 31, 2019 at 01:21:26PM -0700, Eric Biggers wrote:
> > > + /* The file must need contents encryption, not filenames encryption */
> > > + if (!S_ISREG(inode->i_mode))
> > > + return false;
> > 
> > But that isn't really what the check checks for..
> 
> This is how fscrypt has always worked.  The type of encryption to do is
> determined as follows:
> 
> S_ISREG() => contents encryption
> S_ISDIR() || S_ISLNK() => filenames encryption
> 
> See e.g. select_encryption_mode(), and similar checks elsewhere in
> fs/{crypto,ext4,f2fs}/.
> 
> Do you have a suggestion to make it clearer?

Maybe have a fscrypt_content_encryption helper that currently
evaluates to S_ISREG(inode->i_mode) as that documents the intent?

> > > + /* The filesystem must be mounted with -o inlinecrypt */
> > > + if (!sb->s_cop->inline_crypt_enabled ||
> > > + !sb->s_cop->inline_crypt_enabled(sb))
> > > + return false;
> > 
> > So please add a SB_* flag for that option instead of the weird
> > indirection.
> 
> IMO it's not really "weird" given that the point of the fscrypt_operations is 
> to
> expose filesystem-specific stuff to fs/crypto/.  But yes, using one of the 
> SB_*
> bits would make it simpler, so if people are fine with that we'll do that.

I think that is much simpler.  Additionally it could also replace the
need for the has_stable_inodes and get_ino_and_lblk_bits methods, which
are pretty weird.  Instead just document the requirements for the
SB_INLINE_CRYPT flag and handle the rest in the file system.  That is
for f2f always set it, and for ext4 set it based on s_inodes_count.
Which brings me to:

> > 
> > Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
> > That really should be a parameter, not hardcoded.
> 
> To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/, but
> rather in what the blk-crypto API provides.  If blk-crypto were to provide
> longer IV support, fs/crypto/ would pretty easily be able to make use of it.

That's what I meant - we hardcode the value in fscrypt.  Instead we need
to expose the size from blk-crypt and check for it.

> 
> (And if IVs >= 24 bytes were supported and we added AES-128-CBC-ESSIV and
> Adiantum support to blk-crypto.c, then inline encryption would be able to do
> everything that the existing filesystem-layer contents encryption can do.)
> 
> Do you have anything in mind for how to make the API support longer IVs in a
> clean way?  Are you thinking of something like:
> 
>   #define BLK_CRYPTO_MAX_DUN_SIZE 24
> 
>   u8 dun[BLK_CRYPTO_MAX_DUN_SIZE];
>   int dun_size;
> 
> We do have to perform arithmetic operations on it, so a byte array would make 
> it
> ugly and slower, but it should be possible...

Well, we could make it an array of u64s, which means we can do all the
useful arithmetics on components on one of them.  But I see the point,
this adds significant complexity for no real short term gain, and we
should probably postponed it until needed.  Maybe just document the
assumptions a little better.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 3/9] block: blk-crypto for Inline Encryption

[Theodore Y. Ts'o]

On Thu, Oct 31, 2019 at 10:57:13AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 28, 2019 at 12:20:26AM -0700, Satya Tangirala wrote:
> > We introduce blk-crypto, which manages programming keyslots for struct
> > bios. With blk-crypto, filesystems only need to call bio_crypt_set_ctx with
> > the encryption key, algorithm and data_unit_num; they don't have to worry
> > about getting a keyslot for each encryption context, as blk-crypto handles
> > that. Blk-crypto also makes it possible for layered devices like device
> > mapper to make use of inline encryption hardware.
> > 
> > Blk-crypto delegates crypto operations to inline encryption hardware when
> > available, and also contains a software fallback to the kernel crypto API.
> > For more details, refer to Documentation/block/inline-encryption.rst.
> 
> Can you explain why we need this software fallback that basically just
> duplicates logic already in fscrypt?  As far as I can tell this fallback
> logic actually is more code than the actual inline encryption, and nasty
> code at that, e.g. the whole crypt_iter thing.

One of the reasons I really want this is so I (as an upstream
maintainer of ext4 and fscrypt) can test the new code paths using
xfstests on GCE, without needing special pre-release hardware that has
the ICE support.

Yeah, I could probably get one of those dev boards internally at
Google, but they're a pain in the tuckus to use, and I'd much rather
be able to have my normal test infrastructure using gce-xfstests and
kvm-xfstests be able to test inline-crypto.  So in terms of CI
testing, having the blk-crypto is really going to be helpful.

- Ted


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support

[Eric Biggers]

Hi Christoph, thanks for reviewing this.

On Thu, Oct 31, 2019 at 11:32:17AM -0700, Christoph Hellwig wrote:
> > diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> > index 1f4b8a277060..956798debf71 100644
> > --- a/fs/crypto/bio.c
> > +++ b/fs/crypto/bio.c
> > @@ -46,26 +46,38 @@ int fscrypt_zeroout_range(const struct inode *inode, 
> > pgoff_t lblk,
> >  {
> > const unsigned int blockbits = inode->i_blkbits;
> > const unsigned int blocksize = 1 << blockbits;
> > +   const bool inlinecrypt = fscrypt_inode_uses_inline_crypto(inode);
> > struct page *ciphertext_page;
> > struct bio *bio;
> > int ret, err = 0;
> >  
> > -   ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> > -   if (!ciphertext_page)
> > -   return -ENOMEM;
> > +   if (inlinecrypt) {
> > +   ciphertext_page = ZERO_PAGE(0);
> > +   } else {
> > +   ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> > +   if (!ciphertext_page)
> > +   return -ENOMEM;
> 
> I think you just want to split this into two functions for the
> inline crypto vs not cases.
> 
> > @@ -391,6 +450,16 @@ struct fscrypt_master_key {
> >  */
> > struct crypto_skcipher  *mk_iv_ino_lblk_64_tfms[__FSCRYPT_MODE_MAX + 1];
> >  
> > +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> > +   /* Raw keys for IV_INO_LBLK_64 policies, allocated on-demand */
> > +   u8  *mk_iv_ino_lblk_64_raw_keys[__FSCRYPT_MODE_MAX 
> > + 1];
> > +
> > +   /* The data unit size being used for inline encryption */
> > +   unsigned intmk_data_unit_size;
> > +
> > +   /* The filesystem's block device */
> > +   struct block_device *mk_bdev;
> 
> File systems (including f2fs) can have multiple underlying block
> devices.  

Yes, this is a bug: it causes the key to be evicted from only one device.

I'm thinking this might point to deeper problems with the inline encryption API
exposed to filesystems being too difficult to use correctly.  We might want to
move to something like:


struct blk_crypto_key_handle *
blk_crypto_prepare_key(struct request_queue *q, const u8 *raw_key,
   enum blk_crypto_mode_num crypto_mode, unsigned int 
data_unit_size);

int bio_crypt_set_ctx(struct bio *bio, struct blk_crypto_key_handle *h,
  u64 dun, gfp_t gfp_mask);

void blk_crypto_destroy_key(struct blk_crypto_key_handle *h);


Each handle would associate a raw_key, mode, and data_unit_size with a specific
keyslot_manager, which would allocate/evict a keyslot as needed for I/O.
blk_crypto_destroy_key() would both evict the keyslot and free the key.

For each key, multi-device filesystems would then need to allocate a handle for
each device.  This would force them to handle key eviction correctly.

Satya, any thoughts on this?

> 
> > +{
> > +   const struct inode *inode = ci->ci_inode;
> > +   struct super_block *sb = inode->i_sb;
> > +
> > +   /* The file must need contents encryption, not filenames encryption */
> > +   if (!S_ISREG(inode->i_mode))
> > +   return false;
> 
> But that isn't really what the check checks for..

This is how fscrypt has always worked.  The type of encryption to do is
determined as follows:

S_ISREG() => contents encryption
S_ISDIR() || S_ISLNK() => filenames encryption

See e.g. select_encryption_mode(), and similar checks elsewhere in
fs/{crypto,ext4,f2fs}/.

Do you have a suggestion to make it clearer?

> 
> > +   /* The filesystem must be mounted with -o inlinecrypt */
> > +   if (!sb->s_cop->inline_crypt_enabled ||
> > +   !sb->s_cop->inline_crypt_enabled(sb))
> > +   return false;
> 
> So please add a SB_* flag for that option instead of the weird
> indirection.

IMO it's not really "weird" given that the point of the fscrypt_operations is to
expose filesystem-specific stuff to fs/crypto/.  But yes, using one of the SB_*
bits would make it simpler, so if people are fine with that we'll do that.

> 
> > +/**
> > + * fscrypt_inode_uses_inline_crypto - test whether an inode uses inline 
> > encryption
> 
> This adds an overly long line.  This happens many more times in the
> patch.

checkpatch reports 4 overly long lines in the patch, 3 of which are the first
line of kerneldoc comments.  For some reason I thought those are recommended to
be one line even if it exceeds 80 characters, but maybe I'm mistaken.

> 
> Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
> That really should be a parameter, not hardcoded.

To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/, but
rather in what the blk-crypto API provides.  If blk-crypto were to provide
longer IV support, fs/crypto/ would pretty easily be able to make use of it.

(And if IVs >= 24 bytes were supported and we added AES-128-CBC-ESSIV and
Adiantum support to blk-crypto.c, then inline encryption would be able to do
everything that the existing filesystem-layer contents encryption can do.)

Do you have anything in mind for how 

Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support

[Christoph Hellwig]

> diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> index 1f4b8a277060..956798debf71 100644
> --- a/fs/crypto/bio.c
> +++ b/fs/crypto/bio.c
> @@ -46,26 +46,38 @@ int fscrypt_zeroout_range(const struct inode *inode, 
> pgoff_t lblk,
>  {
>   const unsigned int blockbits = inode->i_blkbits;
>   const unsigned int blocksize = 1 << blockbits;
> + const bool inlinecrypt = fscrypt_inode_uses_inline_crypto(inode);
>   struct page *ciphertext_page;
>   struct bio *bio;
>   int ret, err = 0;
>  
> - ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> - if (!ciphertext_page)
> - return -ENOMEM;
> + if (inlinecrypt) {
> + ciphertext_page = ZERO_PAGE(0);
> + } else {
> + ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> + if (!ciphertext_page)
> + return -ENOMEM;

I think you just want to split this into two functions for the
inline crypto vs not cases.

> @@ -391,6 +450,16 @@ struct fscrypt_master_key {
>*/
>   struct crypto_skcipher  *mk_iv_ino_lblk_64_tfms[__FSCRYPT_MODE_MAX + 1];
>  
> +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> + /* Raw keys for IV_INO_LBLK_64 policies, allocated on-demand */
> + u8  *mk_iv_ino_lblk_64_raw_keys[__FSCRYPT_MODE_MAX 
> + 1];
> +
> + /* The data unit size being used for inline encryption */
> + unsigned intmk_data_unit_size;
> +
> + /* The filesystem's block device */
> + struct block_device *mk_bdev;

File systems (including f2fs) can have multiple underlying block
devices.  

> +{
> + const struct inode *inode = ci->ci_inode;
> + struct super_block *sb = inode->i_sb;
> +
> + /* The file must need contents encryption, not filenames encryption */
> + if (!S_ISREG(inode->i_mode))
> + return false;

But that isn't really what the check checks for..

> + /* The filesystem must be mounted with -o inlinecrypt */
> + if (!sb->s_cop->inline_crypt_enabled ||
> + !sb->s_cop->inline_crypt_enabled(sb))
> + return false;

So please add a SB_* flag for that option instead of the weird
indirection.

> +/**
> + * fscrypt_inode_uses_inline_crypto - test whether an inode uses inline 
> encryption

This adds an overly long line.  This happens many more times in the
patch.

Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
That really should be a parameter, not hardcoded.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS

[Christoph Hellwig]

>   /* Transfer request descriptor header fields */
> + if (lrbp->crypto_enable) {

Maybe we want a little inline function so that we can use IS_ENABLED
to make sure the compiler eliminates the dead code if crypt config
option is not set.

 a) don't have to define the crypto_enable if the config options are
not set

> + dword_0 |= UTP_REQ_DESC_CRYPTO_ENABLE_CMD;
> + dword_0 |= lrbp->crypto_key_slot;
> + req_desc->header.dword_1 =
> + cpu_to_le32((u32)lrbp->data_unit_num);
> + req_desc->header.dword_3 =
> + cpu_to_le32((u32)(lrbp->data_unit_num >> 32));

This should use ther upper_32_bits / lower_32_bits helpers.

> +static inline int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba,
> +  struct scsi_cmnd *cmd,
> +  struct ufshcd_lrb *lrbp)
> +{
> + int key_slot;
> +
> + if (!cmd->request->bio ||
> + !bio_crypt_should_process(cmd->request->bio, cmd->request->q)) {
> + lrbp->crypto_enable = false;
> + return 0;
> + }
> +
> + if (WARN_ON(!ufshcd_is_crypto_enabled(hba))) {
> + /*
> +  * Upper layer asked us to do inline encryption
> +  * but that isn't enabled, so we fail this request.
> +  */
> + return -EINVAL;
> + }
> + key_slot = bio_crypt_get_keyslot(cmd->request->bio);
> + if (!ufshcd_keyslot_valid(hba, key_slot))
> + return -EINVAL;
> +
> + lrbp->crypto_enable = true;
> + lrbp->crypto_key_slot = key_slot;
> + lrbp->data_unit_num = bio_crypt_data_unit_num(cmd->request->bio);
> +
> + return 0;

I think this should go into ufshcd-crypto.c so that it can be stubbed
out for non-crypto builds.  That also means we can remove various
stubs for the block layer helpers and just dereference the fields
directly, helping with code readability.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 5/9] scsi: ufs: UFS crypto API

[Christoph Hellwig]

> +static size_t get_keysize_bytes(enum ufs_crypto_key_size size)
> +{
> + switch (size) {
> + case UFS_CRYPTO_KEY_SIZE_128: return 16;
> + case UFS_CRYPTO_KEY_SIZE_192: return 24;
> + case UFS_CRYPTO_KEY_SIZE_256: return 32;
> + case UFS_CRYPTO_KEY_SIZE_512: return 64;
> + default: return 0;
> + }
> +}

Please fix the indentation and move all the returns to their own
lines.  There are various more spots that will need to be fixed
like this as well later in the patch.

> +
> +static int ufshcd_crypto_cap_find(void *hba_p,
> +enum blk_crypto_mode_num crypto_mode,
> +unsigned int data_unit_size)
> +{
> + struct ufs_hba *hba = hba_p;

Please properly type the first argument.

> + case UFS_CRYPTO_ALG_BITLOCKER_AES_CBC: // fallthrough

Please don't use // comments.

> +static void program_key(struct ufs_hba *hba,
> + const union ufs_crypto_cfg_entry *cfg,
> + int slot)

The function name needs a ufshcd prefix.

> + wmb();
> + for (i = 0; i < 16; i++) {
> + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[i]),
> +   slot_offset + i * sizeof(cfg->reg_val[0]));
> + /* Spec says each dword in key must be written sequentially */
> + wmb();
> + }
> + /* Write dword 17 */
> + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[17]),
> +   slot_offset + 17 * sizeof(cfg->reg_val[0]));
> + /* Dword 16 must be written last */
> + wmb();
> + /* Write dword 16 */
> + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[16]),
> +   slot_offset + 16 * sizeof(cfg->reg_val[0]));
> + wmb();

wmb() has no meaning for MMIO operations, something looks very fishy
here.

> +static int ufshcd_crypto_keyslot_program(void *hba_p, const u8 *key,
> +  enum blk_crypto_mode_num crypto_mode,
> +  unsigned int data_unit_size,
> +  unsigned int slot)
> +{
> + struct ufs_hba *hba = hba_p;

This is not a very type safe API.  I think the proper thing to do
would be to allocte the struct keyslot_manager in the driver (ufshcd)
as part of the containing structure (ufs_hba) and then just have
a keyslot_manager_init that initializes the field.  Then pass the
struct keyslot_manager to the methods, which can use container_of
to get the containing structure.

> +#define NUM_KEYSLOTS(hba) (hba->crypto_capabilities.config_count + 1)

Please make this an inline function.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 2/9] block: Add encryption context to struct bio

[Christoph Hellwig]

> +static int num_prealloc_crypt_ctxs = 128;

Where does that magic number come from?

> +struct bio_crypt_ctx *bio_crypt_alloc_ctx(gfp_t gfp_mask)
> +{
> + return mempool_alloc(bio_crypt_ctx_pool, gfp_mask);
> +}
> +EXPORT_SYMBOL(bio_crypt_alloc_ctx);

This isn't used by an modular code.

> +void bio_crypt_free_ctx(struct bio *bio)
> +{
> + mempool_free(bio->bi_crypt_context, bio_crypt_ctx_pool);
> + bio->bi_crypt_context = NULL;
> +}
> +EXPORT_SYMBOL(bio_crypt_free_ctx);

This one is called from modular code, but I think the usage in DM
is bogus, as the caller of the function eventually does a bio_put,
which ends up in bio_free and takes care of the freeing as well.

> +bool bio_crypt_should_process(struct bio *bio, struct request_queue *q)
> +{
> + if (!bio_has_crypt_ctx(bio))
> + return false;
> +
> + WARN_ON(!bio_crypt_has_keyslot(bio));
> + return q->ksm == bio->bi_crypt_context->processing_ksm;
> +}

Passing a struct request here and also adding the ->bio != NULL check
here would simplify the only caller in ufs a bit.

> +/*
> + * Checks that two bio crypt contexts are compatible - i.e. that
> + * they are mergeable except for data_unit_num continuity.
> + */
> +bool bio_crypt_ctx_compatible(struct bio *b_1, struct bio *b_2)
> +{
> + struct bio_crypt_ctx *bc1 = b_1->bi_crypt_context;
> + struct bio_crypt_ctx *bc2 = b_2->bi_crypt_context;
> +
> + if (bio_has_crypt_ctx(b_1) != bio_has_crypt_ctx(b_2))
> + return false;
> +
> + if (!bio_has_crypt_ctx(b_1))
> + return true;
> +
> + return bc1->keyslot == bc2->keyslot &&
> +bc1->data_unit_size_bits == bc2->data_unit_size_bits;
> +}

I think we'd normally call this bio_crypt_ctx_mergeable.

> + if (bio_crypt_clone(b, bio, gfp_mask) < 0) {
> + bio_put(b);
> + return NULL;
> + }
>  
> - if (ret < 0) {
> - bio_put(b);
> - return NULL;
> - }
> + if (bio_integrity(bio) &&
> + bio_integrity_clone(b, bio, gfp_mask) < 0) {
> + bio_put(b);
> + return NULL;

Pleae use a goto to merge the common error handling path

> + if (!bio_crypt_ctx_back_mergeable(req->bio,
> +   blk_rq_sectors(req),
> +   next->bio)) {
> + return ELEVATOR_NO_MERGE;
> + }

No neef for the braces.  And pretty weird alignment, normal Linux style
would be:

if (!bio_crypt_ctx_back_mergeable(req->bio,
blk_rq_sectors(req), next->bio))
return ELEVATOR_NO_MERGE;

> + if (!bio_crypt_ctx_back_mergeable(rq->bio,
> +   blk_rq_sectors(rq), bio)) {
> + return ELEVATOR_NO_MERGE;
> + }
>   return ELEVATOR_BACK_MERGE;
> - else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector)
> + } else if (blk_rq_pos(rq) - bio_sectors(bio) ==
> +bio->bi_iter.bi_sector) {
> + if (!bio_crypt_ctx_back_mergeable(bio,
> +   bio_sectors(bio), rq->bio)) {
> + return ELEVATOR_NO_MERGE;
> + }

Same for these two.

> +++ b/block/bounce.c
> @@ -267,14 +267,15 @@ static struct bio *bounce_clone_bio(struct bio 
> *bio_src, gfp_t gfp_mask,
>   break;
>   }
>  
> - if (bio_integrity(bio_src)) {
> - int ret;
> + if (bio_crypt_clone(bio, bio_src, gfp_mask) < 0) {
> + bio_put(bio);
> + return NULL;
> + }
>  
> - ret = bio_integrity_clone(bio, bio_src, gfp_mask);
> - if (ret < 0) {
> - bio_put(bio);
> - return NULL;
> - }
> + if (bio_integrity(bio_src) &&
> + bio_integrity_clone(bio, bio_src, gfp_mask) < 0) {
> + bio_put(bio);
> + return NULL;

Use a common error path with a goto, please.

> +static inline int bio_crypt_set_ctx(struct bio *bio,
> + const u8 *raw_key,
> + enum blk_crypto_mode_num crypto_mode,
> + u64 dun,
> + unsigned int dun_bits,
> + gfp_t gfp_mask)

Pleae just open code this in the only caller.

> +{
> + struct bio_crypt_ctx *crypt_ctx;
> +
> + crypt_ctx = bio_crypt_alloc_ctx(gfp_mask);
> + if (!crypt_ctx)
> + return -ENOMEM;

Also bio_crypt_alloc_ctx with a waiting mask will never return an
error.  Changing this function and its call chain to void returns will
clean up a lot of code in this series.

> +static inline void bio_set_data_unit_num(struct bio *bio, u64 dun)
> +{
> + bio->bi_crypt_context->data_unit_num = dun;
> +}

Re: [f2fs-dev] [PATCH v5 3/9] block: blk-crypto for Inline Encryption

[Christoph Hellwig]

On Mon, Oct 28, 2019 at 12:20:26AM -0700, Satya Tangirala wrote:
> We introduce blk-crypto, which manages programming keyslots for struct
> bios. With blk-crypto, filesystems only need to call bio_crypt_set_ctx with
> the encryption key, algorithm and data_unit_num; they don't have to worry
> about getting a keyslot for each encryption context, as blk-crypto handles
> that. Blk-crypto also makes it possible for layered devices like device
> mapper to make use of inline encryption hardware.
> 
> Blk-crypto delegates crypto operations to inline encryption hardware when
> available, and also contains a software fallback to the kernel crypto API.
> For more details, refer to Documentation/block/inline-encryption.rst.

Can you explain why we need this software fallback that basically just
duplicates logic already in fscrypt?  As far as I can tell this fallback
logic actually is more code than the actual inline encryption, and nasty
code at that, e.g. the whole crypt_iter thing.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] Revert "ext4 crypto: fix to check feature status before get policy"

[Doug Anderson]

Hi,

On Wed, Oct 30, 2019 at 2:59 PM Doug Anderson  wrote:
>
> Hi,
>
> On Wed, Oct 30, 2019 at 1:57 PM Eric Biggers  wrote:
> >
> > FWIW, from reading the Chrome OS code, I think the code you linked to isn't
> > where the breakage actually is.  I think it's actually at
> > https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/chromeos-common-script/share/chromeos-common.sh#375
> > ... where an init script is using the error message printed by 'e4crypt
> > get_policy' to decide whether to add -O encrypt to the filesystem or not.
> >
> > It really should check instead:
> >
> > [ -e /sys/fs/ext4/features/encryption ]
>
> OK, I filed  and CCed all the people listed
> in the cryptohome "OWNERS" file.  Hopefully one of them can pick this
> up as a general cleanup.  Thanks!

Just to follow-up: I did a quick test here to see if I could fix
"chromeos-common.sh" as you suggested.  Then I got rid of the Revert
and tried to login.  No joy.

Digging a little deeper, the ext4_dir_encryption_supported() function
is called in two places:
* chromeos-install
* chromeos_startup

In my test case I had a machine that I'd already logged into (on a
previous kernel version) and I was trying to log into it a second
time.  Thus there's no way that chromeos-install could be involved.
Looking at chromeos_startup:

https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/init/chromeos_startup

...the function is only used for setting up the "encrypted stateful"
partition.  That wasn't where my failure was.  My failure was with
logging in AKA with cryptohome.  Thus I think it's plausible that my
original commit message pointing at cryptohome may have been correct.
It's possible that there were _also_ problems with encrypted stateful
that I wasn't noticing, but if so they were not the only problems.

It still may be wise to make Chrome OS use different tests, but it
might not be quite as simple as hoped...

-Doug


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] Revert "ext4 crypto: fix to check feature status before get policy"

[Theodore Y. Ts'o]

On Wed, Oct 30, 2019 at 02:51:38PM -0700, Eric Biggers wrote:
> From: Douglas Anderson 
> 
> This reverts commit 0642ea2409f3 ("ext4 crypto: fix to check feature
> status before get policy").
> 
> The commit made a clear and documented ABI change that is not backward
> compatible.  There exists userspace code [1][2] that relied on the old
> behavior and is now broken.
> 
> While we could entertain the idea of updating the userspace code to
> handle the ABI change, it's my understanding that in general ABI
> changes that break userspace are frowned upon (to put it nicely).

The rule is that if someone complains, we have to revert.  Douglas's
email counts as a complaint, so we should revert.  That being said, if
ChromeOS (userspace) changes to using /sys/fs/ext4/features/encryption
to determine whether or not the kernel supports encryption, then we
can in the future change the error code to make things consistent with
f2fs.

This looks good, I'll pull it into ext4 git tree.

- Ted


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [RFC] errno.h: Provide EFSCORRUPTED for everybody

[Jan Kara]

On Wed 30-10-19 21:07:33, Valdis Kletnieks wrote:
> Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> (c) if one patch, who gets to shepherd it through?
> 
> 
> There's currently 6 filesystems that have the same #define. Move it
> into errno.h so it's defined in just one place.
> 
> Signed-off-by: Valdis Kletnieks 

Looks good to me. You can add:

Reviewed-by: Jan Kara 

Honza

> ---
>  drivers/staging/exfat/exfat.h| 2 --
>  fs/erofs/internal.h  | 2 --
>  fs/ext4/ext4.h   | 1 -
>  fs/f2fs/f2fs.h   | 1 -
>  fs/xfs/xfs_linux.h   | 1 -
>  include/linux/jbd2.h | 1 -
>  include/uapi/asm-generic/errno.h | 1 +
>  7 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
> index 84de1123e178..3cf7e54af0b7 100644
> --- a/drivers/staging/exfat/exfat.h
> +++ b/drivers/staging/exfat/exfat.h
> @@ -30,8 +30,6 @@
>  #undef DEBUG
>  #endif
>  
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
> -
>  #define DENTRY_SIZE  32  /* dir entry size */
>  #define DENTRY_SIZE_BITS 5
>  
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 544a453f3076..3980026a8882 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -425,7 +425,5 @@ static inline int z_erofs_init_zip_subsystem(void) { 
> return 0; }
>  static inline void z_erofs_exit_zip_subsystem(void) {}
>  #endif   /* !CONFIG_EROFS_FS_ZIP */
>  
> -#define EFSCORRUPTEDEUCLEAN /* Filesystem is corrupted */
> -
>  #endif   /* __EROFS_INTERNAL_H */
>  
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 03db3e71676c..a86c2585457d 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3396,6 +3396,5 @@ static inline int ext4_buffer_uptodate(struct 
> buffer_head *bh)
>  #endif   /* __KERNEL__ */
>  
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  
>  #endif   /* _EXT4_H */
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4024790028aa..04ebe77569a3 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3752,6 +3752,5 @@ static inline bool is_journalled_quota(struct 
> f2fs_sb_info *sbi)
>  }
>  
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  
>  #endif /* _LINUX_F2FS_H */
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index ca15105681ca..3409d02a7d21 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -123,7 +123,6 @@ typedef __u32 xfs_nlink_t;
>  
>  #define ENOATTR  ENODATA /* Attribute not found */
>  #define EWRONGFS EINVAL  /* Mount with wrong filesystem type */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
>  
>  #define SYNCHRONIZE()barrier()
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 564793c24d12..1ecd3859d040 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1657,6 +1657,5 @@ static inline tid_t  
> jbd2_get_latest_transaction(journal_t *journal)
>  #endif   /* __KERNEL__ */
>  
>  #define EFSBADCRCEBADMSG /* Bad CRC detected */
> -#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>  
>  #endif   /* _LINUX_JBD2_H */
> diff --git a/include/uapi/asm-generic/errno.h 
> b/include/uapi/asm-generic/errno.h
> index cf9c51ac49f9..1d5ffdf54cb0 100644
> --- a/include/uapi/asm-generic/errno.h
> +++ b/include/uapi/asm-generic/errno.h
> @@ -98,6 +98,7 @@
>  #define  EINPROGRESS 115 /* Operation now in progress */
>  #define  ESTALE  116 /* Stale file handle */
>  #define  EUCLEAN 117 /* Structure needs cleaning */
> +#define  EFSCORRUPTEDEUCLEAN
>  #define  ENOTNAM 118 /* Not a XENIX named type file */
>  #define  ENAVAIL 119 /* No XENIX semaphores available */
>  #define  EISNAM  120 /* Is a named type file */
> -- 
> 2.24.0.rc1
> 
-- 
Jan Kara 
SUSE Labs, CR


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [RFC PATCH v2] f2fs: support data compression

[Jaegeuk Kim]

On 10/29, Chao Yu wrote:
> This patch tries to support compression in f2fs.
> 
> - New term named cluster is defined as basic unit of compression, file can
> be divided into multiple clusters logically. One cluster includes 4 << n
> (n >= 0) logical pages, compression size is also cluster size, each of
> cluster can be compressed or not.
> 
> - In cluster metadata layout, one special flag is used to indicate cluster
> is compressed one or normal one, for compressed cluster, following metadata
> maps cluster to [1, 4 << n - 1] physical blocks, in where f2fs stores
> data including compress header and compressed data.
> 
> - In order to eliminate write amplification during overwrite, F2FS only
> support compression on write-once file, data can be compressed only when
> all logical blocks in file are valid and cluster compress ratio is lower
> than specified threshold.
> 
> - To enable compression on regular inode, there are three ways:
> * chattr +c file
> * chattr +c dir; touch dir/file
> * mount w/ -o compress_extension=ext; touch file.ext
> 
> Compress metadata layout:
>  [Dnode Structure]
>  +---+
>  | cluster 1 | cluster 2 | . | cluster N |
>  +---+
>  .   .   .   .
>.   ..  .
>   . Compressed Cluster   ..Normal Cluster 
>.
> +--+-+-+-+  
> +-+-+-+-+
> |compr flag| block 1 | block 2 | block 3 |  | block 1 | block 2 | block 3 | 
> block 4 |
> +--+-+-+-+  
> +-+-+-+-+
>. .
>  .   .
>.   .
>   +-+-+--++
>   | data length | data chksum | reserved |  compressed data   |
>   +-+-+--++
> 
> Changelog:
> 
> 20190326:
> - fix error handling of read_end_io().
> - remove unneeded comments in f2fs_encrypt_one_page().
> 
> 20190327:
> - fix wrong use of f2fs_cluster_is_full() in f2fs_mpage_readpages().
> - don't jump into loop directly to avoid uninitialized variables.
> - add TODO tag in error path of f2fs_write_cache_pages().
> 
> 20190328:
> - fix wrong merge condition in f2fs_read_multi_pages().
> - check compressed file in f2fs_post_read_required().
> 
> 20190401
> - allow overwrite on non-compressed cluster.
> - check cluster meta before writing compressed data.
> 
> 20190402
> - don't preallocate blocks for compressed file.
> 
> - add lz4 compress algorithm
> - process multiple post read works in one workqueue
>   Now f2fs supports processing post read work in multiple workqueue,
>   it shows low performance due to schedule overhead of multiple
>   workqueue executing orderly.
> 
> 20190921
> - compress: support buffered overwrite
> C: compress cluster flag
> V: valid block address
> N: NEW_ADDR
> 
> One cluster contain 4 blocks
> 
>  before overwrite   after overwrite
> 
> - ->  CVNN
> - CVNN->  
> 
> - CVNN->  CVNN
> - CVNN->  CVVV
> 
> - CVVV->  CVNN
> - CVVV->  CVVV
> 
> 20191029
> - add kconfig F2FS_FS_COMPRESSION to isolate compression related
> codes, add kconfig F2FS_FS_{LZO,LZ4} to cover backend algorithm.
> note that: will remove lzo backend if Jaegeuk agreed that too.
> - update codes according to Eric's comments.
> 
> [Jaegeuk Kim]
> - add tracepoint for f2fs_{,de}compress_pages()
> - fix many bugs and add some compression stats
> 
> Signed-off-by: Chao Yu 
> Signed-off-by: Jaegeuk Kim 
> ---
>  Documentation/filesystems/f2fs.txt |   52 ++
>  fs/f2fs/Kconfig|   23 +
>  fs/f2fs/Makefile   |1 +
>  fs/f2fs/compress.c | 1090 
>  fs/f2fs/data.c |  504 +++--
>  fs/f2fs/debug.c|6 +
>  fs/f2fs/f2fs.h |  231 +-
>  fs/f2fs/file.c |  150 +++-
>  fs/f2fs/inode.c|   43 ++
>  fs/f2fs/namei.c|   45 ++
>  fs/f2fs/segment.c  |5 +-
>  fs/f2fs/segment.h  |   12 -
>  fs/f2fs/super.c|  112 ++-
>  fs/f2fs/sysfs.c|7 +
>  include/linux/f2fs_fs.h|   11 +
>  include/trace/events/f2fs.h|   99 +++
>  16 files changed, 2290 insertions(+), 101 deletions(-)
>  create mode 100644 fs/f2fs/compress.c
> 
> diff --git a/Documentation/filesystems/f2fs.txt 
> 

Re: [f2fs-dev] [PATCH 2/2] f2fs: support data compression

[Jaegeuk Kim]

Hi Chao,

On 10/31, Chao Yu wrote:
> On 2019/10/31 0:50, Eric Biggers wrote:
> > No, just use kvmalloc().  The whole point of kvmalloc() is that it tries
> > kmalloc() and then falls back to vmalloc() if it fails.
> 
> Okay, it's fine to me, let me fix this in another patch.

I've fixed some bugs. (e.g., mmap) Please apply this in your next patch, so that
I can continue to test new version as early as possible.

With this patch, I could boot up a device and install some apps successfully
with "compress_extension=*".

---
 fs/f2fs/compress.c | 229 +++--
 fs/f2fs/data.c | 109 +
 fs/f2fs/f2fs.h |  22 +++--
 fs/f2fs/file.c |  71 +-
 fs/f2fs/namei.c|  20 +++-
 5 files changed, 264 insertions(+), 187 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index f276d82a67aa..e03d57396ea2 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -77,8 +77,9 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
 {
struct f2fs_sb_info *sbi = F2FS_I_SB(cc->inode);
 
-   if (cc->rpages)
+   if (cc->nr_rpages)
return 0;
+
cc->rpages = f2fs_kzalloc(sbi, sizeof(struct page *) * cc->cluster_size,
GFP_KERNEL);
if (!cc->rpages)
@@ -88,7 +89,9 @@ int f2fs_init_compress_ctx(struct compress_ctx *cc)
 
 void f2fs_destroy_compress_ctx(struct compress_ctx *cc)
 {
-   kvfree(cc->rpages);
+   f2fs_reset_compress_ctx(cc);
+   WARN_ON(cc->nr_rpages);
+   kfree(cc->rpages);
 }
 
 int f2fs_compress_ctx_add_page(struct compress_ctx *cc, struct page *page)
@@ -224,16 +227,6 @@ static const struct f2fs_compress_ops f2fs_lz4_ops = {
.decompress_pages   = lz4_decompress_pages,
 };
 
-static void f2fs_release_cluster_pages(struct compress_ctx *cc)
-{
-   int i;
-
-   for (i = 0; i < cc->nr_rpages; i++) {
-   inode_dec_dirty_pages(cc->inode);
-   unlock_page(cc->rpages[i]);
-   }
-}
-
 static struct page *f2fs_grab_page(void)
 {
struct page *page;
@@ -321,6 +314,7 @@ static int f2fs_compress_pages(struct compress_ctx *cc)
trace_f2fs_compress_pages_end(cc->inode, cc->cluster_idx,
cc->clen, ret);
return 0;
+
 out_vunmap_cbuf:
vunmap(cc->cbuf);
 out_vunmap_rbuf:
@@ -393,10 +387,9 @@ void f2fs_decompress_pages(struct bio *bio, struct page 
*page, bool verity)
vunmap(dic->rbuf);
 out_free_dic:
f2fs_set_cluster_uptodate(dic->rpages, dic->cluster_size, ret, verity);
-   f2fs_free_dic(dic);
-
trace_f2fs_decompress_pages_end(dic->inode, dic->cluster_idx,
dic->clen, ret);
+   f2fs_free_dic(dic);
 }
 
 static bool is_page_in_cluster(struct compress_ctx *cc, pgoff_t index)
@@ -443,51 +436,25 @@ static bool __cluster_may_compress(struct compress_ctx 
*cc)
return false;
if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
return false;
-   if (f2fs_is_drop_cache(cc->inode))
-   return false;
-   if (f2fs_is_volatile_file(cc->inode))
-   return false;
 
offset = i_size & (PAGE_SIZE - 1);
if ((page->index > end_index) ||
(page->index == end_index && !offset))
return false;
+   if (page->index != start_idx_of_cluster(cc) + i)
+   return false;
}
return true;
 }
 
-int f2fs_is_cluster_existed(struct compress_ctx *cc)
-{
-   struct dnode_of_data dn;
-   unsigned int start_idx = start_idx_of_cluster(cc);
-   int ret;
-   int i;
-
-   set_new_dnode(, cc->inode, NULL, NULL, 0);
-   ret = f2fs_get_dnode_of_data(, start_idx, LOOKUP_NODE);
-   if (ret)
-   return ret;
-
-   for (i = 0; i < cc->cluster_size; i++, dn.ofs_in_node++) {
-   block_t blkaddr = datablock_addr(dn.inode, dn.node_page,
-   dn.ofs_in_node);
-   if (blkaddr == COMPRESS_ADDR) {
-   ret = 1;
-   break;
-   }
-   if (__is_valid_data_blkaddr(blkaddr)) {
-   ret = 2;
-   break;
-   }
-   }
-   f2fs_put_dnode();
-   return ret;
-}
-
 static bool cluster_may_compress(struct compress_ctx *cc)
 {
if (!f2fs_compressed_file(cc->inode))
return false;
+   if (f2fs_is_atomic_file(cc->inode))
+   return false;
+   if (f2fs_is_mmap_file(cc->inode))
+   return false;
if (!f2fs_cluster_is_full(cc))
return false;
return __cluster_may_compress(cc);
@@ -495,19 +462,59 @@ static bool 

Re: [f2fs-dev] [PATCH 1/2] f2fs: support aligned pinned file

[Jaegeuk Kim]

On 10/31, Chao Yu wrote:
> On 2019/10/31 0:09, Jaegeuk Kim wrote:
> > On 10/26, Chao Yu wrote:
> >> On 2019/10/26 2:18, Jaegeuk Kim wrote:
> >>> On 10/24, Chao Yu wrote:
>  Hi Jaegeuk,
> 
>  On 2019/10/23 1:16, Jaegeuk Kim wrote:
> > This patch supports 2MB-aligned pinned file, which can guarantee no GC 
> > at all
> > by allocating fully valid 2MB segment.
> >
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >  fs/f2fs/f2fs.h |  4 +++-
> >  fs/f2fs/file.c | 39 ++-
> >  fs/f2fs/recovery.c |  2 +-
> >  fs/f2fs/segment.c  | 21 -
> >  fs/f2fs/segment.h  |  2 ++
> >  fs/f2fs/super.c|  1 +
> >  fs/f2fs/sysfs.c|  2 ++
> >  7 files changed, 63 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index ca342f4c7db1..c681f51e351b 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -890,6 +890,7 @@ enum {
> > CURSEG_WARM_NODE,   /* direct node blocks of normal files */
> > CURSEG_COLD_NODE,   /* indirect node blocks */
> > NO_CHECK_TYPE,
> > +   CURSEG_COLD_DATA_PINNED,/* cold data for pinned file */
> >  };
> >  
> >  struct flush_cmd {
> > @@ -1301,6 +1302,7 @@ struct f2fs_sb_info {
> >  
> > /* threshold for gc trials on pinned files */
> > u64 gc_pin_file_threshold;
> > +   struct rw_semaphore pin_sem;
> >  
> > /* maximum # of trials to find a victim segment for SSR and GC 
> > */
> > unsigned int max_victim_search;
> > @@ -3116,7 +3118,7 @@ void f2fs_release_discard_addrs(struct 
> > f2fs_sb_info *sbi);
> >  int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool 
> > for_ra);
> >  void allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > unsigned int start, unsigned 
> > int end);
> > -void f2fs_allocate_new_segments(struct f2fs_sb_info *sbi);
> > +void f2fs_allocate_new_segments(struct f2fs_sb_info *sbi, int type);
> >  int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range);
> >  bool f2fs_exist_trim_candidates(struct f2fs_sb_info *sbi,
> > struct cp_control *cpc);
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 29bc0a542759..f6c038e8a6a7 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1545,12 +1545,41 @@ static int expand_inode_data(struct inode 
> > *inode, loff_t offset,
> > if (off_end)
> > map.m_len++;
> >  
> > -   if (f2fs_is_pinned_file(inode))
> > -   map.m_seg_type = CURSEG_COLD_DATA;
> > +   if (!map.m_len)
> > +   return 0;
> > +
> > +   if (f2fs_is_pinned_file(inode)) {
> > +   block_t len = (map.m_len >> sbi->log_blocks_per_seg) <<
> > +   sbi->log_blocks_per_seg;
> > +   block_t done = 0;
> > +
> > +   if (map.m_len % sbi->blocks_per_seg)
> > +   len += sbi->blocks_per_seg;
> >  
> > -   err = f2fs_map_blocks(inode, , 1, 
> > (f2fs_is_pinned_file(inode) ?
> > -   F2FS_GET_BLOCK_PRE_DIO :
> > -   
> > F2FS_GET_BLOCK_PRE_AIO));
> > +   map.m_len = sbi->blocks_per_seg;
> > +next_alloc:
> > +   mutex_lock(>gc_mutex);
> > +   err = f2fs_gc(sbi, true, false, NULL_SEGNO);
> > +   if (err && err != -ENODATA && err != -EAGAIN)
> > +   goto out_err;
> 
>  To grab enough free space?
> 
>  Shouldn't we call
> 
>   if (has_not_enough_free_secs(sbi, 0, 0)) {
>   mutex_lock(>gc_mutex);
>   f2fs_gc(sbi, false, false, NULL_SEGNO);
>   }
> >>>
> >>> The above calls gc all the time. Do we need this?
> >>
> >> Hmmm... my concern is why we need to run foreground GC even if there is 
> >> enough
> >> free space..
> > 
> > In order to get the free segment easily?
> 
> However, I doubt arbitrary foreground GC with greedy algorithm will ruin
> hot/cold data separation, actually, for sufficient free segment case, it's
> unnecessary to call FGGC.

Two things here; 1) I do worry much about when hitting boundary on
has_not_enough_free_secs() which calculates # of free segments based on # of
dirty pages. In this case, we just jump to allocate another free segment so
I think it increases the possiblity of no free segment panic. 2) Even if we
do call FGGC a lot, I don't think it will *ruin* the hot/cold data separation
a lot. Putting hot/warm blocks together into cold log will make another hot
segment which was 

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Chao Yu]

On 2019/10/31 0:33, Jaegeuk Kim wrote:
> On 10/30, Theodore Y. Ts'o wrote:
>> On Wed, Oct 30, 2019 at 11:50:37PM +0800, Gao Xiang wrote:
>>>
>>> So I'm curious about the original issue in commit 740432f83560
>>> ("f2fs: handle failed bio allocation"). Since f2fs manages multiple write
>>> bios with its internal fio but it seems the commit is not helpful to
>>> resolve potential mempool deadlock (I'm confused since no calltrace,
>>> maybe I'm wrong)...
>>
>> Two possibilities come to mind.  (a) It may be that on older kernels
>> (when f2fs is backported to older Board Support Package kernels from
>> the SOC vendors) didn't have the bio_alloc() guarantee, so it was
>> necessary on older kernels, but not on upstream, or (b) it wasn't
>> *actually* possible for bio_alloc() to fail and someone added the
>> error handling in 740432f83560 out of paranoia.
> 
> Yup, I was checking old device kernels but just stopped digging it out.
> Instead, I hesitate to apply this patch since I can't get why we need to
> get rid of this code for clean-up purpose. This may be able to bring
> some hassles when backporting to android/device kernels.

Jaegeuk,

IIUC, as getting hint from commit 740432f83560, are we trying to fix potential
deadlock like this?

In low memory scenario:

- f2fs_write_checkpoint()
 - block_operations()
  - f2fs_sync_node_pages()
   step 1) flush cold nodes, allocate new bio from mempool
   - bio_alloc()
- mempool_alloc()
   step 2) flush hot nodes, allocate a bio from mempool
   - bio_alloc()
- mempool_alloc()
   step 3) flush warm nodes, be stuck in below call path
   - bio_alloc()
- mempool_alloc()
 - loop to wait mempool element release, as we only
   reserved memory for two bio allocation, however above
   allocated two bios never getting submitted.

#define BIO_POOL_SIZE 2

If so, we need avoid using bioset, or introducing private bioset, at least
enlarging mempool size to three (adapt to total log headers' number)...

Thanks,

> 
>>
>> (Hence my suggestion that in the ext4 version of the patch, we add a
>> code comment justifying why there was no error checking, to make it
>> clear that this was a deliberate choice.  :-)
>>
>>  - Ted
> 
> 
> ___
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel