Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Eric Biggers <ebiggers@kernel.org>,
	Satya Tangirala <satyat@google.com>,
	linux-scsi@vger.kernel.org, Kim Boojin <boojin.kim@samsung.com>,
	Kuohong Wang <kuohong.wang@mediatek.com>,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v5 7/9] fscrypt: add inline encryption support
Date: Thu, 31 Oct 2019 14:21:03 -0700
Message-ID: <20191031212103.GA6244@infradead.org> (raw)
In-Reply-To: <20191031202125.GA111219@gmail.com>

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.

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-28  7:20 [PATCH v5 0/9] Inline Encryption Support Satya Tangirala
2019-10-28  7:20 ` [PATCH v5 1/9] block: Keyslot Manager for Inline Encryption Satya Tangirala
2019-10-31 18:04   ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 2/9] block: Add encryption context to struct bio Satya Tangirala
2019-10-31 18:16   ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 3/9] block: blk-crypto for Inline Encryption Satya Tangirala
2019-10-31 17:57   ` Christoph Hellwig
2019-10-31 20:50     ` Theodore Y. Ts'o
2019-10-31 21:22       ` Christoph Hellwig
2019-11-05  2:01         ` Eric Biggers
2019-11-05 15:39           ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 4/9] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala
2019-10-28  7:20 ` [PATCH v5 5/9] scsi: ufs: UFS crypto API Satya Tangirala
2019-10-31 18:23   ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS Satya Tangirala
2019-10-31 18:26   ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 7/9] fscrypt: add inline encryption support Satya Tangirala
2019-10-31 18:32   ` Christoph Hellwig
2019-10-31 20:21     ` Eric Biggers
2019-10-31 21:21       ` Christoph Hellwig [this message]
2019-10-31 22:25         ` Eric Biggers
2019-11-05  0:15           ` Christoph Hellwig
2019-11-05  1:03             ` Eric Biggers
2019-11-05  3:12         ` Eric Biggers
2019-10-28  7:20 ` [PATCH v5 8/9] f2fs: " Satya Tangirala
2019-10-31 17:14   ` Jaegeuk Kim
2019-10-28  7:20 ` [PATCH v5 9/9] ext4: " Satya Tangirala

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191031212103.GA6244@infradead.org \
    --to=hch@infradead.org \
    --cc=bmuthuku@qti.qualcomm.com \
    --cc=boojin.kim@samsung.com \
    --cc=ebiggers@kernel.org \
    --cc=kuohong.wang@mediatek.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=satyat@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git