Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <41cf7793-0816-461f-b8c6-82b3eb1cfeba@linux.dev>
Date:   Wed, 29 Nov 2023 16:15:33 +0100
MIME-Version: 1.0
Subject: Re: [PATCH v6 11/11] blksnap: prevents using devices with data
 integrity or inline encryption
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     axboe@kernel.dk, hch@infradead.org, corbet@lwn.net,
        snitzer@kernel.org, mingo@redhat.com, peterz@infradead.org,
        juri.lelli@redhat.com, viro@zeniv.linux.org.uk, brauner@kernel.org,
        linux-block@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        Sergei Shtepa <sergei.shtepa@veeam.com>
References: <20231124165933.27580-1-sergei.shtepa@linux.dev>
 <20231124165933.27580-12-sergei.shtepa@linux.dev>
 <20231127224719.GD1463@sol.localdomain>
 <6cabaa42-c366-4928-8294-ad261dae0043@linux.dev>
 <20231128171823.GA1148@sol.localdomain>
Content-Language: en-US
From:   Sergei Shtepa <sergei.shtepa@linux.dev>
In-Reply-To: <20231128171823.GA1148@sol.localdomain>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/28/23 18:18, Eric Biggers wrote:
> On Tue, Nov 28, 2023 at 12:00:17PM +0100, Sergei Shtepa wrote:
>> But I haven't tested the code on a device where hardware inline encryption is
>> available. I would be glad if anyone could help with this.
>>> Anyway, this patch is better than ignoring the problem.  It's worth noting,
>>> though, that this patch does not prevent blksnap from being set up on a block
>>> device on which blk-crypto-fallback is already being used (or will be used).
>>> When that happens, I/O will suddenly start failing.  For usability reasons,
>>> ideally that would be prevented somehow.
>> I didn't observe any failures during testing. It's just that the snapshot
>> image shows files with encrypted names and data. Backup in this case is
>> useless. Unfortunately, there is no way to detect a blk-crypto-fallback on
>> the block device filter level.
> Huh, I thought that this patch is supposed to exclude blk-crypto-fallback too.
> __submit_bio() calls bio->bi_bdev->bd_filter->ops->submit_bio(bio) before
> blk_crypto_bio_prep(), so doesn't your check of ->bi_crypt_context exclude
> blk-crypto-fallback?

Thank you, Eric. You're right.
The filter handle unencrypted data when using blk-crypto-fallback.
Indeed, the I/O unit has an encryption context.

And yes, the word "Hardware" is not necessary.
- pr_err_once("Hardware inline encryption is not supported\n");
+ pr_err_once("Inline encryption is not supported\n");

> 
> I think you're right that it might actually be fine to use blksnap with
> blk-crypto-fallback, provided that the encryption is done first.  I would like
> to see a proper explanation of that, though.  And we still have this patch which
> claims that it doesn't work, which is confusing.

I found a bug in my test. I was let down by the cache.
I redid the test and posted it.
Link: https://github.com/veeam/blksnap/blob/stable-v2.0/tests/8000-inline-encryption.sh

When the bi_crypt_context is detected in the write I/O unit, the snapshot
image is marked as corrupted. The COW algorithm is not executed.
The blksnap code does not allow data leakage.

For a disk with hardware encryption, a block device cannot be added to the
snapshot since the encryption context for the disk will be detected for it.
Unfortunately, it is impossible to detect the presence of a blk-crypto-fallback
when adding a block device to the snapshot.

So, I think that the patch fully ensures the confidentiality of data when
using inline encryption. However, it does not allow to perform a backup
for this case.

If we make a filter handling point in the __submit_bio() function after
calling blk_crypto_bio_prep(), then this will not change the situation for
the case of hardware encryption. But the filter will never know what the
blk-crypto-fallback is being used. I have no opinion whether it will be better.