| Message ID | 20170711204316.11283-1-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> -- 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
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 -- 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
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 -- 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
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. -- 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
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; }
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(-)