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@xxxxxxx>
> 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@xxxxxxx/
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;
