Re: [PATCH 2/2] Fix the rename detection limit checking

On Fri, 14 Sep 2007, Linus Torvalds wrote:

... and we also make sure that we don't overflow when doing the matrix 
size calculation.

Side note: by "make sure", I don't really mean a total guarantee.

We could be even more careful here. In particular:

 - we later do end up allocating the matrix with 

	sizeof(*mx) * num_create * num_src

   and I didn't actually fix the overflow that is possible due to the
   "sizeof(*mx)" multiplication.

 - even after we've checked that not *both* of the source and destination 
   counts are larger than the rename_limit, we could still overflow the 
   multiplication in just the limit check.

.. but with the rename_limit being set to 100, in practice neither of 
these are really even close to realistic (ie you'd need to have less than 
100 new files, and deleted over twenty million files to overflow, or vice 
versa).

So with a rename_limit of 100, it's all good (I'm pretty sure you'd have 
*other* issues long before you'd hit the integer overflows on renames ;)

But if somebody sets the rename_limit to something bigger, it gets 
increasingly easier to screw it up.

If somebody wants to be *really* careful, they'd need to do something like

	unsigned long max;

	/* This isn't going to overflow, since we limited 'rename_limit' */
	max = rename_limit * rename_limit;

	/*
	 * But we should also check that multiplying by "sizeof(*mx)" 
	 * won't make it overlof either..
	 */
	while ((sizeof(*mx) * max) / sizeof(*mx) != max)
		max >>= 1;

	/*
	 * And then avoid multiplying "rename_dst_nr" and "rename_src_nr"
	 * together by turning it into a division instead
	 */
	if (max / rename_dst_nr > rename_src_nr)
		goto cleanup;

but the patch I sent out was the "obvious" first one that at least avoided 
the overflow for the triggerable case that Dmitry had, and as per above 
likely in all reasonable cases...

			Linus