[PATCH] RDMA/mlx4: Make check for invalid flags stricter

[PATCH] RDMA/mlx4: Make check for invalid flags stricter

[Dan Carpenter]

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(-)

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);
-- 
2.39.2

Re: [PATCH] RDMA/mlx4: Make check for invalid flags stricter

[Leon Romanovsky]

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

Re: [PATCH] RDMA/mlx4: Make check for invalid flags stricter

[Dan Carpenter]

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;
}

Re: [PATCH] RDMA/mlx4: Make check for invalid flags stricter

[Leon Romanovsky]

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-defined, 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;
> }
> 
> 

Re: [PATCH] RDMA/mlx4: Make check for invalid flags stricter

[Jason Gunthorpe]

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

Re: [PATCH] RDMA/mlx4: Make check for invalid flags stricter

[Leon Romanovsky]

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,
-- 
Leon Romanovsky <leon@kernel.org>