`On Mon, 2019-04-01 at 16:55 +0100, Dmitry Safonov wrote:> Hi Alexander,> > On 4/1/19 4:40 PM, Alexander Duyck wrote:> > > @@ -333,8 +328,7 @@ static struct tnode *tnode_alloc(int bits)> > >  {> > >  	size_t size;> > > > > > -	/* verify bits is within bounds */> > > -	if (bits > TNODE_VMALLOC_MAX)> > > +	if ((BITS_PER_LONG <= KEYLENGTH) && unlikely(bits >= BITS_PER_LONG))> > >  		return NULL;> > > > > >  	/* determine size and verify it is non-zero and didn't overflow */> > > > I think it would be better if we left TNODE_VMALLOC_MAX instead of> > replacing it with BITS_PER_LONG. This way we know that we are limited> > by the size of the node on 32b systems, and by the KEYLENGTH on 64b> > systems. The basic idea is to maintain the logic as to why we are doing> > it this way instead of just burying things by using built in constants> > that are close enough to work.> > > > So for example I believe TNODE_VMALLOC_MAX is 31 on a 32b system.> > This is also true after the change: bits == 31 will *not* return.Actually now that I think about it TNODE_VMALLOC_MAX is likely muchless than 31. The logic that we have to be concerned with is:	size = TNODE_SIZE(1ul << bits);If size is a 32b value, and the size of a pointer is 4 bytes, then ourupper limit is roughly ilog2((4G - 28) / 4), which comes out to 29.What we are trying to avoid is overflowing the size variable, notactually limiting the vmalloc itself.> > The> > main reason for that is because we have to subtract the TNODE_SIZE from> > the upper limit for size. By replacing TNODE_VMALLOC_MAX with> > BITS_PER_LONG that becomes abstracted away and it becomes more likely> > that somebody will mishandle it later.> > So, I wanted to remove run-time check here on x86_64..> I could do it by adding !CONFIG_64BIT around the check.I have no problem with that. All I am suggesting is that if at allpossible we should use TNODE_VMALLOC_MAX instead of BITS_PER_LONG.> But, I thought about the value of the check:> I believe it's here not to limit maximum allocated size:> kzalloc()/vzalloc() will fail and we should be fine with that.No, the problem is we don't want to overflow size. The allocation willsucceed, but give us a much smaller allocation then we expected.> In my opinion it's rather to check that (1UL << bits) wouldn't result in UB.Sort of, however we have to keep mind that 1ul << bits is an index soit is also increased by the size of a pointer. As such the logic mightbe better expressed as sizeof(void*) << bits.`