[PATCH] Btrfs: report errors when checksum is not found

[PATCH] Btrfs: report errors when checksum is not found

[Liu Bo]

When btrfs fails the checksum check, it'll fill the whole page with
"1".

However, if %csum_expected is 0 (which means there is no checksum), then
for some unknown reason, we just pretend that the read is correct, so
userspace would be confused about the dilemma that read is successful but
getting a page with all content being "1".

This can happen due to a bug in btrfs-convert.

This fixes it by always returning errors if checksum doesn't match.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ef3c98c..8a4d8ee 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3151,8 +3151,6 @@ static int __readpage_endio_check(struct inode *inode,
 	memset(kaddr + pgoff, 1, len);
 	flush_dcache_page(page);
 	kunmap_atomic(kaddr);
-	if (csum_expected == 0)
-		return 0;
 	return -EIO;
 }
 
-- 
2.9.4

Re: [PATCH] Btrfs: report errors when checksum is not found

[David Sterba]

On Tue, Jul 11, 2017 at 02:43:16PM -0600, Liu Bo wrote:
> When btrfs fails the checksum check, it'll fill the whole page with
> "1".

One could ask, why is the page filled with 1s. Brought by commit
07157aacb1ecd394a54949 from 2007, without mentioning any justification.
I'm more inclined to revisit this behaviour and drop it eventually.

> However, if %csum_expected is 0 (which means there is no checksum), then
> for some unknown reason, we just pretend that the read is correct, so
> userspace would be confused about the dilemma that read is successful but
> getting a page with all content being "1".

Here 'no checksum' means that no checksum was found but was expected,
right? An EIO would fail the read, I don't see a reason why the page
needs to be "zeroed". The contents would be inaccessible anyway.

> This can happen due to a bug in btrfs-convert.
> 
> This fixes it by always returning errors if checksum doesn't match.

Independent of the above, this fix makes sense.

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH] Btrfs: report errors when checksum is not found

[Liu Bo]

On Wed, Jul 12, 2017 at 04:40:36PM +0200, David Sterba wrote:
> On Tue, Jul 11, 2017 at 02:43:16PM -0600, Liu Bo wrote:
> > When btrfs fails the checksum check, it'll fill the whole page with
> > "1".
> 
> One could ask, why is the page filled with 1s. Brought by commit
> 07157aacb1ecd394a54949 from 2007, without mentioning any justification.
> I'm more inclined to revisit this behaviour and drop it eventually.
> 
> > However, if %csum_expected is 0 (which means there is no checksum), then
> > for some unknown reason, we just pretend that the read is correct, so
> > userspace would be confused about the dilemma that read is successful but
> > getting a page with all content being "1".
> 
> Here 'no checksum' means that no checksum was found but was expected,
> right?

Yes, no checksum was found.

> An EIO would fail the read, I don't see a reason why the page
> needs to be "zeroed". The contents would be inaccessible anyway.
>

Right, resetting page's content is needed when we return 0 instead of
-EIO.  I guess it was introduced for testing.  So yes, I'm glad to
remove that part, will do in a v2.

> > This can happen due to a bug in btrfs-convert.
> > 
> > This fixes it by always returning errors if checksum doesn't match.
> 
> Independent of the above, this fix makes sense.
> 
> Reviewed-by: David Sterba <dsterba@suse.com>

Thank you for the comments.

Thanks,

-liubo

Re: [PATCH] Btrfs: report errors when checksum is not found

[Liu Bo]

On Wed, Jul 12, 2017 at 11:46:29AM -0600, Liu Bo wrote:
> On Wed, Jul 12, 2017 at 04:40:36PM +0200, David Sterba wrote:
> > On Tue, Jul 11, 2017 at 02:43:16PM -0600, Liu Bo wrote:
> > > When btrfs fails the checksum check, it'll fill the whole page with
> > > "1".
> > 
> > One could ask, why is the page filled with 1s. Brought by commit
> > 07157aacb1ecd394a54949 from 2007, without mentioning any justification.
> > I'm more inclined to revisit this behaviour and drop it eventually.
> > 
> > > However, if %csum_expected is 0 (which means there is no checksum), then
> > > for some unknown reason, we just pretend that the read is correct, so
> > > userspace would be confused about the dilemma that read is successful but
> > > getting a page with all content being "1".
> > 
> > Here 'no checksum' means that no checksum was found but was expected,
> > right?
> 
> Yes, no checksum was found.
> 
> > An EIO would fail the read, I don't see a reason why the page
> > needs to be "zeroed". The contents would be inaccessible anyway.
> >
> 
> Right, resetting page's content is needed when we return 0 instead of
> -EIO.  I guess it was introduced for testing.  So yes, I'm glad to
> remove that part, will do in a v2.
>

Since this __readpage_endio_check() is also called by directIO's
btrfs_retry_endio(), in the dio case, userspace can read out the page
content.

For that reason, I think we would have to keep it and return errors to
userspace.

Thanks,

-liubo

> > > This can happen due to a bug in btrfs-convert.
> > > 
> > > This fixes it by always returning errors if checksum doesn't match.
> > 
> > Independent of the above, this fix makes sense.
> > 
> > Reviewed-by: David Sterba <dsterba@suse.com>
> 
> Thank you for the comments.
> 
> Thanks,
> 
> -liubo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: report errors when checksum is not found

[David Sterba]

On Wed, Jul 12, 2017 at 03:35:43PM -0600, Liu Bo wrote:
> On Wed, Jul 12, 2017 at 11:46:29AM -0600, Liu Bo wrote:
> > On Wed, Jul 12, 2017 at 04:40:36PM +0200, David Sterba wrote:
> > > On Tue, Jul 11, 2017 at 02:43:16PM -0600, Liu Bo wrote:
> > > > When btrfs fails the checksum check, it'll fill the whole page with
> > > > "1".
> > > 
> > > One could ask, why is the page filled with 1s. Brought by commit
> > > 07157aacb1ecd394a54949 from 2007, without mentioning any justification.
> > > I'm more inclined to revisit this behaviour and drop it eventually.
> > > 
> > > > However, if %csum_expected is 0 (which means there is no checksum), then
> > > > for some unknown reason, we just pretend that the read is correct, so
> > > > userspace would be confused about the dilemma that read is successful but
> > > > getting a page with all content being "1".
> > > 
> > > Here 'no checksum' means that no checksum was found but was expected,
> > > right?
> > 
> > Yes, no checksum was found.
> > 
> > > An EIO would fail the read, I don't see a reason why the page
> > > needs to be "zeroed". The contents would be inaccessible anyway.
> > >
> > 
> > Right, resetting page's content is needed when we return 0 instead of
> > -EIO.  I guess it was introduced for testing.  So yes, I'm glad to
> > remove that part, will do in a v2.
> >
> 
> Since this __readpage_endio_check() is also called by directIO's
> btrfs_retry_endio(), in the dio case, userspace can read out the page
> content.
> 
> For that reason, I think we would have to keep it and return errors to
> userspace.

We can decide what to do in case of the error:

a) this is what we read from the disk and is presumably the expected
   content yet with some corruptions. Ie. "this is what we found but EIO
   tells you that it's not valid, you may still salvage some data"

b) if the page contents is random and possibly contains sensitive data
   of somebody else, then it should be zeroed.

Although zeoring will be completely safe, I know people are asking about
a way to get the data even if the checksum does not match (other than
manually locating the data on the device). This would need to audit the
call paths leading to __readpage_endio_check.