2022-04-18 08:31:17

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 2/3] f2fs: notify when device not supprt inlinecrypt

On Mon, Apr 18, 2022 at 02:33:11PM +0800, Fengnan Chang via Linux-f2fs-devel wrote:
> Notify when mount filesystem with -o inlinecrypt option, but the device
> not support inlinecrypt.
>
> Signed-off-by: Fengnan Chang <[email protected]>

You didn't include a cover letter in this patchset. Can you explain what
problem this patchset is meant to solve?

Note that there are multiple factors that affect whether inline encryption can
be used with a particular file, such as whether the device supports the required
encryption mode, data unit size, and data unit number size. So your warning
might not trigger even if inline encryption can't be used. Also, your warning
will never trigger if the kernel has CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK=y.

I recently sent out a patch that makes fs/crypto/ consistently log a message
when starting to use an encryption implementation for the first time:
https://lore.kernel.org/r/[email protected]. It
already did this for the crypto API, but not blk-crypto. Being silent for
blk-crypto was somewhat of an oversight. These log messages make it clear which
encryption implementations are in use.

Does that patch solve the problem you are trying to solve?

- Eric


2022-04-18 10:23:16

by 常凤楠

[permalink] [raw]
Subject: RE: [PATCH 2/3] f2fs: notify when device not supprt inlinecrypt

> -----Original Message-----
> From: Eric Biggers <[email protected]>
> Sent: Monday, April 18, 2022 3:22 PM
> To: ?????? <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/3] f2fs: notify when device not supprt inlinecrypt
>
> On Mon, Apr 18, 2022 at 02:33:11PM +0800, Fengnan Chang via
> Linux-f2fs-devel wrote:
> > Notify when mount filesystem with -o inlinecrypt option, but the
> > device not support inlinecrypt.
> >
> > Signed-off-by: Fengnan Chang <[email protected]>
>
> You didn't include a cover letter in this patchset. Can you explain what
> problem this patchset is meant to solve?

What I'm try to make is when devices not support inlinecrypt, do not show inlinecrypt in mount option.
When I test fscrypt first, it make me confused. Not a real problem, just make this logical more reasonable.
Do you think this needs to be revised?

>
> Note that there are multiple factors that affect whether inline encryption can
> be used with a particular file, such as whether the device supports the
> required encryption mode, data unit size, and data unit number size. So
> your warning might not trigger even if inline encryption can't be used. Also,
> your warning will never trigger if the kernel has
> CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK=y.

I get it.

>
> I recently sent out a patch that makes fs/crypto/ consistently log a message
> when starting to use an encryption implementation for the first time:
> https://lore.kernel.org/r/[email protected].
> It already did this for the crypto API, but not blk-crypto. Being silent for
> blk-crypto was somewhat of an oversight. These log messages make it clear
> which encryption implementations are in use.
>
> Does that patch solve the problem you are trying to solve?

I think it's a different point.

Thanks.

>
> - Eric

2022-04-19 00:06:48

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 2/3] f2fs: notify when device not supprt inlinecrypt

On Mon, Apr 18, 2022 at 07:34:52AM +0000, 常凤楠 wrote:
> > -----Original Message-----
> > From: Eric Biggers <[email protected]>
> > Sent: Monday, April 18, 2022 3:22 PM
> > To: 常凤楠 <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH 2/3] f2fs: notify when device not supprt inlinecrypt
> >
> > On Mon, Apr 18, 2022 at 02:33:11PM +0800, Fengnan Chang via
> > Linux-f2fs-devel wrote:
> > > Notify when mount filesystem with -o inlinecrypt option, but the
> > > device not support inlinecrypt.
> > >
> > > Signed-off-by: Fengnan Chang <[email protected]>
> >
> > You didn't include a cover letter in this patchset. Can you explain what
> > problem this patchset is meant to solve?
>
> What I'm try to make is when devices not support inlinecrypt, do not show inlinecrypt in mount option.
> When I test fscrypt first, it make me confused. Not a real problem, just make this logical more reasonable.
> Do you think this needs to be revised?

