On Wed, Aug 14, 2019 at 01:15:35PM +0200, Christoph Hellwig wrote: > On Thu, Aug 08, 2019 at 10:27:37AM +0200, Carlos Maiolino wrote: > > + block = page->index; > > + block <<= shift; > > Can't this cause overflows? Hmm, I honestly don't know. I did look at the code, and I couldn't really spot anything concrete. Maybe if the block size is much smaller than PAGE_SIZE, but I am really not sure. Bear in mind though, I didn't change the logic here at all. I just reused one variable instead of juggling both (block0 and block) old variables. So, if this really can overflow, the code is already buggy even without my patch, I'm CC'ing dhowells just in case. > > > + > > + ret = bmap(inode, &block); > > + ASSERT(!ret); > > I think we want some real error handling here instead of just an > assert.. I left this ASSERT() here, to match the current logic. By now, the only error we can get is -EINVAL, which basically says ->bmap() method does not exist, which is basically what does happen today with: ASSERT(inode->i_mapping->a_ops->bmap); But I do agree, it will be better to provide some sort of error handling here, maybe I should do something like: ASSERT(ret == -EINVAL) to keep the logic exactly the same and do not blow up in the future if/when we expand possible error values from bmap() What you think? -- Carlos
