ext4: underflow in alignment check

My static checker complains that this can underflow if arg is negative which is true.

Signed-off-by Dan Carpenter <[email protected]>

fs/ext4/super.c +1 -2

index b1a3471..a2a17e9 100644
@@ -1576,7 +1576,7 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 	} else if (token = Opt_min_batch_time) {
 		sbi->s_min_batch_time = arg;
 	} else if (token = Opt_inode_readahead_blks) {
-		if (arg && (arg > (1 << 30) || !is_power_of_2(arg))) {
+		if (arg && (arg > (1U << 30) || !is_power_of_2(arg))) {
 			ext4_msg(sb, KERN_ERR,
 				 "EXT4-fs: inode_readahead_blks must be "
 				 "0 or a power of 2 smaller than 2^31");
--

To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Dan Carpenter

My static checker complains that this can underflow if arg is negative which is true.

Signed-off-by Dan Carpenter <[email protected]>

How come? (1 << 30) fits even into 32-bit signed type. So where's the problem?

Quoted 8 lines · Jan Kara

Quoted 4 lines · Dan Carpenter

My static checker complains that this can underflow if arg is negative which is true.

Signed-off-by Dan Carpenter <[email protected]>

How come? (1 << 30) fits even into 32-bit signed type. So where's the problem?

Bad changelog... I was talking about a different issue. I was casting it to unsigned to take advantage of type promototion. Assume we have:

int arg = 1 << 31;

(arg > (1 << 30)) // <-- this is false
(arg > (1U << 30)) // <-- this is true so there is no underflow.

Quoted 10 lines · Dan Carpenter

Quoted 8 lines · Jan Kara

Quoted 4 lines · Dan Carpenter

My static checker complains that this can underflow if arg is negative which is true.

Signed-off-by Dan Carpenter <[email protected]>

How come? (1 << 30) fits even into 32-bit signed type. So where's the problem?

Quoted 7 lines

Bad changelog... I was talking about a different issue. I was casting it to unsigned to take advantage of type promototion. Assume we have:

int arg = 1 << 31;

(arg > (1 << 30)) // <-- this is false
(arg > (1U << 30)) // <-- this is true so there is no underflow.

I see, but match_int() - or more precisely match_number() returns -ERANGE when the number is > INT_MAX, subsequently we check whether the number is < 0 (Opt_inode_readahead_blks has flag MOPT_GTE0 set) and bail out if yes. So at the place you are modifying we are sure the number is in [0, INT_MAX]. So the condition (arg > (1 << 30)) is pointless - just defensive programming in case we decide e.g. to upgrade the type of 'arg' to long - but not wrong...