Re: [PATCH for-next 4/7] RDMA/rxe: Make pool lookup and alloc APIs type safe

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Dec 16, 2020 at 05:15:47PM -0600, Bob Pearson wrote:
> The allocate, lookup index, lookup key and cleanup routines
> in rxe_pool.c currently are not type safe against relocating
> the pelem field in the objects. Planned changes to move
> allocation of objects into rdma-core make addressing this a
> requirement.
> 
> Use the elem_offset field in rxe_type_info make these APIs
> safe against moving the pelem field.
> 
> Signed-off-by: Bob Pearson <rpearson@hpe.com>
>  drivers/infiniband/sw/rxe/rxe_pool.c | 55 +++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
> index 4d667b78af9b..2873ecfb84c2 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
> @@ -315,7 +315,9 @@ void rxe_drop_index(void *arg)
>  
>  void *rxe_alloc(struct rxe_pool *pool)
>  {
> +	struct rxe_type_info *info = &rxe_type_info[pool->type];
>  	struct rxe_pool_entry *elem;
> +	u8 *obj;
>  	unsigned long flags;
>  
>  	might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC));
> @@ -334,16 +336,17 @@ void *rxe_alloc(struct rxe_pool *pool)
>  	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>  		goto out_cnt;
>  
> -	elem = kzalloc(rxe_type_info[pool->type].size,
> -				 (pool->flags & RXE_POOL_ATOMIC) ?
> -				 GFP_ATOMIC : GFP_KERNEL);
> -	if (!elem)
> +	obj = kzalloc(info->size, (pool->flags & RXE_POOL_ATOMIC) ?
> +		      GFP_ATOMIC : GFP_KERNEL);
> +	if (!obj)
>  		goto out_cnt;
>  
> +	elem = (struct rxe_pool_entry *)(obj + info->elem_offset);

This makes more sense squashed with the

https://patchwork.kernel.org/project/linux-rdma/patch/20201216231550.27224-4-rpearson@hpe.com/

patch

But I would suggest a simpler answer, the pool APIs should always work
with 'struct rxe_pool_entry *'

Here it should return it:

struct rxe_pool_entry *_rxe_alloc(struct rxe_pool *pool, size_t size)

And then use a compile time macro to enforce that pelm is always
first:

#define rxe_alloc(pool, type) \
   container_of(_rxe_alloc(pool, sizeof(type) + BUILD_BUG_ON(offsetof(type, pelem) != 0)), type, pelm)

This would eliminate all the ugly void *'s from the API. Just the
allocator needs the BUILD_BUG_ON, but all the other places really
ought to use container_of() not void * to transform the rxe_pool_entry
to its larger struct.

Also I noticed this when I was looking - which also means size can be
removed from the struct rxe_type_info as the allocation here was the
only use.

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index d26730eec720c6..d515314510ed6a 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -128,14 +128,12 @@ int rxe_pool_init(
 	unsigned int		max_elem)
 {
 	int			err = 0;
-	size_t			size = rxe_type_info[type].size;
 
 	memset(pool, 0, sizeof(*pool));
 
 	pool->rxe		= rxe;
 	pool->type		= type;
 	pool->max_elem		= max_elem;
-	pool->elem_size		= ALIGN(size, RXE_POOL_ALIGN);
 	pool->flags		= rxe_type_info[type].flags;
 	pool->index.tree	= RB_ROOT;
 	pool->key.tree		= RB_ROOT;
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h
index 373e08554c1c9d..f34da4d33e243b 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.h
+++ b/drivers/infiniband/sw/rxe/rxe_pool.h
@@ -68,7 +68,6 @@ struct rxe_pool_entry {
 struct rxe_pool {
 	struct rxe_dev		*rxe;
 	rwlock_t		pool_lock; /* protects pool add/del/search */
-	size_t			elem_size;
 	struct kref		ref_cnt;
 	void			(*cleanup)(struct rxe_pool_entry *obj);
 	enum rxe_pool_state	state;