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@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. 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
