| Message ID | 1399590979-15331-3-git-send-email-zab@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote: > uncompress_inline() is silently dropping an error from > btrfs_decompress() after testing it and zeroing the page that was > supposed to hold decompressed data. This can silently turn compressed > inline data in to zeros if decompression fails due to corrupt compressed > data or memory allocation failure. > > I have no idea why uncompress_inline() is zeroing the page for an error > from btrfs_decompress() but not for the earlier ENOMEM from kmalloc(). > Can someone explain this? I don't see a reason for that in the current code, nor in the code that introduced it. The changes that could have affected that are in the error handling, but even with that in mind, the silent error does not make sense. > Signed-off-by: Zach Brown <zab@redhat.com> Reviewed-by: David Sterba <dsterba@suse.cz> And future ACK if you're going to kill the memset. -- 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 Fri, May 09, 2014 at 03:58:00PM +0200, David Sterba wrote: > On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote: > > uncompress_inline() is silently dropping an error from > > btrfs_decompress() after testing it and zeroing the page that was > > supposed to hold decompressed data. This can silently turn compressed > > inline data in to zeros if decompression fails due to corrupt compressed > > data or memory allocation failure. > > > > I have no idea why uncompress_inline() is zeroing the page for an error > > from btrfs_decompress() but not for the earlier ENOMEM from kmalloc(). > > Can someone explain this? > > I don't see a reason for that in the current code, nor in the code that > introduced it. The changes that could have affected that are in the error > handling, but even with that in mind, the silent error does not make > sense. Agreed. I removed it and will send out a v2 of the series. - z -- 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 Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote: > uncompress_inline() is silently dropping an error from > btrfs_decompress() after testing it and zeroing the page that was > supposed to hold decompressed data. This can silently turn compressed > inline data in to zeros if decompression fails due to corrupt compressed > data or memory allocation failure. > > I have no idea why uncompress_inline() is zeroing the page for an error > from btrfs_decompress() but not for the earlier ENOMEM from kmalloc(). > Can someone explain this? I guess that's because decompress() may have put part of real data on the page and then bail out, and we don't want those data to be exposed to users in this error case. And kmalloc() 's ENOMEM runs before that decompress(), page has whatever random data. -liubo > > The fix is to pass the error to its caller. Which still has a BUG_ON(). > So we fix that too. > > I verified this by manually forcing the error from btrfs_decompress() > for a silly named copy of od: > > if (!strcmp(current->comm, "failod")) > ret = -ENOMEM; > > # od -x /mnt/btrfs/dir/80 | head -1 > 0000000 3031 3038 310a 2d30 6f70 6e69 0a74 3031 > # echo 3 > /proc/sys/vm/drop_caches > # cp $(which od) /tmp/failod > # /tmp/failod -x /mnt/btrfs/dir/80 | head -1 > 0000000 0000 0000 0000 0000 0000 0000 0000 0000 > > Signed-off-by: Zach Brown <zab@redhat.com> > --- > fs/btrfs/inode.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 0c0bb45..fc89fa7 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -6091,7 +6091,7 @@ static noinline int uncompress_inline(struct btrfs_path *path, > kunmap_atomic(kaddr); > } > kfree(tmp); > - return 0; > + return ret; > } > > /* > @@ -6292,7 +6292,10 @@ next: > ret = uncompress_inline(path, inode, page, > pg_offset, > extent_offset, item); > - BUG_ON(ret); /* -ENOMEM */ > + if (ret) { > + err = ret; > + goto out; > + } > } else { > map = kmap(page); > read_extent_buffer(leaf, map + pg_offset, ptr, > -- > 1.8.1.4 > > -- > 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 Mon, May 12, 2014 at 11:00:23PM +0800, Liu Bo wrote: > On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote: > > uncompress_inline() is silently dropping an error from > > btrfs_decompress() after testing it and zeroing the page that was > > supposed to hold decompressed data. This can silently turn compressed > > inline data in to zeros if decompression fails due to corrupt compressed > > data or memory allocation failure. > > > > I have no idea why uncompress_inline() is zeroing the page for an error > > from btrfs_decompress() but not for the earlier ENOMEM from kmalloc(). > > Can someone explain this? > > I guess that's because decompress() may have put part of real data on the page > and then bail out, and we don't want those data to be exposed to users in this > error case. > > And kmalloc() 's ENOMEM runs before that decompress(), page has whatever random > data. But we don't return any data in case of error. In the unpatched code, there's no error so a zeroed page is returned, but this would not happen after Zach's fix. -- 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 05/12/2014 01:18 PM, David Sterba wrote: > On Mon, May 12, 2014 at 11:00:23PM +0800, Liu Bo wrote: >> On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote: >>> uncompress_inline() is silently dropping an error from >>> btrfs_decompress() after testing it and zeroing the page that was >>> supposed to hold decompressed data. This can silently turn compressed >>> inline data in to zeros if decompression fails due to corrupt compressed >>> data or memory allocation failure. >>> >>> I have no idea why uncompress_inline() is zeroing the page for an error >>> from btrfs_decompress() but not for the earlier ENOMEM from kmalloc(). >>> Can someone explain this? >> >> I guess that's because decompress() may have put part of real data on the page >> and then bail out, and we don't want those data to be exposed to users in this >> error case. >> >> And kmalloc() 's ENOMEM runs before that decompress(), page has whatever random >> data. > > But we don't return any data in case of error. In the unpatched code, > there's no error so a zeroed page is returned, but this would not happen > after Zach's fix. I dug a little more, the zeroing goes all the way back to the original compression code. It looks like some paranoia of mine. I'd say its fine to leave off as long as we don't mark the page uptodate. -chris -- 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 0c0bb45..fc89fa7 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -6091,7 +6091,7 @@ static noinline int uncompress_inline(struct btrfs_path *path, kunmap_atomic(kaddr); } kfree(tmp); - return 0; + return ret; } /* @@ -6292,7 +6292,10 @@ next: ret = uncompress_inline(path, inode, page, pg_offset, extent_offset, item); - BUG_ON(ret); /* -ENOMEM */ + if (ret) { + err = ret; + goto out; + } } else { map = kmap(page); read_extent_buffer(leaf, map + pg_offset, ptr,
uncompress_inline() is silently dropping an error from btrfs_decompress() after testing it and zeroing the page that was supposed to hold decompressed data. This can silently turn compressed inline data in to zeros if decompression fails due to corrupt compressed data or memory allocation failure. I have no idea why uncompress_inline() is zeroing the page for an error from btrfs_decompress() but not for the earlier ENOMEM from kmalloc(). Can someone explain this? The fix is to pass the error to its caller. Which still has a BUG_ON(). So we fix that too. I verified this by manually forcing the error from btrfs_decompress() for a silly named copy of od: if (!strcmp(current->comm, "failod")) ret = -ENOMEM; # od -x /mnt/btrfs/dir/80 | head -1 0000000 3031 3038 310a 2d30 6f70 6e69 0a74 3031 # echo 3 > /proc/sys/vm/drop_caches # cp $(which od) /tmp/failod # /tmp/failod -x /mnt/btrfs/dir/80 | head -1 0000000 0000 0000 0000 0000 0000 0000 0000 0000 Signed-off-by: Zach Brown <zab@redhat.com> --- fs/btrfs/inode.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)