On Thu, Aug 20, 2020 at 05:58:53PM +0800, Qu Wenruo wrote:

On 2020/8/20 下午5:50, David Sterba wrote:

quoted

On Thu, Aug 20, 2020 at 07:14:13AM +0800, Qu Wenruo wrote:

quoted

quoted

quoted

+static inline int check_eb_range(const struct extent_buffer *eb,
+				 unsigned long start, unsigned long len)
+{
+	/* start, start + len should not go beyond eb->len nor overflow */
+	if (unlikely(start > eb->len || start + len > eb->len ||
+		     len > eb->len)) {

Can the number of condition be reduced? If 'start + len' overflows, then
we don't need to check 'start > eb->len', and for the case where
start = 1024 and len = -1024 the 'len > eb-len' would be enough.

I'm afraid not.
Although 'start > eb->len || len > eb->len' is enough to detect overflow
case, it no longer detects cases like 'start = 2k, len = 3k' while
eb->len == 4K case.

So we still need all 3 checks.

I was suggesting 'start + len > eb->len', not 'start > eb-len'.

"start > eb->len" is implied by "start + len > eb->len".

start > eb->len is not implied if (start + len) over flows.

E.g. start = -2K, len = 2k, eb->len = 4K. We can still pass !(start +
len > eb->len || len > eb->len).

In short, if we want overflow check along with each one checked, we
really need 3 checks.

So what if we add overflow check, that would catch negative start or
negative length, and then do start + len > eb->len?

The check_setget_bounds is different becasue the len is known at compile
time so the overflows can't happen in the same way as for the eb range,
so this this confused me first.

	check_add_overflow(start, len) || start + len > eb->len