Re: [PATCH 2/9] cachefiles: drop direct usage of ->bmap method.

[Carlos Maiolino <cmaiolino@redhat.com>]

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