| Message ID | 233ed975-982d-422a-b498-410f71d8a101@moroto.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | RDMA/mlx4: Make check for invalid flags stricter | expand |
On Thu, Jun 29, 2023 at 09:07:37AM +0300, Dan Carpenter wrote: > This code is trying to ensure that only the flags specified in the list > are allowed. The problem is that ucmd->rx_hash_fields_mask is a u64 and > the flags are an enum which is treated as a u32 in this context. That > means the test doesn't check whether the highest 32 bits are zero. > > Fixes: 4d02ebd9bbbd ("IB/mlx4: Fix RSS hash fields restrictions") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > The MLX4_IB_RX_HASH_INNER value is declared as > "MLX4_IB_RX_HASH_INNER = 1ULL << 31," which suggests that it > should be type ULL but that doesn't work. It will still be basically a > u32. (Enum types are weird). Can you please elaborate more why enum left to be int? It is surprise to me. Thanks
On Tue, Jul 04, 2023 at 04:38:41PM +0300, Leon Romanovsky wrote: > On Thu, Jun 29, 2023 at 09:07:37AM +0300, Dan Carpenter wrote: > > This code is trying to ensure that only the flags specified in the list > > are allowed. The problem is that ucmd->rx_hash_fields_mask is a u64 and > > the flags are an enum which is treated as a u32 in this context. That > > means the test doesn't check whether the highest 32 bits are zero. > > > > Fixes: 4d02ebd9bbbd ("IB/mlx4: Fix RSS hash fields restrictions") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > The MLX4_IB_RX_HASH_INNER value is declared as > > "MLX4_IB_RX_HASH_INNER = 1ULL << 31," which suggests that it > > should be type ULL but that doesn't work. It will still be basically a > > u32. (Enum types are weird). > > Can you please elaborate more why enum left to be int? It is surprise to me. Enum types are not defined very strictly in C so it's up to the compiler. Clang, GCC and Sparse implement them in the same way. They default to u32 unless the values can't fit, then they become whatever type fits. So if you have a negative, it becomes an int or a big value changes the type to unsigned long. Since 1ULL < 31 fits in u32 the type is just u32. regards, dan carpenter #include <stdio.h> enum example_one { VAL = 1ULL << 31, }; enum example_two { NEGATIVE = -2, }; enum example_three { BIG = 1ULL << 32, }; int main(void) { enum example_one one = -1; enum example_two two = -1; enum example_three three = -1; printf("%lu\n", sizeof(enum example_one)); if (one > 0) printf("one unsigned\n"); if (two < 0) printf("two signed\n"); printf("%lu\n", sizeof(enum example_three)); if (three > 0) printf("three unsigned\n"); return 0; }
On Tue, Jul 04, 2023 at 05:07:17PM +0300, Dan Carpenter wrote: > On Tue, Jul 04, 2023 at 04:38:41PM +0300, Leon Romanovsky wrote: > > On Thu, Jun 29, 2023 at 09:07:37AM +0300, Dan Carpenter wrote: > > > This code is trying to ensure that only the flags specified in the list > > > are allowed. The problem is that ucmd->rx_hash_fields_mask is a u64 and > > > the flags are an enum which is treated as a u32 in this context. That > > > means the test doesn't check whether the highest 32 bits are zero. > > > > > > Fixes: 4d02ebd9bbbd ("IB/mlx4: Fix RSS hash fields restrictions") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > The MLX4_IB_RX_HASH_INNER value is declared as > > > "MLX4_IB_RX_HASH_INNER = 1ULL << 31," which suggests that it > > > should be type ULL but that doesn't work. It will still be basically a > > > u32. (Enum types are weird). > > > > Can you please elaborate more why enum left to be int? It is surprise to me. > > Enum types are not defined very strictly in C so it's up to the > compiler. > > Clang, GCC and Sparse implement them in the same way. They default > to u32 unless the values can't fit, then they become whatever type fits. > So if you have a negative, it becomes an int or a big value changes the > type to unsigned long. > > Since 1ULL < 31 fits in u32 the type is just u32. Thanks for an explanation, I found the relevant sentence in the C standard as well. "The choice of type is implementation-deļ¬ned, 128) but shall be capable of representing the values of all the members of the enumeration." Thanks > > regards, > dan carpenter > > #include <stdio.h> > > enum example_one { > VAL = 1ULL << 31, > }; > > enum example_two { > NEGATIVE = -2, > }; > > enum example_three { > BIG = 1ULL << 32, > }; > > int main(void) > { > enum example_one one = -1; > enum example_two two = -1; > enum example_three three = -1; > > printf("%lu\n", sizeof(enum example_one)); > > if (one > 0) > printf("one unsigned\n"); > if (two < 0) > printf("two signed\n"); > > printf("%lu\n", sizeof(enum example_three)); > if (three > 0) > printf("three unsigned\n"); > > return 0; > } > >
On Tue, Jul 04, 2023 at 05:07:17PM +0300, Dan Carpenter wrote: > On Tue, Jul 04, 2023 at 04:38:41PM +0300, Leon Romanovsky wrote: > > On Thu, Jun 29, 2023 at 09:07:37AM +0300, Dan Carpenter wrote: > > > This code is trying to ensure that only the flags specified in the list > > > are allowed. The problem is that ucmd->rx_hash_fields_mask is a u64 and > > > the flags are an enum which is treated as a u32 in this context. That > > > means the test doesn't check whether the highest 32 bits are zero. > > > > > > Fixes: 4d02ebd9bbbd ("IB/mlx4: Fix RSS hash fields restrictions") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > The MLX4_IB_RX_HASH_INNER value is declared as > > > "MLX4_IB_RX_HASH_INNER = 1ULL << 31," which suggests that it > > > should be type ULL but that doesn't work. It will still be basically a > > > u32. (Enum types are weird). > > > > Can you please elaborate more why enum left to be int? It is surprise to me. > > Enum types are not defined very strictly in C so it's up to the > compiler. > > Clang, GCC and Sparse implement them in the same way. They default > to u32 unless the values can't fit, then they become whatever type fits. > So if you have a negative, it becomes an int or a big value changes the > type to unsigned long. It is worse than that, the standard has some wording that the constants have to be 'int' so gcc makes most of those values 'int' when it computes the | across them. There is some 'beyond C' behavior here where gcc will make only the non-int representable constants some larger type (ie MLX4_IB_RX_HASH_INNER is u32 and MLX4_IB_RX_HASH_SRC_IPV4 is int) This is totally un-intuitive that the type of the enum constants is not the type of the enum itself (which is u32 in this case), but here we are. C23 finally fixes this by brining the C++ feature of explicitly typed enums and then the enum and all the constants have a consistent, specified, type. But this is definately the right thing to do, I actually thought we had a function specifically for doing this test becaue of how tricky ~ is... Jason
On Thu, 29 Jun 2023 09:07:37 +0300, Dan Carpenter wrote: > This code is trying to ensure that only the flags specified in the list > are allowed. The problem is that ucmd->rx_hash_fields_mask is a u64 and > the flags are an enum which is treated as a u32 in this context. That > means the test doesn't check whether the highest 32 bits are zero. > > Applied, thanks! [1/1] RDMA/mlx4: Make check for invalid flags stricter https://git.kernel.org/rdma/rdma/c/d64b1ee12a1680 Best regards,
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c index 456656617c33..9d08aa99f3cb 100644 --- a/drivers/infiniband/hw/mlx4/qp.c +++ b/drivers/infiniband/hw/mlx4/qp.c @@ -565,15 +565,15 @@ static int set_qp_rss(struct mlx4_ib_dev *dev, struct mlx4_ib_rss *rss_ctx, return (-EOPNOTSUPP); } - if (ucmd->rx_hash_fields_mask & ~(MLX4_IB_RX_HASH_SRC_IPV4 | - MLX4_IB_RX_HASH_DST_IPV4 | - MLX4_IB_RX_HASH_SRC_IPV6 | - MLX4_IB_RX_HASH_DST_IPV6 | - MLX4_IB_RX_HASH_SRC_PORT_TCP | - MLX4_IB_RX_HASH_DST_PORT_TCP | - MLX4_IB_RX_HASH_SRC_PORT_UDP | - MLX4_IB_RX_HASH_DST_PORT_UDP | - MLX4_IB_RX_HASH_INNER)) { + if (ucmd->rx_hash_fields_mask & ~(u64)(MLX4_IB_RX_HASH_SRC_IPV4 | + MLX4_IB_RX_HASH_DST_IPV4 | + MLX4_IB_RX_HASH_SRC_IPV6 | + MLX4_IB_RX_HASH_DST_IPV6 | + MLX4_IB_RX_HASH_SRC_PORT_TCP | + MLX4_IB_RX_HASH_DST_PORT_TCP | + MLX4_IB_RX_HASH_SRC_PORT_UDP | + MLX4_IB_RX_HASH_DST_PORT_UDP | + MLX4_IB_RX_HASH_INNER)) { pr_debug("RX Hash fields_mask has unsupported mask (0x%llx)\n", ucmd->rx_hash_fields_mask); return (-EOPNOTSUPP);
This code is trying to ensure that only the flags specified in the list are allowed. The problem is that ucmd->rx_hash_fields_mask is a u64 and the flags are an enum which is treated as a u32 in this context. That means the test doesn't check whether the highest 32 bits are zero. Fixes: 4d02ebd9bbbd ("IB/mlx4: Fix RSS hash fields restrictions") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- The MLX4_IB_RX_HASH_INNER value is declared as "MLX4_IB_RX_HASH_INNER = 1ULL << 31," which suggests that it should be type ULL but that doesn't work. It will still be basically a u32. (Enum types are weird). drivers/infiniband/hw/mlx4/qp.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)