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@xxxxxxxxx> > > > > > > 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@xxxxxxxx> > > Fixes: c821e7f3 ("btrfs: pass bytes to btrfs_bio_alloc") > > CC: stable@xxxxxxxxxxxxxxx # 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