Well, I'm just not sure we should do this, or at least by itself, given that
support for inline encryption is not an either-or thing, and the inlinecrypt
mount option is already documented to apply only to files where inline
encryption can be used. See Documentation/filesystems/fscrypt.rst:

Note that the "inlinecrypt" mount option just specifies to use inline
encryption when possible; it doesn't force its use. fscrypt will
still fall back to using the kernel crypto API on files where the
inline encryption hardware doesn't have the needed crypto capabilities
(e.g. support for the needed encryption algorithm and data unit size)
and where blk-crypto-fallback is unusable. (For blk-crypto-fallback
to be usable, it must be enabled in the kernel configuration with
CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK=y.)

And Documentation/admin-guide/ext4.rst and Documentation/filesystems/f2fs.rst:

When possible, encrypt/decrypt the contents of encrypted files using the
blk-crypto framework rather than filesystem-layer encryption. ...

If we do want to warn when inlinecrypt is given but inline encryption cannot be
used, your patchset isn't enough since it only covers the case where no form of
inline encryption is available at all, and not the case where some form of
inline encryption is available but the filesystem can't use it.

- Eric

2022-04-19 14:20:45

by 常凤楠

[permalink] [raw]
Subject: RE: [PATCH 2/3] f2fs: notify when device not supprt inlinecrypt



> -----Original Message-----
> From: Eric Biggers <[email protected]>
> Sent: Tuesday, April 19, 2022 6:27 AM
> To: 常凤楠 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/3] f2fs: notify when device not supprt inlinecrypt
>
> On Mon, Apr 18, 2022 at 07:34:52AM +0000, 常凤楠 wrote:
> > > -----Original Message-----
> > > From: Eric Biggers <[email protected]>
> > > Sent: Monday, April 18, 2022 3:22 PM
> > > To: 常凤楠 <[email protected]>
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH 2/3] f2fs: notify when device not supprt
> > > inlinecrypt
> > >
> > > On Mon, Apr 18, 2022 at 02:33:11PM +0800, Fengnan Chang via
> > > Linux-f2fs-devel wrote:
> > > > Notify when mount filesystem with -o inlinecrypt option, but the
> > > > device not support inlinecrypt.
> > > >
> > > > Signed-off-by: Fengnan Chang <[email protected]>
> > >
> > > You didn't include a cover letter in this patchset. Can you explain
> > > what problem this patchset is meant to solve?
> >
> > What I'm try to make is when devices not support inlinecrypt, do not show
> inlinecrypt in mount option.
> > When I test fscrypt first, it make me confused. Not a real problem, just make
> this logical more reasonable.
> > Do you think this needs to be revised?
>
> Well, I'm just not sure we should do this, or at least by itself, given that
> support for inline encryption is not an either-or thing, and the inlinecrypt
> mount option is already documented to apply only to files where inline
> encryption can be used. See Documentation/filesystems/fscrypt.rst:
>
> Note that the "inlinecrypt" mount option just specifies to use inline
> encryption when possible; it doesn't force its use. fscrypt will
> still fall back to using the kernel crypto API on files where the
> inline encryption hardware doesn't have the needed crypto capabilities
> (e.g. support for the needed encryption algorithm and data unit size)
> and where blk-crypto-fallback is unusable. (For blk-crypto-fallback
> to be usable, it must be enabled in the kernel configuration with
> CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK=y.) and not for other reasons
>
> And Documentation/admin-guide/ext4.rst and
> Documentation/filesystems/f2fs.rst:
>
> When possible, encrypt/decrypt the contents of encrypted files using the
> blk-crypto framework rather than filesystem-layer encryption. ...
>
> If we do want to warn when inlinecrypt is given but inline encryption cannot
> be used, your patchset isn't enough since it only covers the case where no
> form of inline encryption is available at all, and not the case where some form
> of inline encryption is available but the filesystem can't use it.

Thanks for your explanation, this patchset has too many case to forget to handle...
Back to my first thought, maybe there should have one sysfs node to indicate the
device support inlinecrypt or not ? So user can know it's device not support inlinecrypt
and not for other reasons.

Thanks.

>
> - Eric