| Message ID | 20171003173110.14789-1-kreijack@libero.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 03, 2017 at 07:31:10PM +0200, Goffredo Baroncelli wrote: > From: Goffredo Baroncelli <kreijack@inwind.it> > > Jean-Denis Girard noticed commit c821e7f3 "pass bytes to > btrfs_bio_alloc" (https://patchwork.kernel.org/patch/9763081/) introduces a > regression on 32 bit machines. > When CONFIG_LBDAF is _not_ defined (CONFIG_LBDAF == Support for large > (2TB+) block devices and files) sector_t is 32 bit on 32bit machines. > > In the function submit_extent_page, 'sector' (which is sector_t type) is > multiplied by 512 to convert it from sectors to bytes, leading to an > overflow when the disk is bigger than 4GB (!). That's not good. There are some known typedefs that hide the 32bit/64bit differences but the LBDAF and sector_t is new to me. Thanks for the report and fix, I'll get it to linus/master tree in the next batch so it can go to stable tree. I've seen sector_t used in places where it is not necessary so I'll try to minimize the usage and more surprises from the << 9 shifts. Reviewed-by: David Sterba <dsterba@suse.com> Fixes: c821e7f3 ("btrfs: pass bytes to btrfs_bio_alloc") CC: stable@vger.kernel.org # 4.13+ -- 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, Oct 04, 2017 at 04:22:28PM +0200, David Sterba wrote: > On Tue, Oct 03, 2017 at 07:31:10PM +0200, Goffredo Baroncelli wrote: > > From: Goffredo Baroncelli <kreijack@inwind.it> > > > > Jean-Denis Girard noticed commit c821e7f3 "pass bytes to > > btrfs_bio_alloc" (https://patchwork.kernel.org/patch/9763081/) introduces a > > regression on 32 bit machines. > > When CONFIG_LBDAF is _not_ defined (CONFIG_LBDAF == Support for large > > (2TB+) block devices and files) sector_t is 32 bit on 32bit machines. > > > > In the function submit_extent_page, 'sector' (which is sector_t type) is > > multiplied by 512 to convert it from sectors to bytes, leading to an > > overflow when the disk is bigger than 4GB (!). > > That's not good. There are some known typedefs that hide the 32bit/64bit > differences but the LBDAF and sector_t is new to me. Thanks for the > report and fix, I'll get it to linus/master tree in the next batch so it > can go to stable tree. > > I've seen sector_t used in places where it is not necessary so I'll try > to minimize the usage and more surprises from the << 9 shifts. > > Reviewed-by: David Sterba <dsterba@suse.com> > Fixes: c821e7f3 ("btrfs: pass bytes to btrfs_bio_alloc") > CC: stable@vger.kernel.org # 4.13+ However, this sector_t is passed from its callers, e.g. __do_readpage() { sector_t sector; ... sector = em->block_start >> 9; ... } if sector_t is 32bit, the above %sector could also lose high bits. Some cleanups are needed to use u64 directly. Even with this patch, I suspect that there might be errors from block layer as sector_t is used everywhere in block layer. For a btrfs FS that is created and used on 64bit system, it'll be causing trouble anyway if letting it mount 32bit system, lets refuse the mount firstly. 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, Oct 04, 2017 at 11:13:51AM -0600, Liu Bo wrote: > On Wed, Oct 04, 2017 at 04:22:28PM +0200, David Sterba wrote: > > On Tue, Oct 03, 2017 at 07:31:10PM +0200, Goffredo Baroncelli wrote: > > > From: Goffredo Baroncelli <kreijack@inwind.it> > > > > > > Jean-Denis Girard noticed commit c821e7f3 "pass bytes to > > > btrfs_bio_alloc" (https://patchwork.kernel.org/patch/9763081/) introduces a > > > regression on 32 bit machines. > > > When CONFIG_LBDAF is _not_ defined (CONFIG_LBDAF == Support for large > > > (2TB+) block devices and files) sector_t is 32 bit on 32bit machines. > > > > > > In the function submit_extent_page, 'sector' (which is sector_t type) is > > > multiplied by 512 to convert it from sectors to bytes, leading to an > > > overflow when the disk is bigger than 4GB (!). > > > > That's not good. There are some known typedefs that hide the 32bit/64bit > > differences but the LBDAF and sector_t is new to me. Thanks for the > > report and fix, I'll get it to linus/master tree in the next batch so it > > can go to stable tree. > > > > I've seen sector_t used in places where it is not necessary so I'll try > > to minimize the usage and more surprises from the << 9 shifts. > > > > Reviewed-by: David Sterba <dsterba@suse.com> > > Fixes: c821e7f3 ("btrfs: pass bytes to btrfs_bio_alloc") > > CC: stable@vger.kernel.org # 4.13+ > > However, this sector_t is passed from its callers, e.g. > > __do_readpage() > { > sector_t sector; > ... > sector = em->block_start >> 9; > ... > } > > if sector_t is 32bit, the above %sector could also lose high bits. > Some cleanups are needed to use u64 directly. I have the sector_t cleanups ready, will post them tomorrow. > Even with this patch, I suspect that there might be errors from block > layer as sector_t is used everywhere in block layer. Yeah, we'd have to audit all interface calls where the parameters are sector_t, I've addressed only those in btrfs code. > For a btrfs FS that is created and used on 64bit system, it'll be > causing trouble anyway if letting it mount 32bit system, lets refuse > the mount firstly. As long as the sector_t is 64bit on 32bit system, we can let the mount proceed. For 32bit sector_t on 32bit system we could refuse to mount in case one of the devices is larger than 2TB (provided that we don't lose the bits from the conversion similar to what this patch does). This would need check on the device add and replace side, but otherwise I think we should try to keep the systems working even in the limited environments. -- 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 10/04/2017 07:13 PM, Liu Bo wrote: > On Wed, Oct 04, 2017 at 04:22:28PM +0200, David Sterba wrote: >> On Tue, Oct 03, 2017 at 07:31:10PM +0200, Goffredo Baroncelli wrote: >>> From: Goffredo Baroncelli <kreijack@inwind.it> >>> >>> Jean-Denis Girard noticed commit c821e7f3 "pass bytes to >>> btrfs_bio_alloc" (https://patchwork.kernel.org/patch/9763081/) introduces a >>> regression on 32 bit machines. >>> When CONFIG_LBDAF is _not_ defined (CONFIG_LBDAF == Support for large >>> (2TB+) block devices and files) sector_t is 32 bit on 32bit machines. >>> >>> In the function submit_extent_page, 'sector' (which is sector_t type) is >>> multiplied by 512 to convert it from sectors to bytes, leading to an >>> overflow when the disk is bigger than 4GB (!). >> >> That's not good. There are some known typedefs that hide the 32bit/64bit >> differences but the LBDAF and sector_t is new to me. Thanks for the >> report and fix, I'll get it to linus/master tree in the next batch so it >> can go to stable tree. >> >> I've seen sector_t used in places where it is not necessary so I'll try >> to minimize the usage and more surprises from the << 9 shifts. >> >> Reviewed-by: David Sterba <dsterba@suse.com> >> Fixes: c821e7f3 ("btrfs: pass bytes to btrfs_bio_alloc") >> CC: stable@vger.kernel.org # 4.13+ > > However, this sector_t is passed from its callers, e.g. > > __do_readpage() > { > sector_t sector; > ... > sector = em->block_start >> 9; > ... > } > > if sector_t is 32bit, the above %sector could also lose high bits. > Some cleanups are needed to use u64 directly. If sector_t is 32bit, the kernel can't access disk bigger than 2TB. So I suppose that block_start is less than 4GB. > > Even with this patch, I suspect that there might be errors from block > layer as sector_t is used everywhere in block layer. > > For a btrfs FS that is created and used on 64bit system, it'll be > causing trouble anyway if letting it mount 32bit system, lets refuse > the mount firstly. I think that the check should be: if sizeof(sector_t) < 8 && filesystem_end > 2TB then do_not_mount_it I am not sure what the other fs does > > Thanks, > > -liubo >
On Wed, Oct 04, 2017 at 08:23:05PM +0200, David Sterba wrote: > On Wed, Oct 04, 2017 at 11:13:51AM -0600, Liu Bo wrote: > > On Wed, Oct 04, 2017 at 04:22:28PM +0200, David Sterba wrote: > > > On Tue, Oct 03, 2017 at 07:31:10PM +0200, Goffredo Baroncelli wrote: > > > > From: Goffredo Baroncelli <kreijack@inwind.it> > > > > > > > > Jean-Denis Girard noticed commit c821e7f3 "pass bytes to > > > > btrfs_bio_alloc" (https://patchwork.kernel.org/patch/9763081/) introduces a > > > > regression on 32 bit machines. > > > > When CONFIG_LBDAF is _not_ defined (CONFIG_LBDAF == Support for large > > > > (2TB+) block devices and files) sector_t is 32 bit on 32bit machines. > > > > > > > > In the function submit_extent_page, 'sector' (which is sector_t type) is > > > > multiplied by 512 to convert it from sectors to bytes, leading to an > > > > overflow when the disk is bigger than 4GB (!). > > > > > > That's not good. There are some known typedefs that hide the 32bit/64bit > > > differences but the LBDAF and sector_t is new to me. Thanks for the > > > report and fix, I'll get it to linus/master tree in the next batch so it > > > can go to stable tree. > > > > > > I've seen sector_t used in places where it is not necessary so I'll try > > > to minimize the usage and more surprises from the << 9 shifts. > > > > > > Reviewed-by: David Sterba <dsterba@suse.com> > > > Fixes: c821e7f3 ("btrfs: pass bytes to btrfs_bio_alloc") > > > CC: stable@vger.kernel.org # 4.13+ > > > > However, this sector_t is passed from its callers, e.g. > > > > __do_readpage() > > { > > sector_t sector; > > ... > > sector = em->block_start >> 9; > > ... > > } > > > > if sector_t is 32bit, the above %sector could also lose high bits. > > Some cleanups are needed to use u64 directly. > > I have the sector_t cleanups ready, will post them tomorrow. > > > Even with this patch, I suspect that there might be errors from block > > layer as sector_t is used everywhere in block layer. > > Yeah, we'd have to audit all interface calls where the parameters are > sector_t, I've addressed only those in btrfs code. > > > For a btrfs FS that is created and used on 64bit system, it'll be > > causing trouble anyway if letting it mount 32bit system, lets refuse > > the mount firstly. > > As long as the sector_t is 64bit on 32bit system, we can let the mount > proceed. For 32bit sector_t on 32bit system we could refuse to mount in > case one of the devices is larger than 2TB (provided that we don't lose > the bits from the conversion similar to what this patch does). > > This would need check on the device add and replace side, but otherwise > I think we should try to keep the systems working even in the limited > environments. Also sb->s_maxbytes needs to be updated so that we can get -EFBIG. 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
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 12ab19a4b93e..970190cd347e 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2801,7 +2801,7 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree, } } - bio = btrfs_bio_alloc(bdev, sector << 9); + bio = btrfs_bio_alloc(bdev, (u64)sector << 9); bio_add_page(bio, page, page_size, offset); bio->bi_end_io = end_io_func; bio->bi_private = tree;